841356 - GC: Some minor rooting fixes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

A few fixes for rooting hazards found by the static analysis.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Proposed fix

Comment 2

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

Comment on attachment 713896 [details] [diff] [review]
Proposed fix

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

::: js/src/jscntxt.cpp
@@ +283,5 @@
>      if (!table.initialized() && !table.init())
>          return NULL;
>  
>      Key key;
> +    SkipRoot skipKey(cx, &key); /* Prevent the key from being poisoned. */

I'd change the comment. It's actually fine for the key to get poisoned; it's the same effect as it getting moved. This SkipRoot is now to make the analysis happy -- we have unrooted values live across a GC call, but are handling them specially in a way it doesn't understand. I suppose if we do this often enough, we should add some sort of marker that the analysis could understand.

::: js/src/jsobj.cpp
@@ +2024,5 @@
>      JS_ASSERT(IsBackgroundFinalized(getAllocKind()) ==
>                IsBackgroundFinalized(other->getAllocKind()));
>      JS_ASSERT(compartment() == other->compartment());
>  
> +    unsigned r = NotifyGCPreSwap(self, other);

Hm. This method is way too dangerous to be self-rooted. Can you make it static instead?

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Updated fix

Updated following comments.  I handlified JSObject::swap arguments as well.

Comment 4

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

Comment on attachment 714462 [details] [diff] [review]
Updated fix

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

Thanks, that's much nicer.

::: js/src/jscntxt.cpp
@@ +283,5 @@
>      if (!table.initialized() && !table.init())
>          return NULL;
>  
>      Key key;
> +    SkipRoot skipKey(cx, &key); /* Stop the analysis complaining about unrooted key. */

Neither you nor bhackett is on IRC at the moment, so I'll ask: does this work? If you're going to SkipRoot &key, then does that cover both &key.script and &key.original, or do you need to SkipRoot both individually? Alternatively, does this SkipRoot do anything at all? I don't see anything in the static analysis that matches /skip/i; is it done indirectly, or does it just not take SkipRoot into account?

For the dynamic analysis, I think you'd need to SkipRoot both &key.script and &key.original, except that in this case it doesn't really matter whether you skip them or not. (If you don't skip them, they'll get poisoned and re-looked up; if you do skip them, the old lookup will be reused.)

I'm fine with you landing this any which way, since if nothing else we can just see what the static analysis says, but at some point I want to learn the answer.

Comment 5

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] from comment #4)

> ::: js/src/jscntxt.cpp
> > +    SkipRoot skipKey(cx, &key); /* Stop the analysis complaining about unrooted key. */
> 
> Neither you nor bhackett is on IRC at the moment, so I'll ask: does this
> work? 

It looks like the answer is no, so I'll take this out of the patch.

Comment 6

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff752cb0c0cb

Comment 7

[Gregory Szorc [:gps]]

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