1483042 - Another (uncaught) rooting hazard in CopyingStructuredCloneReadCallback

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P1)

Description

[Steve Fink [:sfink] [:s:]]

I'm still trying to land all of my hazard fixes that detect this stuff, and I think this might have been recently introduced?

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Remove more ~RefPtr that fire after a Rooted has been unwrapped for a return value

At any rate, it appears to be a real hazard. Let me know if you'd rather I fixed this by limited the RefPtr scopes instead.

:baku reviewed the previous one of these in bug 1480640, but he's on PTO right now.

Comment 2

[Jan Varga [:janv]]

Comment on attachment 8999761 [details] [diff] [review]
Remove more ~RefPtr that fire after a Rooted has been unwrapped for a return value

Review of attachment 8999761 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Remove more ~RefPtr that fire after a Rooted has been unwrapped for a return value

Oops, sorry. That patch was fine, except for the not compiling part. ToJSValue needs a File&, which it can get out of a RefPtr<File>& but not an already_AddRefed<File>.

Here's one using an extra scope. I'm unfamiliar with RefPtr and already_AddRefed and things, so I don't know if there's another alternative -- passing *file.mBlob->toFile().take() would compile, but I don't know if it does the right thing with ownership?

Comment 4

[Jan Varga [:janv]]

(In reply to Steve Fink [:sfink] [:s:] from comment #3)
> Here's one using an extra scope. I'm unfamiliar with RefPtr and
> already_AddRefed and things, so I don't know if there's another alternative
> -- passing *file.mBlob->toFile().take() would compile, but I don't know if
> it does the right thing with ownership?

ToFile() adds a new strong reference and returns it as already_AddRefed.
Yes, doing take() would compile, but it would also leak since take() transfers the ownership.
You would have to do something like this:

File* file = file.mBlob->ToFile();
doStuff(file);
file->Release();

but doing addref/release manually is usually not recommended, smart pointers are safer.

So it seems adding a scope for that code is probably the cleanest solution.

Comment 5

[Jan Varga [:janv]]

(In reply to Jan Varga [:janv] from comment #4)
> File* file = file.mBlob->ToFile();

that should be:
File* file = file.mBlob->ToFile().take();

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2a1d27fb7a9
Remove more ~RefPtr that fire after a Rooted has been unwrapped for a return value, r=janv

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/c2a1d27fb7a9