Summary: | [BREWMP] Don't pass PassOwnPtr in makeAllDirectories | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kwang Yul Seo <skyul> | ||||||||
Component: | Platform | Assignee: | 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
Kwang Yul Seo
2010-07-12 01:00:42 PDT
Created attachment 61194 [details]
Patch
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.
Comment on attachment 61194 [details]
Patch
Wait, no, this patch is wrong.
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. 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?
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. Created attachment 66220 [details]
Patch
Change rawFileMgr to fileManager as Adam suggested.
Comment on attachment 66220 [details]
Patch
LGTM.
Comment on attachment 66220 [details] Patch Clearing flags on attachment: 66220 Committed r68342: <http://trac.webkit.org/changeset/68342> All reviewed patches have been landed. Closing bug. |