Closed Bug 1337112 Opened 3 years ago Closed 3 years ago

Remove links from JSRuntime to its single context and zone group

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bhackett, Assigned: bhackett)

References

Details

Attachments

(2 files)

Attached patch patchSplinter Review
This patch removes the links from the JSRuntime to its singleton context and zone group, and fixes the last remaining places where these fields are used.  This requires changing the API in a couple of places: CompartmentCreationOptions needs to specify which zone group to use, and JS_GetParentContext is changed back to JS_GetParentRuntime (just getting an arbitrary context from the parent runtime doesn't seem great, due to both the potential for races and the potential for the cx to be destroyed before the runtime).

After this patch we can in theory support multiple contexts or zone groups per runtime, though there is not yet any way to do the former, nor any uses of the latter.
Attachment #8834105 - Flags: review?(jdemooij)
Attachment #8834105 - Attachment is patch: true
This patch has some additional changes to propagate nursery size information to newly created zone groups.

With multiple zone groups in a runtime, the nursery size limit passed to JS_NewContext is treated as a limit on the size of each individual nursery, rather than the total size of the nurseries as it seems it should be.  I think the best course forward is to leave things this way for now, since the runtime will only have one zone group with a nursery and it would be nice to have a functioning cooperatively multithreaded runtime in the browser before we look into how to avoid starving nurseries in active use if the runtime limit has been changed.
Attachment #8834560 - Flags: review?(jcoppeard)
Blocks: 1337491
Attachment #8834560 - Flags: review?(jcoppeard) → review+
Comment on attachment 8834105 [details] [diff] [review]
patch

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

::: js/src/jsapi.h
@@ +2201,5 @@
> +    SystemZone,
> +    ExistingZone,
> +    NewZoneInNewZoneGroup,
> +    NewZoneInSystemZoneGroup,
> +    NewZoneInExistingZoneGroup

Each of these could use a brief comment, to make it clear what's the difference between SystemZone and NewZoneInSystemZoneGroup etc.

@@ +2218,5 @@
>    public:
>      CompartmentCreationOptions()
>        : addonId_(nullptr),
>          traceGlobal_(nullptr),
> +        zoneSpec_(NewZoneInSystemZoneGroup),

The previous behavior was to default to JS::FreshZone. Using NewZoneInSystemZoneGroup does not mean the compartment is actually in the system *Zone* right? I would be worried about security issues in that case.

::: js/src/jscntxt.cpp
@@ +175,5 @@
> +    // Cancel all off thread Ion compiles before destroying a cooperative
> +    // context. Completed Ion compiles may try to interrupt arbitrary
> +    // cooperative contexts which they have read off the owner context of a
> +    // zone group. See HelperThread::handleIonWorkload.
> +    CancelOffThreadIonCompile(cx->runtime());

Follow-up bug to add a CancelOffThreadIonCompile overload that takes a JSContext* so we can cancel only these compilations that have zoneGroup->ownerContext == context? Would that work?

::: js/src/jsgc.cpp
@@ +6819,5 @@
> +        }
> +
> +        // Lazily set the runtime's sytem zone.
> +        if (zoneSpec == JS::SystemZone) {
> +            rt->gc.systemZone = zone;

MOZ_ASSERT(!rt->gc.systemZone); before this. Maybe MOZ_RELEASE_ASSERT even.

@@ +6827,5 @@
> +
> +    if (groupHolder) {
> +        if (!rt->gc.groups.ref().append(group)) {
> +            ReportOutOfMemory(cx);
> +            return nullptr;

If we OOM here, rt->gc.systemZone is non-null and the Zone is in the Vector. The Zone destructor will null out rt->gc.systemZone, will it also remove the Zone from the group's Zone Vector?

@@ +6832,5 @@
> +        }
> +
> +        // Lazily set the runtime's system zone group.
> +        if (zoneSpec == JS::SystemZone || zoneSpec == JS::NewZoneInSystemZoneGroup)
> +            rt->gc.systemZoneGroup = group;

Assert !systemZoneGroup here, and same question about the Vector.

::: js/src/shell/js.cpp
@@ +4845,5 @@
>  
>          if (!JS_GetProperty(cx, opts, "sameZoneAs", &v))
>              return false;
>          if (v.isObject())
> +            creationOptions.setExistingZone(UncheckedUnwrap(&v.toObject()));

Follow-up to let us test all Zone spec options here?
Attachment #8834105 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #2)
> @@ +2218,5 @@
> >    public:
> >      CompartmentCreationOptions()
> >        : addonId_(nullptr),
> >          traceGlobal_(nullptr),
> > +        zoneSpec_(NewZoneInSystemZoneGroup),
> 
> The previous behavior was to default to JS::FreshZone. Using
> NewZoneInSystemZoneGroup does not mean the compartment is actually in the
> system *Zone* right? I would be worried about security issues in that case.

Yes, NewZoneInSystemZoneGroup will create a new zone that just happens to be in the same group as the system zone (but shares no other properties with it).  Until the browser is updated to start splitting zones into groups, we'll just have one zone group that contains all zones in the runtime except those created for off thread parses.
(In reply to Jan de Mooij [:jandem] from comment #2)
> ::: js/src/jscntxt.cpp
> @@ +175,5 @@
> > +    // Cancel all off thread Ion compiles before destroying a cooperative
> > +    // context. Completed Ion compiles may try to interrupt arbitrary
> > +    // cooperative contexts which they have read off the owner context of a
> > +    // zone group. See HelperThread::handleIonWorkload.
> > +    CancelOffThreadIonCompile(cx->runtime());
> 
> Follow-up bug to add a CancelOffThreadIonCompile overload that takes a
> JSContext* so we can cancel only these compilations that have
> zoneGroup->ownerContext == context? Would that work?

No, the compilation threads read ownerContext atomically without locking and could get any context which has previously entered the zone group, not just the current owner of the group.

This code is necessary to avoid the race but I doubt it will end up being worth optimizing; I'm assuming that Quantum DOM will have a pool of threads available for running JS and will only very rarely create or destroy JSContexts.

> ::: js/src/jsgc.cpp
> @@ +6819,5 @@
> > +        }
> > +
> > +        // Lazily set the runtime's sytem zone.
> > +        if (zoneSpec == JS::SystemZone) {
> > +            rt->gc.systemZone = zone;
> 
> MOZ_ASSERT(!rt->gc.systemZone); before this. Maybe MOZ_RELEASE_ASSERT even.

I can assert this, but !rt->gc.systemZone should hold here based on the earlier logic in the zoneSpec switch statement and later tests, so we shouldn't have to be paranoid here about what creation options external users are giving us.

> @@ +6827,5 @@
> > +
> > +    if (groupHolder) {
> > +        if (!rt->gc.groups.ref().append(group)) {
> > +            ReportOutOfMemory(cx);
> > +            return nullptr;
> 
> If we OOM here, rt->gc.systemZone is non-null and the Zone is in the Vector.
> The Zone destructor will null out rt->gc.systemZone, will it also remove the
> Zone from the group's Zone Vector?

The group will also be deleted if we return prematurely here.

> ::: js/src/shell/js.cpp
> @@ +4845,5 @@
> >  
> >          if (!JS_GetProperty(cx, opts, "sameZoneAs", &v))
> >              return false;
> >          if (v.isObject())
> > +            creationOptions.setExistingZone(UncheckedUnwrap(&v.toObject()));
> 
> Follow-up to let us test all Zone spec options here?

Hmm, it would be good to test more of the zone creation options here but the problem is that we won't be supporting arbitrary cross-zone-group pointers.  If a thread enters one zone group which another thread is already in, we bust on an assert.  Even trickier is that the exact constraints which Quantum DOM will have aren't clear to me yet, and whatever testing functionality we have in the shell should mirror what the browser can do.  Would it be OK to wait on this until things are better formed?

Also, the next bug in the series will add APIs and shell testing functionality for cooperative contexts.  Each of these contexts has its own zone group and while there won't initially be a way to get cross zone group pointers between the groups the contexts can still interact with each other via things like 'gc()'.
(In reply to Brian Hackett (:bhackett) from comment #4)
> This code is necessary to avoid the race but I doubt it will end up being
> worth optimizing; I'm assuming that Quantum DOM will have a pool of threads
> available for running JS and will only very rarely create or destroy
> JSContexts.

Oh right. I was thinking this would happen when the user closes a tab, but ZoneGroup != JSContext, of course. Nevermind then.

(In reply to Brian Hackett (:bhackett) from comment #4)
> Also, the next bug in the series will add APIs and shell testing
> functionality for cooperative contexts.  Each of these contexts has its own
> zone group and while there won't initially be a way to get cross zone group
> pointers between the groups the contexts can still interact with each other
> via things like 'gc()'.

OK, sounds good to me. I just want to make sure we have shell/fuzz testing for multiple ZoneGroups/JSContexts before Gecko starts using this.
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/920d5dfea5de
Remove links from JSRuntime to its single context and zone group, r=jandem,jonco.
https://hg.mozilla.org/mozilla-central/rev/920d5dfea5de
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.