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

"Entry" concept is broken while executing enqueued jobs #1426

Closed
domenic opened this issue Jun 15, 2016 · 5 comments · Fixed by #5212
Closed

"Entry" concept is broken while executing enqueued jobs #1426

domenic opened this issue Jun 15, 2016 · 5 comments · Fixed by #5212
Assignees
@domenic
Labels
topic: multiple globals

Comments

@domenic
Copy link
Member

domenic commented Jun 15, 2016

#1189 ties up almost all the loose ends around defining entry and incumbent, as part of a series of work (most of which can be found by following links from #473). However, in the course of working on it, @bzbarsky and I discovered that figuring out how to preserve the entry concept correctly while executing EnqueueJob (i.e., promise jobs) is tricky. It looks like doing so would either require some strange contortions, or some more invasive changes to JS's promise mechanisms. See #1189 (comment) onward for details. In the meantime, #1189 adds a warning:

Let job settings be some appropriate environment settings object.

Warning: It is not yet clear how to specify the environment settings object that should be used here. In practice, this means that the entry concept is not correctly specified while executing a job. See discussion in issue #1189.
@domenic domenic self-assigned this Jun 15, 2016
domenic added a commit that referenced this issue Jun 15, 2016
@domenic
Fix incumbent settings object definition and add examples
f97c3e4 
As discussed in #473, starting especially from around
#473 (comment), the
definition of incumbent introduced in #401 falls down in certain
important cases. In order to fix this, we introduce several new
concepts, which takes care of these trickier examples. These examples
are now included in the spec, and spell out exactly how exactly
incumbent settings object calculation works in increasingly-complex
scenarios.

The new algorithms "prepare to run a callback" and "clean up after
running a callback" will be used by Web IDL, similarly to how it already
uses "prepare to run script" and "clean up after running script."

Another notable change is that EnqueueJob now correctly tracks the
necessary goings-on in order to make the incumbent settings object work
correctly when promises are used to schedule callbacks. However, we
noticed that it does not correctly track the entry settings object; the
previous text, introduced in #1091, incorrectly referred to
job.[[Realm]], which does not exist. The correct fix is unfortunately
not obvious. So we add a warning there for now, with #1426 tracking
further work.
@domenic
Copy link
Member Author

domenic commented Jun 28, 2016

Hahahaha look at what I found from 2014: domenic/promises-unwrapping#108

@domenic domenic mentioned this issue Jul 14, 2016
Closed
@domenic
Copy link
Member Author

domenic commented Oct 27, 2017

@bzbarsky this suddenly came up as indirectly blocking me on #3117.

It looks like reviewing earlier threads the following would be the most consistent:

  • PromiseResolveThenableJob(promiseToResolve, thenable, then): should choose [[Realm]] of then
  • PromiseReactionJob (reaction, argument): should choose [[Realm]] of reaction.[[Handler]] when reaction.[[Handler]] exists; otherwise it is unobservable (no user code is called)

Does this seem right to you, and/or match with Firefox (especially after recent work to move promises into the JS engine)?

This is going to suck to write tests for.

Right now as a spec strategy I am contemplating just listing these two cases in the HTML spec's EnqueueJob. Maybe we can do better ES integration later if they ever want to work with us on jobs.

@bzbarsky
Copy link
Contributor

I'll try to dig through the Firefox code here, but it certainly won't happen until Monday. Maybe @tschneidereit recalls offhand what we ended up implementing...

@tschneidereit
Copy link

It looks like reviewing earlier threads the following would be the most consistent:

  • PromiseResolveThenableJob(promiseToResolve, thenable, then): should choose [[Realm]] of then
  • PromiseReactionJob (reaction, argument): should choose [[Realm]] of reaction.[[Handler]] when reaction.[[Handler]] exists; otherwise it is unobservable (no user code is called)

I think that matches exactly what we implemented. See this comment and this one for PromiseReactionJob and this one for PromiseResolveThenableJob.

@domenic domenic mentioned this issue Feb 8, 2019
Closed
joyeecheung pushed a commit to joyeecheung/v8 that referenced this issue Feb 26, 2019
@tzik
Use handler's context on Promise resolution
334510a 
V8 used to use the microtask context when it runs EnqueueJob
step 2.
> Let job settings be some appropriate environment settings object.
https://html.spec.whatwg.org/multipage/webappapis.html#enqueuejob(queuename,-job,-arguments)

However, it's being updated to use the handler's context.
whatwg/html#1426 (comment)

Change-Id: I24840a28ef2c903539fe4ace74ae59da290f5109
Reviewed-on: https://chromium-review.googlesource.com/c/1465902
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Taiju Tsuiki <tzik@chromium.org>
Cr-Commit-Position: refs/heads/master@{#59870}
@tzik tzik mentioned this issue Mar 25, 2019
Open
@domenic domenic mentioned this issue Jan 15, 2020
Merged
@domenic
Copy link
Member Author

domenic commented Jan 15, 2020

I tried to spec out #1426 (comment) but I realized that

otherwise it is unobservable (no user code is called)

is not quite true, because the settings object chosen here is also used for determining whether we can run script. This runs into the thorny issues around promises in navigated-away-from documents.

I guess there is no harm in running promise.then(null, null) inside scripting-disabled documents?

domenic added a commit that referenced this issue Jan 15, 2020
@domenic
Fix the entry settings object for promise callbacks
325bff9 
Closes #1426.
This was referenced Jan 15, 2020
Merged
Closed
domenic added a commit that referenced this issue Feb 12, 2020
@domenic
Fix the entry settings object for promise callbacks
380763b 
Closes #1426.
domenic added a commit that referenced this issue Feb 12, 2020
@domenic
Fix the entry settings object for promise callbacks
344798b 
Closes #1426.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@domenic domenic

Labels
topic: multiple globals
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix the entry settings object for promise callbacks whatwg/html
3 participants
@tschneidereit @domenic @bzbarsky