GC: Some minor rooting fixes

RESOLVED FIXED in mozilla21

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

Trunk
mozilla21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

A few fixes for rooting hazards found by the static analysis.
Posted patch Proposed fix (obsolete) — Splinter Review
Attachment #713896 - Flags: review?(sphink)
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?
Attachment #713896 - Flags: review?(sphink)
Posted patch Updated fixSplinter Review
Updated following comments.  I handlified JSObject::swap arguments as well.
Attachment #713896 - Attachment is obsolete: true
Attachment #714462 - Flags: review?(sphink)
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.
Attachment #714462 - Flags: review?(sphink) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/ff752cb0c0cb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.