1674505 - async functions from DOM window contexts using `await` stall forever after the window is closed

Status: NEW

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[:Gijs (he/him)]

How you run into this in practice:

1. Call into Sqlite.jsm and/or PlacesUtils.withConnectionWrapper from a window. Pass it some async function declared in that window that awaits on e.g. db.execute() or similar calls (which are async and implemented in Sqlite.jsm).
2. close said window before or during the main call from (1)

ER:
the call completes and sqlite code waiting for the wrapper to become unused unblocks

AR:
the call never completes and sqlite trying to be vigilant about it completing queries before shutdown causes AsyncShutdown crashes

Minimal STR to see the problem:

1. add something like this into browser.js

async function deleteme1(promise) {
  let pfx = "deleteme1 " + document.title + " -- ";
  dump(pfx + "waiting for promise\n");
  await promise;
  dump(pfx + "waited for promise\n");
  return 42;
}

2. run a test that invokes this function on a closed window, passing a resolved promise, e.g.:

  let win = await BrowserTestUtils.openNewBrowserWindow();
  await BrowserTestUtils.closeWindow(win);
  info("Starting");
  await win.deleteme1(Promise.resolve());
  info("finishing.");

(I'll upload a phab patch with a test that does this after filing this bug.)

ER:
because the argument is a resolved promise, I'd expect that the promise returned by the deleteme1 call also resolves, the finishing and waited for promise lines get logged.

AR:
the promise never resolves, those lines don't get logged.

This doesn't sound so bad in theory. After all, why invoke stuff on closed windows, right?

The problem is that most async functions in our windows assume that either they get invoked, or they run to completion - we don't obsessively check whether the window is now closed after every await (and in fact, even if we did, AFAICT we might still get "stuck").

Kris pointed out to me that finally handlers do get run. But you cannot use await promise.finally(() => {}) because the implied promise resolution handler for the await still won't run. There is also no exception (as there would be if the promise passed to await rejected).

The only way to make this "foolproof" is to stop using await and async functions, and instead use "manual" finally .then calls and thus chain the resulting promises directly, such that the promise returned by the function invocation still ends up being resolved (instead of remaining indeterminate forever) and so you can attach resolution handlers that do get invoked from the jsm or other longer-lived context.

The longer-lived consumer/helper (in the practical case, Sqlite.jsm) also struggles to detect this: it could periodically check if the global associated with a passed-in promise or callback function has gone away (using Cu.getGlobalForObject or such), but that would require a lot of vigilance on the part of the consumer/helper, and would require adding complexity into every helper that takes async callbacks.

I'm not quite sure where this lives, implementation wise, how deliberate it is, and what we can do about it to make it less of a footgun in frontend code. Jan or Andrew, do you know the answer to any of those questions? :-)

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1674505 - simplified testcase

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

(do I remember totally wrong that this is a dup of some bug where the conclusion was that we're following the spec.)

Comment 3

[Tooru Fujisawa [:arai]]

sounds related to bug 1663090, that's the case with iframe gets detached, but the underlying situation should be same as here.
if a window is closed, the global should be considered dying, and in that case queued promise reaction jobs won't be triggered.

the spec issue is https://github.com/whatwg/html/issues/2621

The only way to make this "foolproof" is to stop using await and async functions, and instead use "manual" finally .then calls and thus chain the resulting promises directly,

Can you elaborate this part?
I'm not sure why this differs from async function's case. if you use .then, it still uses promise reaction job, and gets affected by whether the global is dying.
so, if the different behavior happens, the reason may be in somewhere different.

I'll investigate the testcase maybe next week, to look into our implementation.

Comment 4

[Jan de Mooij [:jandem]]

Clearing needinfo. arai knows more about this and already commented.

Comment 5

[:Gijs (he/him)]

(In reply to Tooru Fujisawa [:arai] from comment #3)

sounds related to bug 1663090, that's the case with iframe gets detached, but the underlying situation should be same as here.
if a window is closed, the global should be considered dying, and in that case queued promise reaction jobs won't be triggered.

OK. As outlined, this is unfortunate when those promises block resolution of promises in non-dying globals. I assume the engine cannot figure this out? It'd need some kind of acyclic dependency graph between promises...

the spec issue is https://github.com/whatwg/html/issues/2621

Thanks for linking this. Karl also linked to https://github.com/whatwg/html/issues/5319

Perhaps this case is less frequent in web dev because situations with multiple windows whose lifetimes are interdependent yet not tightly controlled is not as common...

The only way to make this "foolproof" is to stop using await and async functions, and instead use "manual" finally .then calls and thus chain the resulting promises directly,

Can you elaborate this part?
I'm not sure why this differs from async function's case. if you use .then, it still uses promise reaction job, and gets affected by whether the global is dying.
so, if the different behavior happens, the reason may be in somewhere different.

Doing this on top of the attached patch:

    diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
    --- a/browser/base/content/browser.js
    +++ b/browser/base/content/browser.js
    @@ -9627,14 +9627,15 @@ if (AppConstants.NIGHTLY_BUILD) {
     
           newFissionWindow.hidden = gFissionBrowser;
           newNonFissionWindow.hidden = !gFissionBrowser;
         },
       };
     }
     
     async function deleteme1(arg) {
       let pfx = "deleteme1 " + document.title + " -- ";
       dump(pfx + "calling callback\n");
    -  await arg;
    -  dump(pfx + "awaited callback\n");
    -  return 42;
    +  return arg.finally(() => { 
    +    dump(pfx + "got finally.\n");
    +    return 42; /* wont impact return value */ 
    +  });
     }

runs the finally dump, from the closed window, though strangely still hangs the test (whereas I would have expected that the finally call returns a promise with the same resolution state as arg, which was already resolved, and the resolution handler from the test to still be invoked - but it doesn't appear to be).

I'll investigate the testcase maybe next week, to look into our implementation.

Thank you!

Mostly I'm interested in how we can write module code that is resilient to this kind of thing happening. I understand we could in theory count on just not writing consumers in windows, but that feels like it's not easy to enforce and too easy to footgun yourself with in future.

Comment 6

[Tooru Fujisawa [:arai]]

Thanks!

I see different behavior between finally and then/await.
the reasoning is here:

1. When calling a function, it enters the function's realm [1][2]
2. if finally's argument is callable, finally creates new functions for onFulfilled and onRejected handlers [3][4] and passes it to promise.then
   and the function is created in the current realm (that is, finally's realm)
   (I think this is an spec bug. I'll file it)
3. when enqueuing a job, it creates a new function for a job, and it's associated with handler's realm [5][6][7]
4. when running a job, it gets the job's global and checks if it's dying [8][9]

If you use .then(() => ...) or await in the dying global, the (passed or internally created) handler is in the realm for the dying global, and job's function is also in the dying global.
and when running the job, it checks the global and it's dying, and it doesn't run the job.

If you use .finally(() => ...), it enters finally's realm, that's testcase's realm because arg is Promise.resolve() passed from the testcase,
and it creates handlers in the realm, and job's function is also in the testcase's realm/global.
and when running the job, it checks the global and it's not dying, and the callback is executed.

Then, async function awaits on the return-statement operand implicitly [10], and that uses the dying global for the promise reacion job.
so, finally callback runs but async function's promise doesn't resolve.

If you remove async from deleteme1, it returns the promise directly and the testcase finishes.

[1] https://tc39.es/ecma262/#sec-prepareforordinarycall
[2] https://searchfox.org/mozilla-central/rev/e75e8e5b980ef18f4596a783fbc8a36621de7d1e/js/src/vm/Interpreter.cpp#3361
[3] https://tc39.es/ecma262/#sec-promise.prototype.finally
[4] https://searchfox.org/mozilla-central/rev/e75e8e5b980ef18f4596a783fbc8a36621de7d1e/js/src/builtin/Promise.js#29-66
[5] https://tc39.es/ecma262/#sec-newpromisereactionjob
[6] https://searchfox.org/mozilla-central/rev/e75e8e5b980ef18f4596a783fbc8a36621de7d1e/js/src/builtin/Promise.cpp#1189
[7] https://searchfox.org/mozilla-central/rev/e75e8e5b980ef18f4596a783fbc8a36621de7d1e/js/src/builtin/Promise.cpp#1199-1201
[8] https://html.spec.whatwg.org/#hostenqueuepromisejob
[9] https://searchfox.org/mozilla-central/rev/e75e8e5b980ef18f4596a783fbc8a36621de7d1e/xpcom/base/CycleCollectedJSContext.cpp#200-201
[10] https://tc39.es/ecma262/#sec-return-statement-runtime-semantics-evaluation

Comment 7

[Tooru Fujisawa [:arai]]

(In reply to :Gijs (he/him) from comment #5)

OK. As outlined, this is unfortunate when those promises block resolution of promises in non-dying globals. I assume the engine cannot figure this out?

It could, but it depends on HTML spec how to handle this case.

runs the finally dump, from the closed window

As explained in the above comment, promise.finally's promise handler and reaction job is created in the promise's global, and it's not dying in the testcase.

though strangely still hangs the test.

As explained in the above comment, async function awaits on the return operand,
and the promise reaction job uses dying global.

Mostly I'm interested in how we can write module code that is resilient to this kind of thing happening. I understand we could in theory count on just not writing consumers in windows, but that feels like it's not easy to enforce and too easy to footgun yourself with in future.

I'm afraid I cannot think of any easy way as long as we follow current spec.
Clean solution would be modifying the spec to run all enqueued job in some way, or throw error when enqueuing a job to dying global.
Hacky solution would be using different algorithm for chrome-priv code maybe?

Comment 8

[:Gijs (he/him)]

(In reply to Tooru Fujisawa [:arai] from comment #7)

Clean solution would be modifying the spec to run all enqueued job in some way, or throw error when enqueuing a job to dying global.

I guess this would mean throwing when calling resolve related to a promise from a dying global? That'd be interesting - but not sure how it'd work for throwing from all the return statement (or end-of-execution) of async functions...

From https://github.com/whatwg/html/issues/2621 it would seem that maybe a newer consensus is to reject promises instead - Could we force a rejection happening in these cases (ie instead of resolving / returning successfully from an async function), and propagating the rejection?

Comment 9

[Tooru Fujisawa [:arai]]

(In reply to :Gijs (he/him) from comment #8)

From https://github.com/whatwg/html/issues/2621 it would seem that maybe a newer consensus is to reject promises instead - Could we force a rejection happening in these cases (ie instead of resolving / returning successfully from an async function), and propagating the rejection?

to propagate rejection, Promise reaction handler should support force-rejection mode (reject promise even if the job is for onFulfilled handler, and do not run the handler itself but propagate the rejection to depending promises) or something like that I think,
and that will require some refactoring both in ECMAScript spec and HTML spec.
not sure how much work it will be tho.

btw, filed spec issue around finally https://github.com/tc39/ecma262/issues/2222 and looks like calling finally callback there is a spec bug,
so please do not rely on the current behavior.

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Can this be closed as a dup of bug 1663090?