Closed Bug 1483042 Opened 2 years ago Closed 2 years ago

Another (uncaught) rooting hazard in CopyingStructuredCloneReadCallback

Categories

(Core :: JavaScript Engine, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(1 file, 1 obsolete file)

I'm still trying to land all of my hazard fixes that detect this stuff, and I think this might have been recently introduced?
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.
Attachment #8999761 - Flags: review?(bugs)
Attachment #8999761 - Flags: review?(bugs) → review?(jvarga)
Summary: Another (uncaught) roozing hazard in CopyingStructuredCloneReadCallback → Another (uncaught) rooting hazard in CopyingStructuredCloneReadCallback
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!
Attachment #8999761 - Flags: review?(jvarga) → review+
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?
Attachment #8999811 - Flags: review?(jvarga)
Attachment #8999761 - Attachment is obsolete: true
(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.
(In reply to Jan Varga [:janv] from comment #4)
> File* file = file.mBlob->ToFile();

that should be:
File* file = file.mBlob->ToFile().take();
Attachment #8999811 - Flags: review?(jvarga) → review+
See Also: → 1480640
Priority: -- → P1
Pushed by sfink@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/c2a1d27fb7a9
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.