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

Propagate active script to callbacks #902

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

domenic
Copy link
Member

@domenic domenic commented Jul 9, 2020

See discussion in tc39/ecma262#2086 (comment).

This ensures that in cases like

<!-- index.html -->
<script src="sub/path/foo.js"></script>
// sub/path/foo.js
setTimeout(eval, 0, 'import(`./example.mjs`)');

the resulting imported module is sub/path/example.mjs, and not example.mjs. I.e. it ensures that import() resolves relative to the active script, even when it is being called via a Web IDL callback.

Tests: web-platform-tests/wpt#24535

/cc @hiroshige-g as he was involved in very similar discussions for cases like setTimeout("import(`./example.mjs`)") and @yuki3 as the Chromium binding person working on incumbents.

/cc @jonco3 as he worked on fixing the tests for very similar issues.


Preview | Diff

domenic added a commit to web-platform-tests/wpt that referenced this pull request Jul 9, 2020
@yuki3
Copy link

yuki3 commented Jul 9, 2020

Quite interesting. Thanks for letting me know. :)

@domenic
Copy link
Member Author

domenic commented Jul 10, 2020

Based on the test results at web-platform-tests/wpt#24535, nobody seems to implement this yet. Even Firefox, who does great on the existing tests.

However, I think this is definitely the right thing to do. Can you agree on Chrome's behalf, @yuki3 and/or @hiroshige-g? It's fine if it's lower priority, but I think we should make sure this happens...

@yuki3
Copy link

yuki3 commented Jul 11, 2020

I don't fully understand about 'import', but I totally agree that 'eval's incumbent must be the realm of the caller of 'setTimeout'. Thus, I agree with the test.

AFAIK, in Chromium, setTimeout + IDL callback function already supports the backup incumbent stack, so probably this is @hiroshige-g's area, I think?

@domenic
Copy link
Member Author

domenic commented Jul 13, 2020

Basically, this PR is adding another "incumbent-like thing" that gets propagated in the same way as incumbent: namely, the active script. In particular, the active script's base URL is used for import().

So the example setTimeout(eval, 0, 'import(`./example.mjs`)'); is about propagating the active script, not about propagating the incumbent. (The incumbent would come into play with an example like setTimeout(eval, 0, 'postMessage(...)').)

So indeed, this is a bit more in @hiroshige-g's area. But, it will likely impact the bindings code, because it will need propagation through Web IDL callback conversion + calling in the same way incumbent currently does.

@yuki3
Copy link

yuki3 commented Jul 13, 2020

Oh my. I'm sorry. Now I read this PR a little more carefully. IIUC, active script is not always the script or module of the incumbent realm, so we have to track active script in addition to incumbent. It means another runtime cost. Is this correct?

If so, it will be 25% object size increase of all IDL callback functions and IDL callback interfaces (in Blink). Probably it's okay, but if there is a guarantee that, in case of IDL callbacks, the incumbent settings object's execution context's ScriptOrModule never be null, that's great. (Anyway, I think IDL callbacks will be okay.)

I think it's better to notify those who will implement Promise callbacks and WeakRef callbacks of Chromium. They will need to implement not only the incumbent but also the active script.

Summary:

  1. I think we can support active script in IDL callback functions/interfaces. But not sure about Promise and WeakRef callbacks.
  2. Q: Is there a case that IDL callback's incumbent's execution context's ScriptOrModule is null?

@domenic
Copy link
Member Author

domenic commented Jul 13, 2020

Yes, I was a bit worried about the runtime costs. I'm glad you think that cost will be OK, but don't hesitate to push back if you think it is too expensive. Although I think this change is a good one, I don't think it's infinitely good; if it has high costs then we should reevaluate.

I think it's better to notify those who will implement Promise callbacks and WeakRef callbacks of Chromium. They will need to implement not only the incumbent but also the active script.

Yes, @syg is helping with that, and that is actually what prompted this discussion. Currently promises are specced to propagate active script, and they do in Chromium; we were working on generalizing that to WeakRef, but noticed that there was an inconsistency with Web IDL callbacks.

Q: Is there a case that IDL callback's incumbent's execution context's ScriptOrModule is null?

I think it is never null. By "incumbent's execution context" you probably mean the execution context in step 1 of https://html.spec.whatwg.org/#incumbent-settings-object? In that case then it is definitely never null.

This is very interesting. I guess your idea is that Chromium would store the "topmost script-having execution context" (i.e. "incumbent's execution context") instead of storing the (incumbent settings object, active script) tuple? Then it could infer active settings object using the algorithm in https://html.spec.whatwg.org/#incumbent-settings-object, and active script using the ScriptOrModule component.

I think that works, and means no extra runtime cost. (At least, in spec terms; I am not sure how it maps to Chromium implementation primitives.)

@yuki3
Copy link

yuki3 commented Jul 13, 2020

I think it is never null. By "incumbent's execution context" you probably mean the execution context in step 1 of https://html.spec.whatwg.org/#incumbent-settings-object? In that case then it is definitely never null.

