Promise.prototype.finally should use onFinally's realm for newly creating functions

[arai-a]

derived from https://bugzilla.mozilla.org/show_bug.cgi?id=1674505#c6

promise.finally(onFinally) and promise.then(onFinally, onFinally) behaves differently in unexpected way if promise and onFinally comes from different realm.

Here's the details.

If Promise.prototype.finally is called with a function, it creates new functions and calls then with them.

https://tc39.es/ecma262/#sec-promise.prototype.finally

26.6.5.3 Promise.prototype.finally ( onFinally )
...
5. If IsCallable(onFinally) is false, then
6. Else,
  a. Let stepsThenFinally be the algorithm steps defined in Then Finally Functions.
  b. Let thenFinally be ! CreateBuiltinFunction(stepsThenFinally, « [[Constructor]], [[OnFinally]] »).
...
7. Return ? Invoke(promise, "then", « thenFinally, catchFinally »).

CreateBuiltinFunction uses current realm because realm parameter isn't passed here.
And the current realm is Promise.prototype.finally function's realm.

https://tc39.es/ecma262/#sec-createbuiltinfunction

9.3.3 CreateBuiltinFunction ( steps, internalSlotsList [ , realm [ , prototype ] ] )
...
2. If realm is not present, set realm to the current Realm Record.
...
6. Set func.[[Realm]] to realm.

At this point, handler function passed to Promise.prototype.then comes from different realm for promise.finally(onFinally) and promise.then(onFinally, onFinally).
For promise.finally(onFinally) case, the handler belongs to finally's function's realm.
For promise.then(onFinally, onFinally) case, the handler is onFinally itself.
And this difference is propagated to embedding as following:

Promise.prototype.then calls PerformPromiseThen

https://tc39.es/ecma262/#sec-promise.prototype.then

26.6.5.4 Promise.prototype.then ( onFulfilled, onRejected )
...
5. Return PerformPromiseThen(promise, onFulfilled, onRejected, resultCapability).

And PerformPromiseThen calls NewPromiseReactionJob with onFulfilledJobCallback, that contains the onFulfilled in [[Callback]] field.

https://tc39.es/ecma262/#sec-performpromisethen

26.6.5.4.1 PerformPromiseThen ( promise, onFulfilled, onRejected [ , resultCapability ] )
...
3. If IsCallable(onFulfilled) is false, then
4. Else,
  a. Let onFulfilledJobCallback be HostMakeJobCallback(onFulfilled).
7. Let fulfillReaction be the PromiseReaction { [[Capability]]: resultCapability, [[Type]]: Fulfill, [[Handler]]: onFulfilledJobCallback }.
10. Else if promise.[[PromiseState]] is fulfilled, then
  a. Let value be promise.[[PromiseResult]].
  b. Let fulfillJob be NewPromiseReactionJob(fulfillReaction, value).
  c. Perform HostEnqueuePromiseJob(fulfillJob.[[Job]], fulfillJob.[[Realm]]).

https://tc39.es/ecma262/#sec-hostmakejobcallback

8.4.2 HostMakeJobCallback ( callback )
...
2. Return the JobCallback Record { [[Callback]]: callback, [[HostDefined]]: empty }.

And NewPromiseReactionJob creates a job with onFulfilled's realm.

https://tc39.es/ecma262/#sec-newpromisereactionjob

26.6.2.1 NewPromiseReactionJob ( reaction, argument )

2. Let handlerRealm be null.
3. If reaction.[[Handler]] is not empty, then
  a. Let getHandlerRealmResult be GetFunctionRealm(reaction.[[Handler]].[[Callback]]).
  b. If getHandlerRealmResult is a normal completion, set handlerRealm to getHandlerRealmResult.[[Value]].
  c. Else, set handlerRealm to the current Realm Record.
  d. NOTE: handlerRealm is never null unless the handler is undefined. When the handler is a revoked Proxy and no ECMAScript code runs, handlerRealm is used to create error objects.
4. Return the Record { [[Job]]: job, [[Realm]]: handlerRealm }.

and the realm is passed to HostEnqueuePromiseJob.
It checks if the realm is "fully active" or not.

https://html.spec.whatwg.org/#hostenqueuepromisejob

8.1.5.1 HostEnqueuePromiseJob(job, realm)
1. If realm is not null, then let job settings be the settings object for realm. Otherwise, let job settings be null.
...
6. Queue a microtask on the surrounding agent's event loop to perform the following steps:
  1. If job settings is not null, then check if we can run script with job settings. If this returns "do not run" then return.

