1370349 - Allow relinquishing access to zone groups when yielding

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

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

Attachment: patch_relinquish.txt

The attached patch adds a JS::AutoRelinquishZoneGroups class
in the API that relinquishes control of all zone groups which the
current thread owns, so that it can run a nested event loop and other
threads can do whatever they want.  The patch also adds shell
functionality to exercise this and to allow a zone group to be entered
by multiple threads.

(Brian wrote this patch but he can't access Bugzilla right now.)

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed6b392e9e94a67468145fcee87dc9ef7da101ed

Comment 1

[Jan de Mooij [:jandem]]

Note that the Try push shows non-unified-build failures, ZoneGroup.cpp is probably missing some #includes.

Btw, Brian, what about ScriptFrameIter? There are some places where we use that where we want to see all frames - JSScript::argumentsOptimizationFailed for instance.

Comment 2

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

Attachment: patch v2

Do you mind taking a second look at this Jan? I want to make sure I didn't mangle any of Brian's changes.

Here's a new try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c840fab2fbf83320c3246abee41815da1cf29d81

Comment 3

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

Comment on attachment 8876329 [details] [diff] [review]
patch v2

Never mind, there is another patch I'm supposed to apply and I forgot.

Comment 4

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

Attachment: patch v3

Here's the patch with the assertions fixed.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8460729bbc572b97126c64fa7f51666af0db575e&selectedJob=107498151

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8878293 [details] [diff] [review]
patch v3

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

Sorry for the delay.

It's not clear to me why this doesn't break things like fun.caller. Bill, if you agree, can you file a follow-up bug to investigate this and have it block the relevant bugs?

Also if we need to use this in more places, we should add a new iterator (something like the existing AllScriptFramesIter) that looks at all contexts so we don't need multiple loops just to iterate over all frames.

::: js/src/jsapi.h
@@ +1036,5 @@
> +// thread. This allows other cooperative threads to enter the zone groups
> +// and modify their contents.
> +struct AutoRelinquishZoneGroups
> +{
> +    AutoRelinquishZoneGroups(JSContext* cx);

This should be |explicit|

Comment 6

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

Brian, it seems like you're back now. Can you comment on the fun.caller thing?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab36bb2ce844
Allow relinquishing access to zone groups when yielding (r=jandem)

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Brian Hackett [Laid off!]]

(In reply to Bill McCloskey (:billm) from comment #6)
> Brian, it seems like you're back now. Can you comment on the fun.caller
> thing?

Well, my understanding is that the same zone group will only have frames on different contexts' stacks when the older frames are on a context(s) that is running a nested event loop.  I don't know what the semantics are wrt fun.caller being able to see through these nested event loops, and don't know how to test them in the browser --- if I setTimeout() a function and then run a nested event loop (I guess?) using alert() or by triggering the slow script dialog, the timeout doesn't run until after the event loop finishes.