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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

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

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.

Comment 1

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

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

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.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
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

[Stefan Hindli [:stefan_hindli]]

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

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

: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!

Comment 5

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

(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.

Comment 6

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

Done.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Thanks so much, that's a fantastic addition!

Comment 8

[Byron Campen [:bwc]]

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