https://html.spec.whatwg.org/#check-if-we-can-run-script

The steps to check if we can run script with an environment settings object settings are as follows. They return either "run" or "do not run".
...
1. If the global object specified by settings is a Window object whose Document object is not fully active, then return "do not run".

So, the difference in realm affects here.
If onFinally's global is dying (= not fully active),
promise.finally(onFinally) runs the job, but promise.then(onFinally, onFinally) doesn't run the job.

IIUC, creating new functions in finally is mostly to avoid passing valueOrReason to onFinally,
and using finally's realm isn't much intentional?
If that's the case, it should use onFinally's realm when creating those functions.

[syg]

Well, this is fascinating. cc @domenic

[ljharb]

You’re correct that the choice of realm isn’t intentional - it makes sense to me to use onFinally’s realm.

[domenic]

Nice find! I agree with @ljharb. The internal functions created are mostly specification artifacts, from my point of view (e.g. they could be replaced with spec closures if we had a coherent way of doing PerformPromiseThen with spec closures). So they should be changed in whatever way is necessary to give reasonable behavior, and matching the realm of onFinally seems great in that regard.

[codehag]

cc @erights To follow up on our meeting yesterday: The bug I mentioned is referenced at the top of this issue thread ( https://bugzilla.mozilla.org/show_bug.cgi?id=1674505). I was mistaken in bringing up incumbent realms as I thought this was browser specific and had tied it in my head to that. It may be browse specific but it looks more general.

Context: we use JS to write many components of the browser UI. We also use JS in testing the browser. The problem arose when we found that tests using await were hanging indefinitely and that we could work around those hanging promises by using onFinally.

We have a realm, where a promise object "promise" was created. The browser is shutting down, so the realm is dying. The bug was that our test cases were relying on running functions as the browser was closing, but promise.then and promise.catch check the dying global and never run. However, a work around for this was to use onFinally. onFinally, unlike then and catch, would resolve -- because its realm was not the promise's realm (the window). It was instead the test's realm -- which was currently running and not dying. Here is what that looks like in practice:

async function deleteme1(promise) { // Promise object is from Realm A. Realm A is dying. function deleteme1 is in Realm B
  console.log("waiting for promise\n");
  await promise;
  console.log( "waited for promise\n"); // never runs, Promise.then checks if Realm A is alive -- if it is not it doesn't get queued. 
  return 42;
}

However, this will work:

async function deleteme1(promise) {
  console.log("waiting for promise\n");
  promise.finally(() => {
    console.log("waited for promise\n"); // runs, because finally is running in Realm B, which is not dying.
  });
  return 42;
}

But this will not:

async function deleteme1(promise) {
  console.log("waiting for promise\n");
  await promise.finally(() => {
    console.log("waited for promise\n"); // fails to run -- await changes the context of finally to run in Realm A
  });
  return 42;
}

While onFinally "works" for getting around the dying global problem, it doesn't seem to be right to run a function associated with a promise from realm A in realm B as this is inconsistent with how promise then/catch methods work. It also feels unintentional as I would expect promise.then to run in its original realm just later. This indicates that we likely want to align the onFinally behavior with then. We definitely want to align the behavior, as the inconsistency between await promise.finally, await promise and promise.finally does not appear to be intentional and will be confusing for users. We are likely to run into web compat issues as others may also have been relying on onFinally running in another realm -- but this is an edge case so hopefully it won't be too bad. I am much more nervous of changing the behavior of promise.then.

[mhofman]

I'm still not sure I fully understand how dying realms tying into this, but I think the main concern we have is that as proposed, finally could expose user code to a realm that was previously not accessible, and which as you mention is apparently not consistent with then either. I also tried to explore the behavior of Promise.prototype.then in tc39/test262#3356 and from what I gather, not all engines behave the same way, so hopefully there is less of a webcompat issue than expected?

[codehag]

The issue i think here is consistent across browser, but it is likely that we have a more general issue across browsers with regards to how realms work in asynchronous contexts. There is a web compat risk, outside of what we are discussing in this issue -- which is that the browsers are not agreeing on the constructor's realm. I am not sure which one is correct in that case, but for this issue we should likely focus on the disparity found between then and onFinally. We can resolve the other issue separately.

[codehag]

For context: the issue with the dying global was how we identified the issue and is also a real world example of where this might be a problem -- in testing code. The problem itself is more general than that.