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

Scripts moving between documents #2469

Closed
domenic opened this issue Mar 27, 2017 · 8 comments
Closed

Scripts moving between documents #2469

domenic opened this issue Mar 27, 2017 · 8 comments
Labels
interop Implementations are not interoperable with each other topic: script

Comments

@domenic
Copy link
Member

domenic commented Mar 27, 2017

Diffing https://html.spec.whatwg.org/commit-snapshots/c9e804f04d03a0658bfa689cb0f368a4d2e37936 against the current spec, I seem to have inadvertently modified the semantics of what happens when:

  1. A script is inserted into document old, causing it to fetch
  2. While it is being fetched, the script is moved to document new
  3. The fetch finishes and the script executes

In the old spec (which only contained classic scripts), the script would execute in document new.

In the current spec, both classic and module scripts execute in document old.

I'm not sure exactly what the consequences of executing in a document are for classic scripts; I need to research a bit more. For module scripts it impacts which module map they go into.

I doubt changing from executing in new to old is web-compatible. new is probably better anyway. So I just need to fix the spec most likely. I'll be sure to add tests when I do (especially for module scripts).

/cc @whatwg/modules discovered by @hiroshige-g

@rniwa
Copy link

rniwa commented Mar 27, 2017

What do existing browsers do with respect to classic scripts?

@domenic
Copy link
Member Author

domenic commented Mar 27, 2017

Yeah, fair point, I'd better test that everywhere. Blink apparently uses document new.

@domenic
Copy link
Member Author

domenic commented Mar 27, 2017

Note: I bungled the original post here, and my follow-up, by confusing "document A" and "document B" in a few places. I've since edited it to use documents new and old, and to state things correctly instead of backward. Apologies!

@zcorpan
Copy link
Member

zcorpan commented Mar 27, 2017

This seems related to #2137 except not involving the parser, right?

Test for classic script: http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4982

Firefox does not execute the script at all.

@domenic
Copy link
Member Author

domenic commented Mar 27, 2017

Neither does Edge, according to that test case!

Both Chrome and Safari TP log "inner: delayed-script", so they go with the document new.

I imagine Firefox probably doesn't want to enable this behavior for similar reasons as they don't want to for #2137, but I haven't reviewed those reasons recently; maybe they were parser-specific.

Well, this kind of sucks. I guess the spec is definitely wrong since it says old, but now I'm not sure what to do...

@domenic
Copy link
Member Author

domenic commented Mar 27, 2017

Talking with @nyaxt and @hiroshige-g we are all tentatively fans of the Edge and Firefox behavior. Notably there have been several bad XSS-related bugs in Chrome due to this code path, and since it seems likely web-compatible to just make scripts moving between documents never execute, we'd like to try that.

@rniwa, if we can show that's web-compatible by shipping it in Chrome, does it seem like a reasonable thing for us to spec and for you to implement in WebKit?

/cc @bzbarsky

@domenic domenic added the interop Implementations are not interoperable with each other label Mar 28, 2017
@domenic
Copy link
Member Author

domenic commented May 12, 2017

It appears I mis-tested Edge, and it does execute... we're still hoping to try this in Chrome though.

@domenic
Copy link
Member Author

domenic commented May 12, 2017

I take it back. Edge does not execute, but if you type anything into live-dom-viewer it will. Proper web platform tests (uploading momentarily) show that it does not.

Firefox, notably, fires the onerror event. I don't think that's correct, so I'll test that it's not allowed.

domenic added a commit that referenced this issue May 20, 2017
domenic added a commit that referenced this issue Sep 22, 2017
Fixes #2469. Also clarifies surrounding conditions and cleans up the
algorithm by pulling out a variable for the element's node document.
domenic added a commit that referenced this issue Feb 20, 2018
Fixes #2469. Also clarifies surrounding conditions and cleans up the
algorithm by pulling out a variable for the element's node document.
domenic added a commit that referenced this issue Dec 13, 2019
This causes scripts that move between documents between the preparation
and execution phases to not execute, aligning with most browsers. Closes
#2469.

This does not address #2137, which is about scripts moving between
documents between the parsing and preparation, or parsing and execution
phases.
domenic added a commit that referenced this issue Jan 10, 2020
This causes scripts that move between documents between the preparation
and execution phases to not execute, aligning with most browsers. Closes
#2469.

