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

Fix import() inside setTimeout()/setInterval() strings #3117

Merged
merged 1 commit into from Dec 18, 2017

Conversation

3 participants
@domenic
Member

domenic commented Oct 11, 2017

This fixes part of #3116, leaving the question of cryptographic nonce
and parser metadata for later. For these two, the only impact is on
dynamic import() inside such strings.

Tests: web-platform-tests/wpt#7684

@nyaxt to review


/timers-and-user-prompts.html ( diff )

domenic added a commit to web-platform-tests/wpt that referenced this pull request Oct 11, 2017

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Oct 22, 2017

Member

When you get the active script here, can you be sure it's not null? Might be nice to add an assert.

Member

annevk commented Oct 22, 2017

When you get the active script here, can you be sure it's not null? Might be nice to add an assert.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Oct 27, 2017

Member

I actually cannot guarantee that. This is basically tc39/ecma262#871 all over again, arg. Let me think harder...

Member

domenic commented Oct 27, 2017

I actually cannot guarantee that. This is basically tc39/ecma262#871 all over again, arg. Let me think harder...

domenic added a commit that referenced this pull request Oct 27, 2017

Ensure there is an active script while running JS jobs
Fixes tc39/ecma262#871, given that we have our
own version of EnqueueJob. Important for #3117.

domenic added a commit that referenced this pull request Oct 27, 2017

Ensure there is an active script while running JS jobs
Fixes tc39/ecma262#871, given that we have our
own version of EnqueueJob. Important for #3117.

domenic added a commit that referenced this pull request Oct 27, 2017

Ensure there is an active script while running JS jobs
"Fixes" tc39/ecma262#871, at least for HTML,
given that we have our own version of EnqueueJob. Important for #3117.

domenic added a commit that referenced this pull request Nov 17, 2017

Ensure there is an active script while running JS jobs
"Fixes" tc39/ecma262#871, at least for HTML,
given that we have our own version of EnqueueJob. Important for #3117.

domenic added a commit that referenced this pull request Nov 17, 2017

Ensure there is an active script while running JS jobs
"Fixes" tc39/ecma262#871, at least for HTML,
given that we have our own version of EnqueueJob. Important for #3117.

domenic added a commit that referenced this pull request Nov 17, 2017

Ensure there is an active script while running JS jobs
"Fixes" tc39/ecma262#871, at least for HTML,
given that we have our own version of EnqueueJob. Important for #3117.
@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Dec 15, 2017

Member

This is ready for re-review. Working on tests now.

Member

domenic commented Dec 15, 2017

This is ready for re-review. Working on tests now.

@domenic domenic changed the title from Make setTimeout(string) scripts inherit base URL and credentials mode to Fix import() inside setTimeout()/setInterval() strings Dec 15, 2017

Fix import() inside setTimeout()/setInterval() strings
Closes #3116. Before this change, the "new script"-ness of setTimeout()
and setInterval()'s string compilation was making any import() calls
inside the compiled source behave unexpectedly. After this, they behave
like eval().

domenic added a commit to web-platform-tests/wpt that referenced this pull request Dec 15, 2017

More dynamic import() string compilation tests
Follows whatwg/html#3117. Parser state is implicitly tested because lots of tests would otherwise fail.

Referrer policy tests omitted for now since you can only set that via modulepreload at the moment.

@domenic domenic removed the needs tests label Dec 15, 2017

@annevk

annevk approved these changes Dec 16, 2017

I assume there are tests for the base URL thing? That it does indeed come from that script and not some settings object? Because I thought that the base URL always came from a settings object, but I might not have paid enough attention to all the settings object changes.

<var>calleeRealm</var>). If this throws an exception, catch it, and <span>report the
exception</span>.</p></li>
<var>calleeRealm</var>). If this throws an exception, catch it, <span>report the
exception</span>, and abort these steps.</p></li>

This comment has been minimized.

@annevk

annevk Dec 16, 2017

Member

return?

@annevk

annevk Dec 16, 2017

Member

return?

This comment has been minimized.

@domenic

domenic Dec 16, 2017

Member

This is inside a set of substeps that run in parallel.

@domenic

domenic Dec 16, 2017

Member

This is inside a set of substeps that run in parallel.

This comment has been minimized.

@annevk

annevk Dec 16, 2017

Member

How can you report an exception from there?

@annevk

annevk Dec 16, 2017

Member

How can you report an exception from there?

This comment has been minimized.

@domenic

domenic Dec 16, 2017

Member

Sorry, they run inside a task that is enqueued from in parallel. See https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#timer-initialisation-steps for the actual context.

@domenic

domenic Dec 16, 2017

Member

Sorry, they run inside a task that is enqueued from in parallel. See https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#timer-initialisation-steps for the actual context.

This comment has been minimized.

@annevk

annevk Dec 16, 2017

Member

In that case return seems fine, since it's a synchronous algorithm, but okay either way.

@annevk

annevk Dec 16, 2017

Member

In that case return seems fine, since it's a synchronous algorithm, but okay either way.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Dec 16, 2017

Member

Yeah, the -base-url tests in web-platform-tests/wpt#7684 test what you're referring to.

Member

domenic commented Dec 16, 2017

Yeah, the -base-url tests in web-platform-tests/wpt#7684 test what you're referring to.

@nyaxt

This comment has been minimized.

Show comment
Hide comment
@nyaxt

nyaxt Dec 18, 2017

Member

lgtm

Member

nyaxt commented Dec 18, 2017

lgtm

@domenic domenic merged commit 12cdfef into master Dec 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@domenic domenic deleted the inherit-some-setTimeout-stuff branch Dec 18, 2017

annevk added a commit to web-platform-tests/wpt that referenced this pull request Dec 18, 2017

More dynamic import() string compilation tests
Follows whatwg/html#3117. Parser state is implicitly tested because lots of tests would otherwise fail.

Referrer policy tests omitted for now since you can only set that via modulepreload at the moment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment