Fix rooting hazard between return value and destructor in structured clone test function

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P2
normal
RESOLVED FIXED
8 months ago
3 months ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

8 months ago
In bug 1487463, it looks like there must have been a hazard failure at some point from returning a value and having ~RefPtr fire and possibly GC. Apparently, adding the RootedObject made the problem go away -- but it shouldn't have; the return unwraps the Rooted and places a bare JSObject* on the stack, then runs ~RefPtr (and then runs ~Rooted<JSObject*>, which doesn't matter here because we've already unwrapped it.)

It seems like some analysis bug prevented the hazard from being detected. I am attempting to push a bunch of analysis patches, and it is now finding the bug.
Assignee

Comment 1

8 months ago
Create a scope so the 'result' return value can hide inside the Rooted while ~RefPtr runs. Also, Gecko style is to use Rooted<JSObject*> instead of RootedObject, so change that too.
Attachment #9017285 - Flags: review?(perry)
Priority: -- → P2

Updated

8 months ago
Attachment #9017285 - Flags: review?(perry) → review+

Comment 2

8 months ago
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc4d8d0bf575
Fix rooting hazard between return value and destructor in structured clone test function, r=perry

Comment 3

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bc4d8d0bf575
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
:sfink, would it be possible to add some explanatory test around this hazard to https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/GC_Rooting_Guide#Return_values or could you point me at a better source of docs that already covers this?  This is a tricky area and I'd love to have great docs to reference for myself and to point others at, but I'm never quite sure where is best.  Thanks!
Flags: needinfo?(sphink)
Assignee

Comment 5

8 months ago
(In reply to Andrew Sutherland [:asuth] from comment #4)
> :sfink, would it be possible to add some explanatory test around this hazard
> to
> https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/
> GC_Rooting_Guide#Return_values or could you point me at a better source of
> docs that already covers this?  This is a tricky area and I'd love to have
> great docs to reference for myself and to point others at, but I'm never
> quite sure where is best.  Thanks!

That is an excellent idea. I think I've written up a description of this problem a couple of times before, but I have no idea where, and the spot you pointed to seems like a good place. I'll write something up ASAP.
Assignee

Comment 6

8 months ago
Done.
Flags: needinfo?(sphink)
Thanks so much, that's a fantastic addition!
https://hg.mozilla.org/projects/larch/rev/bc4d8d0bf57555a39cf825c91c82fea0843072bc
Bug 1499177 - Fix rooting hazard between return value and destructor in structured clone test function, r=perry
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.