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

Ensure there is an active script while running JS jobs #3163

Merged
merged 2 commits into from
Nov 17, 2017

Conversation

domenic
Copy link
Member

@domenic domenic commented Oct 27, 2017

"Fixes" tc39/ecma262#871, at least for HTML, given that we have our own version of EnqueueJob. Important for #3117.


People who may be interested in reviewing this: @anba @bzbarsky @nyaxt. Alternately I'm happy with just editorial review as this stuff is pretty subtle.

Tests: web-platform-tests/wpt#8287

source Outdated

<p class="note">This affects the <span>active script</span> while the job runs, in cases like
<code data-x="">Promise.resolve("...").then(eval)</code> where there would otherwise be no
active script since <code data-x="">eval</code> is a built-in function that does not original
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/original/originate/?

@anba
Copy link

anba commented Nov 1, 2017

This appears to be correct for fixing tc39/ecma262#871 in HTML, but it could be that I'm missing some HTML-specific details, because I simply don't know the HTML spec that well. 😄

And I'm still wondering if calling GetActiveScriptOrModule and creating a new execution context (or its equivalent in actual implementations), will be an expensive operation in implementations, because it needs to be done whenever a new Promise Job gets enqueued. Well, let's just see what implementors think of this change.

@domenic
Copy link
Member Author

domenic commented Nov 17, 2017

Tests are up. This is ready for review/merging.

Copy link
Contributor

@sideshowbarker sideshowbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as far as editorial review

@annevk
Copy link
Member

annevk commented Nov 17, 2017

It seems this includes a commit that shouldn't be here.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving "Ensure there is an active script while running JS jobs" only.

source Outdated
<li><p><span>Prepare to run a callback</span> with <var>incumbent settings</var>.</p></li>
<p class="note">This affects the <span>active script</span> while the job runs, in cases like
<code data-x="">Promise.resolve("...").then(eval)</code> where there would otherwise be no
active script since <code data-x="">eval</code> is a built-in function that does not originate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eval()

"Fixes" tc39/ecma262#871, at least for HTML,
given that we have our own version of EnqueueJob. Important for #3117.
@domenic domenic merged commit cc3fcc8 into master Nov 17, 2017
@domenic domenic deleted the enqueuejob-script branch November 17, 2017 16:48
jgraham pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 21, 2018
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 30, 2018
…ise jobs, a=testonly

Automatic update from web-platform-testsTest active script, via import(), in promise jobs

Follows whatwg/html#3163.

--

wpt-commits: 1c3621705bdd047fde1e4a2a165c49ec837ffa1c
wpt-pr: 8287
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 2, 2019
…ise jobs, a=testonly

Automatic update from web-platform-testsTest active script, via import(), in promise jobs

Follows whatwg/html#3163.

--

wpt-commits: 1c3621705bdd047fde1e4a2a165c49ec837ffa1c
wpt-pr: 8287

UltraBlame original commit: 89d303876a7e1b2b43d099822477b7f80333b71b
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
…ise jobs, a=testonly

Automatic update from web-platform-testsTest active script, via import(), in promise jobs

Follows whatwg/html#3163.

--

wpt-commits: 1c3621705bdd047fde1e4a2a165c49ec837ffa1c
wpt-pr: 8287

UltraBlame original commit: 89d303876a7e1b2b43d099822477b7f80333b71b
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 3, 2019
…ise jobs, a=testonly

Automatic update from web-platform-testsTest active script, via import(), in promise jobs

Follows whatwg/html#3163.

--

wpt-commits: 1c3621705bdd047fde1e4a2a165c49ec837ffa1c
wpt-pr: 8287

UltraBlame original commit: 89d303876a7e1b2b43d099822477b7f80333b71b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants