Jan de Mooij [:jandem] Assignee
	Description • 6 years ago

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...

	Jan de Mooij [:jandem] 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.

	Jan de Mooij [:jandem] 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.

	Boris Zbarsky [:bzbarsky]
	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?

	Boris Zbarsky [:bzbarsky]
	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!

	Jan de Mooij [:jandem] 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).

	Jan de Mooij [:jandem] 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.

	Jan de Mooij [:jandem] 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.

	Pulsebot
	Comment 8 • 6 years ago

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

	Bogdan Tara[:bogdan_tara | bogdant]
	Comment 9 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/28b00e409033
https://hg.mozilla.org/mozilla-central/rev/e8d648b668d1