1360372 - Acquire a cooperative thread lock when entering the system zone

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

This patch adds a callback where we can ask Gecko to yield to other cooperative threads. We call the yield callback when entering the system zone group when another context already owns the system zone group.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8862643 [details] [diff] [review]
patch

Brian gave an r+ by email since his internet connectivity is limited.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd049b7bf3d2
Acquire cooperative lock when entering system zone group (r=bhackett)

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for hazards:

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

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=fd049b7bf3d233fa51227992f1cbd8bfcdad1e98&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix rooting hazard

This patch avoids a rooting hazard involving the atoms compartment. There is
code in jsatom.cpp that enters the atoms compartment while unrooted pointers
are on the stack. Now that enterZoneGroup can potentially yield, this leads to
a rooting hazard. This patch avoids the hazard by using a totally separate path
that avoids enterZoneGroup when entering the atoms compartment.

Comment 5

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

Comment on attachment 8863301 [details] [diff] [review]
fix rooting hazard

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

::: js/src/jscntxt.h
@@ +206,5 @@
> +    // Special enterCompartment for atoms compartment. It requires
> +    // locking. Also, we avoid calling enterZoneGroup since that can induce GC
> +    // hazards.
> +    inline void enterAtomsCompartment(JSCompartment* c,
> +                               const js::AutoLockForExclusiveAccess* maybeLock);

Can't this be AutoLockForExclusiveAccess& ?

::: js/src/jscntxtinlines.h
@@ +455,4 @@
>  {
>      enterCompartmentDepth_++;
>  
>      if (!c->zone()->isAtomsZone())

Can this MOZ_ASSERT(!c->zone()->isAtomsZone()), or is there another way of getting here with an atoms compartment? It seems like you don't want to allow that, since the caller might not have the lock in that case.

@@ +464,5 @@
> +
> +inline void
> +JSContext::enterAtomsCompartment(
> +    JSCompartment* c,
> +    const js::AutoLockForExclusiveAccess* maybeLock)

You're in JS-land, I think that'll all fit in 99 chars.

::: js/src/jscompartment.h
@@ +1059,2 @@
>      inline AutoCompartment(JSContext* cx, JSCompartment* target,
> +                           AutoLockForExclusiveAccess* maybeLock);

If this is only used for the atoms compartment now, can you make this AutoLockForExclusiveAccess& lock?

::: js/src/jscompartmentinlines.h
@@ +43,5 @@
>  {
>      cx_->enterCompartmentOf(target);
>  }
>  
>  // Protected constructor that bypasses assertions in enterCompartmentOf.

Also mention here that it's only for the atoms compartment. (If there were no comment here at all, it'd be fine to just not say anything since the header has it, but given that there is, it seems weird to omit that detail.)

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix rooting hazard, v2

Looks like that was the only way we entered the atoms compartment (to the extent that I can start a browser with the assertion in place).

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11a43a7edbae
Acquire cooperative lock when entering system zone group (r=bhackett)
https://hg.mozilla.org/integration/mozilla-inbound/rev/7292d50807c9
Avoid rooting hazard when entering atoms compartment (r=sfink)

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11a43a7edbae
https://hg.mozilla.org/mozilla-central/rev/7292d50807c9