This does not address #2137, which is about scripts moving between
documents between the parsing and preparation, or parsing and execution
phases.
webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this issue Aug 11, 2021
https://bugs.webkit.org/show_bug.cgi?id=202714
<rdar://problem/56208425>

Reviewed by Geoffrey Garen.

LayoutTests/imported/w3c:

Rebaseline WPT tests now that more checks are passing. Note that these checks were already passing in both Firefox and Chrome.

* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/after-prepare-iframe-fetch-error-external-classic-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/after-prepare-iframe-fetch-error-external-module-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/after-prepare-iframe-parse-error-external-classic-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/after-prepare-iframe-parse-error-external-module-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/after-prepare-iframe-parse-error-inline-classic-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/after-prepare-iframe-success-external-classic-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/after-prepare-iframe-success-external-module-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/after-prepare-iframe-success-inline-classic-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/move-back-iframe-fetch-error-external-classic-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/move-back-iframe-fetch-error-external-module-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/move-back-iframe-parse-error-external-classic-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/move-back-iframe-parse-error-external-module-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/move-back-iframe-parse-error-inline-classic-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/move-back-iframe-success-external-classic-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/move-back-iframe-success-external-module-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/move-back-iframe-success-inline-classic-expected.txt:

Source/WebCore:

Stop evaluating <script>s moved between Documents during fetching:
- whatwg/html#2469
- whatwg/html#2673

Both Firefox and Chrome already behave this way.

No new tests, rebaselined existing tests.

* dom/ScriptElement.cpp:
(WebCore::ScriptElement::prepareScript):
Set the element's preparation-time document to its node document, as per:
- https://html.spec.whatwg.org/multipage/scripting.html#prepare-a-script (step 11)

(WebCore::ScriptElement::executePendingScript):
If scriptElement's preparation-time document is not equal to scriptElement's node document, then return, as per:
- https://html.spec.whatwg.org/multipage/scripting.html#execute-the-script-block (step 2)

* dom/ScriptElement.h:


Canonical link: https://commits.webkit.org/240442@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@280924 268f45cc-cd09-0410-ab3c-d52691b4dbfc
bertogg pushed a commit to Igalia/webkit that referenced this issue Aug 16, 2021
https://bugs.webkit.org/show_bug.cgi?id=202714
<rdar://problem/56208425>

Reviewed by Geoffrey Garen.

LayoutTests/imported/w3c:

Rebaseline WPT tests now that more checks are passing. Note that these checks were already passing in both Firefox and Chrome.

* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/after-prepare-iframe-fetch-error-external-classic-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/after-prepare-iframe-fetch-error-external-module-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/after-prepare-iframe-parse-error-external-classic-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/after-prepare-iframe-parse-error-external-module-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/after-prepare-iframe-parse-error-inline-classic-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/after-prepare-iframe-success-external-classic-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/after-prepare-iframe-success-external-module-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/after-prepare-iframe-success-inline-classic-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/move-back-iframe-fetch-error-external-classic-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/move-back-iframe-fetch-error-external-module-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/move-back-iframe-parse-error-external-classic-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/move-back-iframe-parse-error-external-module-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/move-back-iframe-parse-error-inline-classic-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/move-back-iframe-success-external-classic-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/move-back-iframe-success-external-module-expected.txt:
* web-platform-tests/html/semantics/scripting-1/the-script-element/moving-between-documents/move-back-iframe-success-inline-classic-expected.txt:

Source/WebCore:

Stop evaluating <script>s moved between Documents during fetching:
- whatwg/html#2469
- whatwg/html#2673

Both Firefox and Chrome already behave this way.

No new tests, rebaselined existing tests.

* dom/ScriptElement.cpp:
(WebCore::ScriptElement::prepareScript):
Set the element's preparation-time document to its node document, as per:
- https://html.spec.whatwg.org/multipage/scripting.html#prepare-a-script (step 11)

(WebCore::ScriptElement::executePendingScript):
If scriptElement's preparation-time document is not equal to scriptElement's node document, then return, as per:
- https://html.spec.whatwg.org/multipage/scripting.html#execute-the-script-block (step 2)

* dom/ScriptElement.h:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@280924 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Implementations are not interoperable with each other topic: script
Development

No branches or pull requests

4 participants