Should we cancel promise resolution on an inactive context with an undefined handler? #4443

tzik · 2019-03-25T05:11:38Z

On an example below, should we see "PASS" on #output?:
https://jsfiddle.net/64eaqtr7/

function onload_event(iframe) {
    return new Promise(resolve => {
        iframe.addEventListener('load', ()=>{resolve();}, {once: true});
    });
}

(async function runTest() {
    const iframe = document.createElement('iframe');
    document.body.appendChild(iframe);

    iframe.srcdoc = '<!doctype html>';
    await onload_event(iframe);
    const promise = iframe.contentWindow.Promise;

    iframe.srcdoc = '';
    await onload_event(iframe);

    promise
        .resolve()
        .then()
        .then(()=>{
            document.querySelector('#output').textContent = 'PASS';
        });
})();

promise is the Promise constructor on the inactive (non fully-active) browsing context, and resolve() returns a resolved promise on it.
When then() is called, we run PerformPromiseThen, and run EnqueueJob for a PromiseReactionJob to perform the reaction, where [[Handler]] of the job is undefined. At the step 7.1 of EnqueueJob, we check if an appropriate browsing context is fully-active, and if it's not fully-active, we cancel the job.

Then, according to a discussion of the appropriate browsing context, we should use [[Handler]]'s context for the check, but it's undefined on the example. If we choose the promise's creation context (the result of GetFunctionRealm of the constructor) as the second most appropriate one, the PromiseReactionJob will be cancelled, and the resulting Promise of then() is not resolved nor rejected.

The text was updated successfully, but these errors were encountered:

tzik · 2019-03-25T05:12:15Z

@domenic @yuki3

Per an offline discussion with @yuki3, we should probably use incumbent realm when the handler's realm is unavailable.

annevk · 2019-03-27T11:33:35Z

cc @bzbarsky

yuki3 · 2019-03-28T10:03:27Z

I'd vaguely think that it would be better for Web APIs (user agents) to return a promise created in the incumbent realm (not this object's realm nor the current realm). Then, we can check the promise's associated realm later instead of a promise handler's associated realm. Maybe this way would be simpler than the current way?

bzbarsky · 2019-03-28T14:02:59Z

I'd vaguely think that it would be better for Web APIs (user agents) to return a promise created in the incumbent realm

Please talk to @domenic, who has been trying to kill off the concept of incumbent realm. ;)

But also, that doesn't really work for promises that are conceptually created as part of the object (e.g. the ready promise of FontFaceSet), right?

For the original question, I would expect that Gecko just uses the Realm of the promise itself for the checks (though I have not verified this). And if all ES objects plan to grow a Realm, maybe we should just converge on that?

yuki3 · 2019-03-29T11:51:20Z

Please talk to @domenic, who has been trying to kill off the concept of incumbent realm. ;)

Yeah, I knew @domenic's effort and it's not fun for me, too, to use the incumbent realm.

But also, that doesn't really work for promises that are conceptually created as part of the object (e.g. the ready promise of FontFaceSet), right?

You're right. FontFaceSet.ready() doesn't work anyway.

For the original question, I would expect that Gecko just uses the Realm of the promise itself for the checks (though I have not verified this). And if all ES objects plan to grow a Realm, maybe we should just converge on that?

I love that idea, but some of Web APIs seem not happy with that idea. @tzik, would you show a concrete example that a certain Web API (fullscreen API?) wants their promise to get resolved even after the browsing context navigated away?

danyao · 2019-03-29T14:55:36Z

@tzik, would you show a concrete example that a certain Web API (fullscreen API?) wants their promise to get resolved even after the browsing context navigated away?

One example is from Payment Request API: https://w3c.github.io/payment-request/#show-method

4. If document is not fully active, then return a promise rejected with an "AbortError" DOMException.

crbug/941271 has a concrete example.

domenic · 2019-03-29T16:28:21Z

(This issue reminded me of #2621 ; I can't quite tell how related they are, though.)

Let me try to understand the issue a bit better. From what I can see, one issue is that when you do promise.then(undefined), i.e. promise.then(), there is no [[Handler]], so our previous resolution on how to fix step 2 does not work there. Right? The proposed fix is then to use the Promise's realm.

In the particular example, this then means we will make the job a no-op, because the iframe is inactive in that example. This is similar to if you do, for example:

// (1)
fulfilledPromiseFromInactiveFrame.then(functionFromInactiveFrame);

I don't know if we should try to "fix" this by using the incumbent realm. It seems that the goal is to not run the event loop for inactive frames, and we shouldn't poke holes in that. So, I think (1) should not run, probably?

However, let's consider some more cases:

// (2)
fulfilledPromiseFromInactiveIframe.then(functionFromActiveFrame);

Should functionFromActiveFrame run? Our previous resolution says yes. Are we sticking with that?

Then we come to a case that is basically the OP case:

// (3)
fulfilledPromiseFromInactiveFrame.then().then(functionFromActiveFrame);

I think (3) should behave the same as (2). So if (2) runs then we need to make (3) run.

I am not sure where this leaves us. I will try to think harder, but concrete proposals and more thoughts are also welcome.

Maybe one thing we should do is implement the resolution from #1426 (comment) ASAP, with a TODO remaining for undefined [[Handler]] that links to this thread. That will make the discussion a little more concrete, as right now we are all kind of assuming that resolution, but it is not specced very clearly anywhere.

tzik · 2019-04-01T08:13:32Z

@tzik, would you show a concrete example that a certain Web API (fullscreen API?) wants their promise to get resolved even after the browsing context navigated away?

One example is from Payment Request API: https://w3c.github.io/payment-request/#show-method
4. If document is not fully active, then return a promise rejected with an "AbortError" DOMException.
crbug/941271 has a concrete example.

Right, Payment Request API is an example of such an Web API.
requestFullscreen is another example that returns a rejected promise on an inactive document.

4. If pendingDoc is not fully active, then reject promise with a TypeError exception and return promise.

tzik · 2019-04-01T08:41:15Z

(This issue reminded me of #2621 ; I can't quite tell how related they are, though.)

Let me try to understand the issue a bit better. From what I can see, one issue is that when you do promise.then(undefined), i.e. promise.then(), there is no [[Handler]], so our previous resolution on how to fix step 2 does not work there. Right? The proposed fix is then to use the Promise's realm.

Right, the issue is we don't have the [[Handler]] that we can pull a browsing context from.
But, the proposed fix is to use the incumbent context of then rather than Promise, that pulls a browsing context from the caller of then.

In the particular example, this then means we will make the job a no-op, because the iframe is inactive in that example. This is similar to if you do, for example:
// (1)
fulfilledPromiseFromInactiveFrame.then(functionFromInactiveFrame);
I don't know if we should try to "fix" this by using the incumbent realm. It seems that the goal is to not run the event loop for inactive frames, and we shouldn't poke holes in that. So, I think (1) should not run, probably?

However, let's consider some more cases:
// (2)
fulfilledPromiseFromInactiveIframe.then(functionFromActiveFrame);
Should functionFromActiveFrame run? Our previous resolution says yes. Are we sticking with that?

Then we come to a case that is basically the OP case:
// (3)
fulfilledPromiseFromInactiveFrame.then().then(functionFromActiveFrame);
I think (3) should behave the same as (2). So if (2) runs then we need to make (3) run.

I am not sure where this leaves us. I will try to think harder, but concrete proposals and more thoughts are also welcome.

Maybe one thing we should do is implement the resolution from #1426 (comment) ASAP, with a TODO remaining for undefined [[Handler]] that links to this thread. That will make the discussion a little more concrete, as right now we are all kind of assuming that resolution, but it is not specced very clearly anywhere.

The latest Chrome implementation uses the handler's browsing context. So, it doesn't run (1)'s functionFromInactiveFrame, while it runs (2)'s functionFromActiveFrame.
However, it doesn't run (3)'s functionFromActiveFrame. For then() of (3), Chrome uses the context of fulfilledPromiseFromInactiveFrame as the fallback, and the resulting promise of then() will not be resolved or rejected, as it's to-be-resolved by the cancelled
PromiseReactionJob.

bzbarsky · 2019-04-01T13:10:55Z

requestFullscreen is another example that returns a rejected promise on an inactive document.

So that is an interesting case. This testcase:

<iframe srcdoc="<div>"></iframe>
<script>
  var div;
  onload = function() {
    var ifr = document.querySelector("iframe");
    div = ifr.contentDocument.querySelector("div");
    doTest("While active: ")
    ifr.onload = doTest.bind(null, "While inactive: ");
    ifr.srcdoc = "<span>";
  };

  function doTest(context) {
    div.requestFullscreen().catch().then(() => console.log(context + 'ok'),
                                         () => console.log(context + 'error'));
  }
</script>

does not log the "inactive" case in Firefox or Chrome. Safari doesn't seem to support unprefixed fullscreen, and webkitRequestFullscreen does not return a promise. In both Firefox and Chrome if I pass () => {} to catch() something useful is logged.

That's consistent with what I understand of Gecko's implementation and what @tzik describes for Chrome's implementation in #4443 (comment) and the two seem to match each other...

Now maybe the reasons for the execution prevention simply don't apply to the "then() with no argument" case, and we should simply whitelist those to not do the "should this execute?" check at all, in which case we don't need to worry about which global is used for the (now non-existent) check? The intent of the execution prevention is to keep script from running unnecessarily in inactive documents, and then() with no argument case doesn't obviously have problems in that regard, right?

yuki3 · 2019-04-04T10:29:54Z

Boris's proposal makes much sense to me, although I'm not an expert in this area. I support the idea to not check the condition at all in case of then().

tzik · 2019-04-04T13:08:20Z

Hmm, I'm reluctant to go that way, to be honest.
To implement that behavior, we have to touch the microtask queue on an inactive realm, and that will alter a lifetime assumption of the V8 impl.

annevk added the topic: event loop label Mar 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Should we cancel promise resolution on an inactive context with an undefined handler? #4443

Should we cancel promise resolution on an inactive context with an undefined handler? #4443

tzik commented Mar 25, 2019

tzik commented Mar 25, 2019 •

edited

Loading

annevk commented Mar 27, 2019

yuki3 commented Mar 28, 2019

bzbarsky commented Mar 28, 2019 •

edited

Loading

yuki3 commented Mar 29, 2019

danyao commented Mar 29, 2019

domenic commented Mar 29, 2019

tzik commented Apr 1, 2019

tzik commented Apr 1, 2019

bzbarsky commented Apr 1, 2019

yuki3 commented Apr 4, 2019

tzik commented Apr 4, 2019

Should we cancel promise resolution on an inactive context with an undefined handler? #4443

Should we cancel promise resolution on an inactive context with an undefined handler? #4443

Comments

tzik commented Mar 25, 2019

tzik commented Mar 25, 2019 • edited Loading

annevk commented Mar 27, 2019

yuki3 commented Mar 28, 2019

bzbarsky commented Mar 28, 2019 • edited Loading

yuki3 commented Mar 29, 2019

danyao commented Mar 29, 2019

domenic commented Mar 29, 2019

tzik commented Apr 1, 2019

tzik commented Apr 1, 2019

bzbarsky commented Apr 1, 2019

yuki3 commented Apr 4, 2019

tzik commented Apr 4, 2019

tzik commented Mar 25, 2019 •

edited

Loading

bzbarsky commented Mar 28, 2019 •

edited

Loading