Closed Bug 797695 Opened 7 years ago Closed 7 years ago

GC: more ad hoc exact rooting

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: njn, Assigned: njn)

Details

(Whiteboard: [js:t])

Attachments

(4 files)

Patches coming shortly.
This patch exactly roots most of jsexn.cpp.
Attachment #667833 - Flags: review?(terrence)
This patch exactly roots most of jsexn.cpp.
Attachment #667836 - Flags: review?(terrence)
This patch exactly roots js_NewFunction().
Attachment #667867 - Flags: review?(terrence)
This patch exactly roots most of jsscript.cpp.

I inlined the single call to GetGSNCache() because it made the non-GCing use
of the |cx| more obvious.
Attachment #667870 - Flags: review?(terrence)
Comment on attachment 667833 [details] [diff] [review]
(part 1) - Exactly root jsexn.cpp.

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

Nice!
Attachment #667833 - Flags: review?(terrence) → review+
Comment on attachment 667836 [details] [diff] [review]
(part 2) - Exactly root jsfun.cpp.

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

Nice again.  It's going to land too late for this series, but once bug 793577 we'll need to start adding AutoAssertNoGC anywhere we want to access script as a return without rooting it first.

::: js/src/jsfun.cpp
@@ +133,5 @@
> +        {
> +            RawScript script = iter.script();
> +            if (!script->hasIonScript())
> +                ion::ForbidCompilation(cx, script);
> +        }

I think |script|'s lifetime does not escape a NoGC region here.  Why the extra {}?
Attachment #667836 - Flags: review?(terrence) → review+
Comment on attachment 667867 [details] [diff] [review]
(part 3) - Exactly root js_NewFunction().

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

Nice!
Attachment #667867 - Flags: review?(terrence) → review+
Comment on attachment 667870 [details] [diff] [review]
(part 4) - Exactly root jsscript.cpp.

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

Looks good!
Attachment #667870 - Flags: review?(terrence) → review+
> > +        {
> > +            RawScript script = iter.script();
> > +            if (!script->hasIonScript())
> > +                ion::ForbidCompilation(cx, script);
> > +        }
> 
> I think |script|'s lifetime does not escape a NoGC region here.  Why the
> extra {}?

Just paranoia :)  I'll take it out.
You need to log in before you can comment on or make changes to this bug.