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.

Sign up for GitHub

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

Jump to bottom

Weird interactions between JS and EnqueueJob make await p different from p.then #5319

Closed
TimothyGu opened this issue Feb 29, 2020 · 4 comments
Closed

Weird interactions between JS and EnqueueJob make await p different from p.then #5319

TimothyGu opened this issue Feb 29, 2020 · 4 comments
Labels
integration Better coordination across standards needed topic: event loop

Comments

@TimothyGu
Copy link
Member

TimothyGu commented Feb 29, 2020 •
edited
Loading

Consider this test case:

<!DOCTYPE html>
<iframe id=iframe></iframe>
<script>
  const p = iframe.contentWindow.Promise.resolve(1);
  iframe.remove();
  p.then(() => {
    console.log("Yay from then!");
  });
  (async () => {
    await p;
    console.log("Yay from await!");
  })();
</script>

One would ordinarily expect that either both Yays are printed, or neither of them, based on the intuition that await and .then are essentially equivalent.

However, this turns out not to be the case after #5212 (and also in Firefox currently). This is because await p would call PromiseResolve(p), which actually treats p as if it's a non-standard-Promise thenable (step 2b fails), so a PromiseResolveThenableJob microtask would be queued up. The HTML EnqueueJob then looks at the p.then method, and decide that the microtask should not be run since iframe's document is already inactive.

On the other hand, the job settings of PromiseReactionJobs is decided based on the then handler, so the callback in p.then(() => {}) would get executed alright.

This behavior is tremendously weird but interesting, since it is at the intersection between JS's desire to keep Promise subclassing work, the recent PR to reduce number of ticks in await (tc39/ecma262#1250), the newly specified job settings in EnqueueJob, and possibly the upcoming Jobs infrastructure refactoring too. However the question is, is this behavior desirable? If not, is fixing it viable?

If we don't get to change how p was created, a workaround I found that'll let await keep working is:

p.then = Promise.prototype.then;

(I'm implementing #5212 in Chrome, and this showed up in one of the strange corners in WPT.)
@TimothyGu
Copy link
Member Author

Let me also add that Promise.resolve(p).then(…) will never actually call the handler, even though p.then(...) will.

/cc @bzbarsky @syg

@domenic
Copy link
Member

domenic commented Feb 29, 2020

I'm personally prepared to accept this as a weird consequence of a set of otherwise-reasonable rules. It's not a great end result, but it's acceptable for something that's somewhat of an edge case. (It's hard to imagine this coming up in a disruptive way outside of tests. Outside of tests, I don't think developers have strong expectations about promise callbacks firing after removing iframes.)

If I were to start picking out rules to change in order to align these, I'd probably start by trying to knock down promise subclassing support; adding all the complexity to support that is one my personal great regrets. E.g. I might suggest just changing step 2 in PromiseResolve to "If IsPromise(x), return x".

TimothyGu added a commit to TimothyGu/wpt that referenced this issue Mar 1, 2020
@TimothyGu
wakelock: Fix active document test
89a7451 
- Use DOMException constructor from the proper realm (regression after
  b7f2dd3).
- Make sure that promise microtasks are run, due to
  whatwg/html#5319.
@TimothyGu TimothyGu mentioned this issue Mar 1, 2020
Merged
@annevk annevk added integration Better coordination across standards needed topic: event loop labels Mar 2, 2020
@TimothyGu
Copy link
Member Author

Another work around seems to be new Promise(p.then.bind(p)), which seems a bit more palatable than the then overriding trick.

@domenic
Copy link
Member

domenic commented Mar 25, 2020

Closing as working as intended, I guess, since nobody seems to have strong opinions to the contrary. Let's just add it to the pile of evidence that promise subclassing was a bad idea.

@domenic domenic closed this as completed Mar 25, 2020
@karlt karlt mentioned this issue Oct 19, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
integration Better coordination across standards needed topic: event loop
Milestone
No milestone
Development

No branches or pull requests
3 participants
@domenic @TimothyGu @annevk