Yes, I meant it, and I had the same idea with yours. That's a good news that we can avoid increase of memory consumption about IDL callbacks. :) Then, I have no concern on this PR.

@domenic
Copy link
Member Author

domenic commented Jul 13, 2020

Excellent. I will probably update this PR to use that technique, then, to make it more obvious for implementers.

@syg
Copy link
Contributor

syg commented Jul 13, 2020

I'll update whatwg/html#5722 to follow suit so we have a common mechanism.

@domenic
Copy link
Member Author

domenic commented Jul 13, 2020

Hmm, now I am worried about the case where the topmost script-having execution context is null. In that case we still need to capture the incumbent settings object as per the backup stack.

I think this could still be done, but we would have to convert the backup incumbent settings object stack into a backup incumbent execution context stack.

That is a larger change. So I would prefer to land this plus whatwg/html#5722 first using the mechanism from this PR, i.e. an (incumbent settings object, active script) tuple. Then I can follow up with a refactoring to consolidate everything to use execution contexts.

@hiroshige-g
Copy link

+1 for making active script the ScriptOrModule component of the incumbent.
Active script and incumbent have similar intention, and the concept of incumbent is already complicated (e.g. whatwg/html#1430). It's better to merge active script into incumbent and maintain the incumbent spec/impl as the single source of truth.

I also suspect the current browsers implementation is more like referring the incumbent rather than the current specification of active script, according to cases where a script triggers event firing synchronously.
WPT: web-platform-tests/wpt#24305
In such cases, https://html.spec.whatwg.org/multipage/webappapis.html#skip-when-determining-incumbent-counter makes the incumbent's script and the active script different.

(Thanks @yuki3, the chat we had today was quite helpful for me!)

@domenic
Copy link
Member Author

domenic commented Jul 22, 2020

+1 for making active script the ScriptOrModule component of the incumbent.

This change will require more 262-side changes, @syg. In particular https://tc39.es/ecma262/#sec-getactivescriptormodule would not longer be the correct algorithm; we'd need instead to skip execution contexts whose skip-when-determining-incumbent counter is non-zero.

@syg
Copy link
Contributor

syg commented Jul 22, 2020

Oh boy. That makes me very wary to support such a unification...

@domenic
Copy link
Member Author

domenic commented Jul 22, 2020

Yeah, I can understand that. Here is my proposal:

  1. Land Track the incumbent settings and active script in Promise callbacks html#5722 (tracking active script/incumbent for promises correctly)
  2. Land Describe integration with the WeakRefs proposal html#4571 on top of that (weak references, with proper active script/incumbent tracking)
  3. Land this PR and its associated tests, in basically this form. (Tracking active script for Web IDL callbacks)
  4. Investigate unification, i.e. no longer tracking active script and incumbent separately, but instead tracking incumbent execution context, and deriving active script from that.
    • This could involve tests like @hiroshige-g's above. Or even simplifications of incumbent to make it more like active script.

I will set aside my (local on-disk) unification pull request, so we can proceed incrementally. So at this point I think the ball is in other folks' courts. In particular, @syg is working on those HTML pull requests (and their 262 dependencies), and it would be good to get Mozilla or Apple input on the question posed by this PR and its associated tests about

// sub/path/foo.js
setTimeout(eval, 0, 'import(`./example.mjs`)');

@annevk
Copy link
Member

annevk commented Jul 23, 2020

I discussed it briefly with @jonco3 and neither of us feel strongly.

@littledan
Copy link
Collaborator

This change feels "correct" to me, but I'm curious, does it affect any cases besides when eval (or possibly other kinds of functions implemented by the platform which parse JavaScript) is used directly as a WebIDL callback? This feels like a relatively narrow case; it doesn't seem so bad to fall back to the document's base URL in this edge case.

I'm wondering how much harm the current semantics of falling back to import() being relative to the document's base URL is causing for web developers. Have we seen any bug reports from web developers confused about these semantics, for example?

@domenic domenic self-assigned this Aug 2, 2021
@domenic
Copy link
Member Author

domenic commented Aug 30, 2021

I'm starting to come around to @littledan's point of view, that while this is the correct thing to do, but is not worth the implementation effort.

does it affect any cases besides when eval (or possibly other kinds of functions implemented by the platform which parse JavaScript) is used directly as a WebIDL callback?

It affects any case where the platform function is dependent on the active script. I think in practice that's only going to be things like eval and setTimeout, because the only thing that currently depends on the active script is import() path resolution, and the only thing that triggers import() is author code.

(maybe it could affect exception stack traces??)

We may want to remove active script tracking from job callbacks as well, since @hiroshige-g is discovering that no browsers seem to do that either?

@hiroshige-g
Copy link

FYI The test that confirms there seem no active scripts for Promise.resolve('import(...)').then(eval) on browsers is web-platform-tests/wpt#30241.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 7, 2021
The tests fail on Safari/Firefox/Chromium, and thus this change
exposes that the browsers set no active scripts for
`Promise.resolve(...).then(eval)`.

Bug: 1245063, whatwg/webidl#902
Change-Id: Ic7debcd66e3f90c2fcb973d1e4100daabf2895dc
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 7, 2021
The tests fail on Safari/Firefox/Chromium, and thus this change
exposes that the browsers set no active scripts for
`Promise.resolve(...).then(eval)`.

Bug: 1245063, whatwg/webidl#902
Change-Id: Ic7debcd66e3f90c2fcb973d1e4100daabf2895dc
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 7, 2021
The tests fail on Safari/Firefox/Chromium, and thus this change
exposes that the browsers set no active scripts for
`Promise.resolve(...).then(eval)`.

Bug: 1245063, whatwg/webidl#902
Change-Id: Ic7debcd66e3f90c2fcb973d1e4100daabf2895dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3130646
Commit-Queue: Hiroshige Hayashizaki <[email protected]>
Reviewed-by: Domenic Denicola <[email protected]>
Cr-Commit-Position: refs/heads/main@{#918862}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 7, 2021
The tests fail on Safari/Firefox/Chromium, and thus this change
exposes that the browsers set no active scripts for
`Promise.resolve(...).then(eval)`.

Bug: 1245063, whatwg/webidl#902
Change-Id: Ic7debcd66e3f90c2fcb973d1e4100daabf2895dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3130646
Commit-Queue: Hiroshige Hayashizaki <[email protected]>
Reviewed-by: Domenic Denicola <[email protected]>
Cr-Commit-Position: refs/heads/main@{#918862}
pull bot pushed a commit to jamlee-t/chromium that referenced this pull request Sep 8, 2021
The tests fail on Safari/Firefox/Chromium, and thus this change
exposes that the browsers set no active scripts for
`Promise.resolve(...).then(eval)`.

Bug: 1245063, whatwg/webidl#902
Change-Id: Ic7debcd66e3f90c2fcb973d1e4100daabf2895dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3130646
Commit-Queue: Hiroshige Hayashizaki <[email protected]>
Reviewed-by: Domenic Denicola <[email protected]>
Cr-Commit-Position: refs/heads/main@{#918862}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 22, 2021
…mpilation-of-promise-result.html, a=testonly

Automatic update from web-platform-tests
[WPT] Distinguish base URLs in string-compilation-of-promise-result.html

The tests fail on Safari/Firefox/Chromium, and thus this change
exposes that the browsers set no active scripts for
`Promise.resolve(...).then(eval)`.

Bug: 1245063, whatwg/webidl#902
Change-Id: Ic7debcd66e3f90c2fcb973d1e4100daabf2895dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3130646
Commit-Queue: Hiroshige Hayashizaki <[email protected]>
Reviewed-by: Domenic Denicola <[email protected]>
Cr-Commit-Position: refs/heads/main@{#918862}

--

wpt-commits: e51a2c65259e52cf9258660ed0a3d8565409e128
wpt-pr: 30241
aosmond pushed a commit to aosmond/gecko that referenced this pull request Sep 24, 2021
…mpilation-of-promise-result.html, a=testonly

Automatic update from web-platform-tests
[WPT] Distinguish base URLs in string-compilation-of-promise-result.html

The tests fail on Safari/Firefox/Chromium, and thus this change
exposes that the browsers set no active scripts for
`Promise.resolve(...).then(eval)`.

Bug: 1245063, whatwg/webidl#902
Change-Id: Ic7debcd66e3f90c2fcb973d1e4100daabf2895dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3130646
Commit-Queue: Hiroshige Hayashizaki <[email protected]>
Reviewed-by: Domenic Denicola <[email protected]>
Cr-Commit-Position: refs/heads/main@{#918862}

--

wpt-commits: e51a2c65259e52cf9258660ed0a3d8565409e128
wpt-pr: 30241
Gabisampaio pushed a commit to Gabisampaio/wpt that referenced this pull request Nov 18, 2021
The tests fail on Safari/Firefox/Chromium, and thus this change
exposes that the browsers set no active scripts for
`Promise.resolve(...).then(eval)`.

Bug: 1245063, whatwg/webidl#902
Change-Id: Ic7debcd66e3f90c2fcb973d1e4100daabf2895dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3130646
Commit-Queue: Hiroshige Hayashizaki <[email protected]>
Reviewed-by: Domenic Denicola <[email protected]>
Cr-Commit-Position: refs/heads/main@{#918862}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
The tests fail on Safari/Firefox/Chromium, and thus this change
exposes that the browsers set no active scripts for
`Promise.resolve(...).then(eval)`.

Bug: 1245063, whatwg/webidl#902
Change-Id: Ic7debcd66e3f90c2fcb973d1e4100daabf2895dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3130646
Commit-Queue: Hiroshige Hayashizaki <[email protected]>
Reviewed-by: Domenic Denicola <[email protected]>
Cr-Commit-Position: refs/heads/main@{#918862}
NOKEYCHECK=True
GitOrigin-RevId: 068960dd4873c69c2d588fe17bfa68480614983e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants