Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sort out interaction of promises with document unloading and browsing context teardown #2621

Open
bzbarsky opened this issue May 3, 2017 · 12 comments
Labels
needs tests Moving the issue forward requires someone to write tests topic: event loop topic: multiple globals topic: script

Comments

@bzbarsky
Copy link
Contributor

bzbarsky commented May 3, 2017

Consider this testcase: Page A opens a same-origin page B in a new window/tab, page B runs the following code:

(function f() { ++opener.counter; return Promise.resolve().then(f) })()

then the user gets sick of the proceedings closes the window/tab B is in.

What should happen?

Per current spec, the answer is "User can never interact with A again, because that promise loop never terminates". This is in fact what Chrome implements, until it crashes on OOM. Same for Safari. This is obviously somewhat user-hostile; users expect that when they close a page it gets closed.

Firefox seems to show a "would you like to stop this script?" dialog after some time, independently of whether the window/tab is closed or not.

What should the spec require here?

Also, what should happen to attempts to do such promise loops on already-closed or navigated-away-from pages? What should happen on navigation attempts in the middle of a loop like the one above? Can those even happen?

Some relevant reading:

https://bugzilla.mozilla.org/show_bug.cgi?id=1058695
https://bugzilla.mozilla.org/show_bug.cgi?id=1165237
https://bugzilla.mozilla.org/show_bug.cgi?id=1124322

@bzbarsky
Copy link
Contributor Author

bzbarsky commented May 3, 2017

Testcase:

<script>
  var counter = 0;
  var blob = new Blob(["<script>(function f() { ++opener.counter; return Promise.resolve().then(f) })()</sc" + "ript>"], { type: "text/html" });
  var url = URL.createObjectURL(blob);
</script>
<input type="button" onclick="window.open(url)" value="click me">

@annevk
Copy link
Member

annevk commented May 4, 2017

This seems related to #1989.

I also found that you raised this in https://esdiscuss.org/topic/event-loops-in-navigated-away-from-windows which has a solution of sorts at the end (associate JavaScript jobs with a realm), but I'm not sure if @domenic has taken that into account with his revamp of how JavaScript and HTML are "layered".

I also found domenic/promises-unwrapping#108 which is probably the original sin, if anything. (Another case of TC39 dismissing host integration concerns and those then resurfacing for the next couple of years.)

@annevk
Copy link
Member

annevk commented Feb 8, 2019

It seems HTML has had "check if we can run script" since 2016. How does that not address this concern?

@bzbarsky
Copy link
Contributor Author

bzbarsky commented Feb 8, 2019

Hmm. This is the check at https://html.spec.whatwg.org/#enqueuejob(queuename,-job,-arguments) step 7.1?

That might in fact address the concern. We probably need to get some tests written to test the various edge cases here and then poke browsers to fix things as needed.

@annevk
Copy link
Member

annevk commented Feb 8, 2019

Yeah, I'm also somewhat surprised I hadn't seen it before.

@domenic
Copy link
Member

domenic commented Mar 12, 2020

This has come up in Chromium a few times now. The spec side issues are w3c/payment-request#872 and WebAudio/web-audio-api#2176.

Basically, for some time in the past I've been encouraging others to write specs like "If this's relevant settings object's responsible Document is not fully active, then return a rejected promise."

However, this advice seems wrong, since per the "can run script" check in the event loop, the promise's rejection handler will almost never be called. It could be called if the document becomes fully active again, which would happen if the document was in the bfcache and then got navigated back to. IMO this is quite bad as it means whether you get the failure signal or not is nondeterministic and non-interoperable, and the failure signal would be significantly delayed from the original operation that caused it.

Instead I've been recently advising people to return a forever-pending promise, so that handlers will never be called (i.e., this gives us determinism). This is a bit weird; there is neither good spec infrastructure nor, apparently, Chromium implementation infrastructure, to do this. But it makes the most sense to me.

Do others have thoughts? I'm getting some pushback in Chromium about this so I think it'd be good to settle the best practices here one way or another.

@bzbarsky
Copy link
Contributor Author

since per the "can run script" check in the event loop

@domenic, can you point to the check involved? I'm not obviously finding it... In particular, https://html.spec.whatwg.org/#event-loop-processing-model:event-loop always does step 7, which will run the microtasks for the Promise. And in addition to that, the microtask checkpoint at the end of the script execution that called the promise-returning method will run promise microtasks.

My memory is that the rejection handler bits do the "is script enabled" check on the handler, not the promise, but I might be mis-remembering... So if you call a promise-returning method on a thing from a non-fully-active document but pass in a function from a fully-active document as the rejection handler that seems like it should work, unless I am missing something.

@domenic
Copy link
Member

domenic commented Mar 12, 2020

Hmm. I was thinking of https://html.spec.whatwg.org/#enqueuejob(queuename,-job,-arguments) step 8.1, which indeed looks at the realm of the handler. You're totally right then.

In that case I'd be prepared to flip-flop back to recommending rejected promises for cases like that. What do you think? I think the code in your OP is still prevented from infinite-looping by 8.1.

@bzbarsky
Copy link
Contributor Author

It would be, though it would not if the evil f came from the opener page. So there may still be a need for some sort of browser UI for stopping these sorts of loops...

@domenic
Copy link
Member

domenic commented Jun 8, 2020

For the sake of having something to recommend, I'm going to suggest that specifications reject promises with an "InvalidStateError" DOMException, over any other type of exception.

@annevk
Copy link
Member

annevk commented Sep 7, 2020

(async () => {
var i = document.createElement('iframe');
document.body.appendChild(i);
var a = i.contentWindow.eval('(async () => await 1)');
i.remove(); // someone removes the iframe
try {
const v = await a();
console.log(v);
} catch (e) {
console.log(e);
}
})();

from https://bugzilla.mozilla.org/show_bug.cgi?id=1663090 by @arai-a is interesting with Chrome logging 1 and Firefox/Safari waiting.

@domenic
Copy link
Member

domenic commented Apr 23, 2021

Note to future self:

I thought that maybe promise resolution stuff was prevented in inactive documents because the event loop processing model only runs tasks from inactive documents.

This is not strictly true. That only applies to the main task-selection algorithm. The "run a microtask checkpoint" algorithm has no document check. The document of a microtask appears to be never consulted by the spec.

domenic added a commit that referenced this issue Mar 28, 2023
Found while doing a code refactoring in https://chromium-review.googlesource.com/c/chromium/src/+/4369772

#2621 is related, but since we don't have an overall fix yet, let's patch it here
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Moving the issue forward requires someone to write tests topic: event loop topic: multiple globals topic: script
Development

No branches or pull requests

3 participants