Closed
Bug 1515754
Opened 6 years ago
Closed 6 years ago
Problem with promise globals and same-compartment realms
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla66
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(2 files)
I get just a few test failures on try with my patches for bug 1514210. One of them looks like a Promise/async-await situation where we end up in the wrong global. The relevant code is in the browser_sidebar_adopt.js test: await win1.SidebarUI.show("viewBookmarksSidebar"); await BrowserTestUtils.closeWindow(win1); How this goes down: (1) We create a Promise P1 under Promise.then (browser-sidebar.js:394). This Promise is in win1's realm. So far so good. (2) We call PromiseReactionJob => AsyncFunctionPromiseReactionJob => AsyncFunctionAwait (3) Now we are in a different realm (the test Window I think, which makes sense I think). However, under EnqueuePromiseResolveThenableJob we do a GetProperty P1.then. Then (no pun intended) we switch to its realm: // We enter the `then` callable's compartment so that the job function is // created in that compartment. // That guarantees that the embedding ends up with the right entry global. // This is relevant for some html APIs like fetch that derive information // from said global. RootedObject then(cx, CheckedUnwrap(&thenVal.toObject())); AutoRealm ar(cx, then); Now we allocate the PromiseResolveThenableJob function in win1's realm, and we keep doing this for various other functions we end up allocating and calling (PromiseReactionJob, ResolvePromiseFunction). Eventually we end up with some other job in win1 but that window then IsDying() so we don't call it => time out and we don't get to the next await. So where's the bug? I think EnqueuePromiseResolveThenableJob changing realms is where things go off the rails, but I'm not sure what the right thing to do is...
Assignee | ||
Comment 1•6 years ago
|
||
Sorry, Boris, but I think you're the most likely person to have some insight here :/ Tracking this down so far took me a long time, but I still don't know what the code *should* be doing.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 2•6 years ago
|
||
So in a flash of insight I decided to add ar.emplace(cx, ..) calls also to the *non-CCW* branches in Promise.cpp and, guess what, I think that fixes this test failure. So maybe I should start writing tests for each of these AutoRealms and make sure our behavior is the same with same-compartment realms as without.
Comment 3•6 years ago
|
||
So there are two issues: 1) Which globals should our promises be in? Till, do you recall how/why things work like they are written right now? 2) What should happen to promises when "their" windows go away? This is an open spec question; ES pretends like windows never go away so theoretically does not have this problem, but in practice of course windows _do_ go away... This doesn't help us here, of course. If we did the thenable thing without switching globals, we might run the callback, but in general we try to avoid script in already-closed windows, and the IsDying protections are part of that. So we would be subverting those protections. How did this use to work? What's really changing with the compartment changes? One other thing that's not clear to me: I would think the window is not getting closed until the BrowserTestUtils.closeWindow call. But at that point we should have finished with all the win1 promise bits, no?
Comment 4•6 years ago
|
||
> also to the *non-CCW* branches in Promise.cpp
Oh. Yeah. Promise.cpp probably assumes that cross-global means "CCW" or some such, and that's just not true anymore... That explains what's changing, at least!
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 5•6 years ago
|
||
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #3) > One other thing that's not clear to me: I would think the window is not > getting closed until the BrowserTestUtils.closeWindow call. But at that > point we should have finished with all the win1 promise bits, no? We do the closeWindow and that registers some observers, does a Promise.all etc. I think right before we get back to the await in our test we end up resolving a promise in the now-closed window (it should be happening in another global, at least that's what I think is happening today).
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•6 years ago
|
||
This is consistent with what we do for cross-compartment wrappers and avoids problems with running jobs against a dying global (Gecko drops such jobs). I added a globalOfFirstJobInQueue() shell function so I could write a test that checks both the compartment-per-global and single-compartment cases.
Assignee | ||
Comment 7•6 years ago
|
||
As discussed on IRC this is hard to trigger, but this is more consistent with the cross-compartment code.
Pushed by jdemooij@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/28b00e409033 Enter the reaction's realm in EnqueuePromiseReactionJob before creating the job. r=arai https://hg.mozilla.org/integration/autoland/rev/e8d648b668d1 Change Debugger::onPromiseSettled to check promise->realm()->isDebuggee() instead of cx->realm()->isDebuggee(). r=arai
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/28b00e409033 https://hg.mozilla.org/mozilla-central/rev/e8d648b668d1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in
before you can comment on or make changes to this bug.
Description
•