Bug 42058

Summary: [BREWMP] Don't pass PassOwnPtr in makeAllDirectories
Product: WebKit Reporter: Kwang Yul Seo <skyul>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: beergun, commit-queue, joybro201, xhiloh
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Other   
Bug Depends on:    
Bug Blocks: 33564    
Attachments:
Description Flags
Patch
darin: review-, darin: commit-queue-
Patch
abarth: review-
Patch none

Kwang Yul Seo
Reported 2010-07-12 01:00:42 PDT
PassOwnPtr's release is renamed to leakPtr.
Attachments
Patch (1.44 KB, patch)
2010-07-12 01:02 PDT, Kwang Yul Seo 		darin: review-
darin: commit-queue-
DetailsFormatted DiffDiff
Patch (2.40 KB, patch)
2010-08-27 08:40 PDT, Kwang Yul Seo 		abarth: review-
DetailsFormatted DiffDiff
Patch (2.40 KB, patch)
2010-09-01 09:27 PDT, Kwang Yul Seo 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Kwang Yul Seo
Comment 1 2010-07-12 01:02:15 PDT
Created attachment 61194 [details] Patch
Darin Adler
Comment 2 2010-07-12 13:44:03 PDT
Comment on attachment 61194 [details] Patch But why do you want to leak a pointer here? If the makeAllDirectories function needs to take a file manager object it should take a PassOwnPtr.
Darin Adler
Comment 3 2010-07-12 13:44:46 PDT
Comment on attachment 61194 [details] Patch Wait, no, this patch is wrong.
Darin Adler
Comment 4 2010-07-12 13:47:52 PDT
Comment on attachment 61194 [details] Patch > @@ -144,7 +144,7 @@ static bool makeAllDirectories(PassOwnPt > } > > if (lastDivPos > 0) { > - if (!makeAllDirectories(fileMgr.release(), path.substring(0, lastDivPos))) > + if (!makeAllDirectories(fileMgr.leakPtr(), path.substring(0, lastDivPos))) > return false; This is buggy and needs a change in design. If this function fails, you'll be left with 0 in fileMgr and not be able to complete the function. Since makeAllDirectories takes a pointer to an object that it destroys, it can't call itself, and then go on to use that object. We'll need to refactor this code to find the mistake. You could get this to compile by just passing fileMgr here. That would change the behavior back to what it was before, but there is a bug here because fileMgr will be 0 once it's passed to another PassOwnPtr. > - return makeAllDirectories(fileMgr.release(), canonicalPath(path)); > + return makeAllDirectories(fileMgr.leakPtr(), canonicalPath(path)); This code was correct before. fileMgr here is an OwnPtr, and calling release on it gives you a PassOwnPtr, which is exactly what you need to call makeAllDirectories.
Kwang Yul Seo
Comment 5 2010-08-27 08:40:02 PDT
Created attachment 65717 [details] Patch Change makeAllDirectories to take a raw FileMgr pointer. I am not sure if this is the best way to avoid the problem. Any suggestion?
Adam Barth
Comment 6 2010-08-31 19:41:10 PDT
Comment on attachment 65717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=65717&action=prettypatch > WebCore/platform/brew/FileSystemBrew.cpp:134 > -static bool makeAllDirectories(PassOwnPtr<IFileMgr> fileMgr, const String& path) > +static bool makeAllDirectories(IFileMgr* rawFileMgr, const String& path) rawFileMgr isn't the right name here, for sure. Perhaps fileManager? I'd have to look at the larger context to see what the memory ownership is like here.
Kwang Yul Seo
Comment 7 2010-09-01 09:27:20 PDT
Created attachment 66220 [details] Patch Change rawFileMgr to fileManager as Adam suggested.
Eric Seidel (no email)
Comment 8 2010-09-26 10:12:01 PDT
Comment on attachment 66220 [details] Patch LGTM.
WebKit Commit Bot
Comment 9 2010-09-26 10:25:35 PDT
Comment on attachment 66220 [details] Patch Clearing flags on attachment: 66220 Committed r68342: <http://trac.webkit.org/changeset/68342>
WebKit Commit Bot
Comment 10 2010-09-26 10:25:41 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.