Bug 42058 – [BREWMP] Don't pass PassOwnPtr in makeAllDirectories

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Kwang Yul Seo]

PassOwnPtr's release is renamed to leakPtr.

[Kwang Yul Seo]

Created attachment 61194 [details]
Patch

[Darin Adler]

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 on attachment 61194 [details]
Patch

Wait, no, this patch is wrong.

[Darin Adler]

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]

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 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]

Created attachment 66220 [details]
Patch

Change rawFileMgr to fileManager as Adam suggested.

[Eric Seidel (no email)]

Comment on attachment 66220 [details]
Patch

LGTM.

[WebKit Commit Bot]

Comment on attachment 66220 [details]
Patch

Clearing flags on attachment: 66220

Committed r68342: <http://trac.webkit.org/changeset/68342>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.