Closed Bug 1370349 Opened 7 years ago Closed 7 years ago

Allow relinquishing access to zone groups when yielding

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: billm, Assigned: bhackett1024)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch_relinquish.txt (obsolete) — Splinter Review
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
Attachment #8874574 - Flags: review?(jdemooij)
Attachment #8874574 - Flags: review?(jdemooij) → review+
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.
Flags: needinfo?(bhackett1024)
Attached patch patch v2 (obsolete) — Splinter Review
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
Attachment #8874574 - Attachment is obsolete: true
Attachment #8876329 - Flags: review?(jdemooij)
Comment on attachment 8876329 [details] [diff] [review]
patch v2

Never mind, there is another patch I'm supposed to apply and I forgot.
Attachment #8876329 - Flags: review?(jdemooij)
Flags: needinfo?(bhackett1024) → needinfo?(wmccloskey)
Attached patch patch v3Splinter Review
Here's the patch with the assertions fixed.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8460729bbc572b97126c64fa7f51666af0db575e&selectedJob=107498151
Attachment #8876329 - Attachment is obsolete: true
Flags: needinfo?(wmccloskey)
Attachment #8878293 - Flags: review?(jdemooij)
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|
Attachment #8878293 - Flags: review?(jdemooij) → review+
Flags: needinfo?(wmccloskey)
Brian, it seems like you're back now. Can you comment on the fun.caller thing?
Flags: needinfo?(wmccloskey) → needinfo?(bhackett1024)
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab36bb2ce844
Allow relinquishing access to zone groups when yielding (r=jandem)
https://hg.mozilla.org/mozilla-central/rev/ab36bb2ce844
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(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.
Flags: needinfo?(bhackett1024)
You need to log in before you can comment on or make changes to this bug.