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

Compat: Should the parser execute scripts created in another document #2137

Closed
jakearchibald opened this issue Dec 6, 2016 · 22 comments
Closed
Labels
interop Implementations are not interoperable with each other security/privacy There are security or privacy implications topic: parser topic: script

Comments

@jakearchibald
Copy link
Contributor

The spec says no:

[Scripts] parsed into different Documents than the one the parser was created for do not execute

But Edge, Chrome and Safari all allow it. Reduced test (see the console) - source.

I really like how this silly hack allows you to inject content in a streaming manner in a way that's very close to how a regular page-load behaves, including executing scripts, but some kind of streaming document fragment would be a better way to deliver that.

@zcorpan
Copy link
Member

zcorpan commented Dec 6, 2016

The normative part is

  1. If the element is flagged as "parser-inserted", but the element's node document is not the Document of the parser that created the element, then abort these steps.

https://html.spec.whatwg.org/multipage/scripting.html#prepare-a-script

and

  1. If the element is flagged as "parser-inserted", but the element's node document is not the Document of the parser that created the element, then abort these steps.

https://html.spec.whatwg.org/multipage/scripting.html#execute-the-script-block

@zcorpan
Copy link
Member

zcorpan commented Dec 6, 2016

Checking git log --grep "execut" I found:

This changed in:
7c52fca

Other commits that may be relevant:
90d9255
2ba7d3a
41c8f10

@zcorpan zcorpan added security/privacy There are security or privacy implications topic: parser labels Dec 6, 2016
@zcorpan
Copy link
Member

zcorpan commented Dec 6, 2016

cc @hsivonen @bzbarsky

@bzbarsky
Copy link
Contributor

bzbarsky commented Dec 6, 2016

I think https://bugzilla.mozilla.org/show_bug.cgi?id=592366 summarizes the issue well enough, no? It's possible to enable this behavior, but it leads to all sorts of complexity in the interaction of script elements and the parser, which is already quite complex. Absent an actual web compat need, I don't think we should be introducing this complexity into the spec and into implementations. Of course @jakearchibald just tried his hardest to introduce actual web compat need. :(

@jakearchibald
Copy link
Contributor Author

Meh, I'm proposing it as an alternative to innerHTML = fetchedContent, which doesn't execute script either.

@bzbarsky
Copy link
Contributor

bzbarsky commented Dec 6, 2016

Well, except for the part where you said it executes scripts. ;)

Anyway, for this particular case the real question is whether the non-Gecko browsers are willing to align with the spec. If not, I would like to see a concrete spec proposal that describes what their actual behavior is, assuming they actually have the same behavior.

@bzbarsky
Copy link
Contributor

bzbarsky commented Dec 6, 2016

And unrelatedly, we should in fact just have some sort of streaming-parse-into-an-existing-document API that doesn't involve hacks. Is there an existing issue filed on that?

@jakearchibald
Copy link
Contributor Author

jakearchibald commented Dec 6, 2016

I know it's something @domenic has had bouncing around his head. I'm hoping this iframe hack shows it could be easier than we initially thought.

I'd love to be able to do:

const div = document.createElement('div');
document.body.appendChild(div);
const response = await fetch(url);
await response.body.pipeTo(div.writable);

…or something.

@domenic
Copy link
Member

domenic commented Dec 6, 2016

I think the "concrete proposal" being discussed is to remove the line @zcorpan pointed out. That seems like the right thing to do unless we can get implementer commitment to remove this behavior in other engines. Hmm, @yuki3 maybe?

@bzbarsky
Copy link
Contributor

bzbarsky commented Dec 6, 2016

Is just removing that line enough to deal with the issues Henri raised? As a simple example, has someone carefully stepped through the resulting logic and ensured that the parser being blocked and unblocked ends up being the same parser?

@bzbarsky
Copy link
Contributor

bzbarsky commented Dec 6, 2016

And, importantly, has someone audited every use of "node document" around the <script> processing model to make sure it (1) makes sense and (2) matches the non-Gecko implementations if this restriction is removed?

@zcorpan
Copy link
Member

zcorpan commented Dec 6, 2016

If the script should be executed then this thread is reopened: https://lists.w3.org/Archives/Public/public-whatwg-archive/2010Sep/0030.html

@bzbarsky
Copy link
Contributor

bzbarsky commented Dec 6, 2016

Ah, I had forgotten about that one. It's been a while.

I strongly urge everyone in this discussion to read that entire thread, which describes the incompatibilities in behavior at the time, the crashes this behavior caused in WebKit at the time, and the lack of clarity around interactions with CSP and whatnot....

Then please come back with an actual concrete proposal and tests that show what browsers actually do in the various interesting edge cases.

@jakearchibald
Copy link
Contributor Author

@bzbarsky I've created #2142 to discuss a less-hacky solution for streaming HTML into an element.

@hsivonen
Copy link
Member

hsivonen commented Dec 8, 2016

I think we should keep the spec as not executing scripts that have moved between docs during the parse. As the earlier-referenced W3C and Mozilla Bugzilla entries show, the spec text was arrived at as a result of implementor feedback resulting from first trying to do things the way that allowed the execution of moved scripts.

@domenic
Copy link
Member

domenic commented May 12, 2017

Note that Edge allows less than Chrome (and I assume Safari) here; in Jake's reduced test case, it only logs "Inline script", and not "Inline script" + "External script" as Chrome does.

I'm going to write tests against this behavior and hopefully Chrome can conform with the current spec as part of also fixing #2673.

domenic added a commit to web-platform-tests/wpt that referenced this issue May 12, 2017
This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.
domenic added a commit to web-platform-tests/wpt that referenced this issue May 20, 2017
This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.
@domenic domenic added the interop Implementations are not interoperable with each other label Aug 28, 2017
domenic added a commit to web-platform-tests/wpt that referenced this issue Feb 20, 2018
This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.
domenic added a commit to hiroshige-g/wpt that referenced this issue Nov 4, 2019
This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.
domenic added a commit to hiroshige-g/wpt that referenced this issue Dec 4, 2019
This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.
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.
hiroshige-g pushed a commit to hiroshige-g/wpt that referenced this issue Apr 21, 2020
This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.
domenic added a commit that referenced this issue Apr 21, 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.

Tests: web-platform-tests/wpt#19632
stephenmcgruer pushed a commit to web-platform-tests/wpt that referenced this issue Apr 22, 2020
This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.

Co-Authored-By: Domenic Denicola <d@domenic.me>
@domenic
Copy link
Member

domenic commented Apr 22, 2020

I think we're in a better position to tackle this issue now, as:

In particular there are two pieces of the current spec that are signposted as "under debate":

@hiroshige-g, would you be able to extend your test generator to see whether browsers implement these checks or not? That seems like the first step.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 1, 2020
Automatic update from web-platform-tests
Scripts moved between documents (#19632)

This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.

Co-Authored-By: Domenic Denicola <d@domenic.me>
--

wpt-commits: 299dd665683c2c5a4896bfb7727d8666aa6bba89
wpt-pr: 19632
xeonchen pushed a commit to xeonchen/gecko that referenced this issue May 1, 2020
Automatic update from web-platform-tests
Scripts moved between documents (#19632)

This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.

Co-Authored-By: Domenic Denicola <d@domenic.me>
--

wpt-commits: 299dd665683c2c5a4896bfb7727d8666aa6bba89
wpt-pr: 19632
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 1, 2020
Automatic update from web-platform-tests
Scripts moved between documents (#19632)

This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.

Co-Authored-By: Domenic Denicola <d@domenic.me>
--

wpt-commits: 299dd665683c2c5a4896bfb7727d8666aa6bba89
wpt-pr: 19632
xeonchen pushed a commit to xeonchen/gecko that referenced this issue May 1, 2020
Automatic update from web-platform-tests
Scripts moved between documents (#19632)

This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.

Co-Authored-By: Domenic Denicola <d@domenic.me>
--

wpt-commits: 299dd665683c2c5a4896bfb7727d8666aa6bba89
wpt-pr: 19632
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue May 3, 2020
Automatic update from web-platform-tests
Scripts moved between documents (#19632)

This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.

Co-Authored-By: Domenic Denicola <ddomenic.me>
--

wpt-commits: 299dd665683c2c5a4896bfb7727d8666aa6bba89
wpt-pr: 19632

UltraBlame original commit: 1dec2d9d8ecb6c3d48e595c3fe53167f365dd7a9
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue May 3, 2020
Automatic update from web-platform-tests
Scripts moved between documents (#19632)

This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.

Co-Authored-By: Domenic Denicola <ddomenic.me>
--

wpt-commits: 299dd665683c2c5a4896bfb7727d8666aa6bba89
wpt-pr: 19632

UltraBlame original commit: fdfdeaafaa3ea6cd8883bc904cd1fdab533b22e9
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue May 3, 2020
Automatic update from web-platform-tests
Scripts moved between documents (#19632)

This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.

Co-Authored-By: Domenic Denicola <ddomenic.me>
--

wpt-commits: 299dd665683c2c5a4896bfb7727d8666aa6bba89
wpt-pr: 19632

UltraBlame original commit: 1dec2d9d8ecb6c3d48e595c3fe53167f365dd7a9
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue May 3, 2020
Automatic update from web-platform-tests
Scripts moved between documents (#19632)

This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.

Co-Authored-By: Domenic Denicola <ddomenic.me>
--

wpt-commits: 299dd665683c2c5a4896bfb7727d8666aa6bba89
wpt-pr: 19632

UltraBlame original commit: fdfdeaafaa3ea6cd8883bc904cd1fdab533b22e9
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue May 3, 2020
Automatic update from web-platform-tests
Scripts moved between documents (#19632)

This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.

Co-Authored-By: Domenic Denicola <ddomenic.me>
--

wpt-commits: 299dd665683c2c5a4896bfb7727d8666aa6bba89
wpt-pr: 19632

UltraBlame original commit: 1dec2d9d8ecb6c3d48e595c3fe53167f365dd7a9
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue May 3, 2020
Automatic update from web-platform-tests
Scripts moved between documents (#19632)

This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.

Co-Authored-By: Domenic Denicola <ddomenic.me>
--

wpt-commits: 299dd665683c2c5a4896bfb7727d8666aa6bba89
wpt-pr: 19632

UltraBlame original commit: fdfdeaafaa3ea6cd8883bc904cd1fdab533b22e9
@domfarolino
Copy link
Member

@hiroshige-g, would you be able to extend your test generator to see whether browsers implement these checks or not? That seems like the first step.

Doesn't the test generator already support testing the points scenarios you raised @domenic?

@domenic
Copy link
Member

domenic commented May 26, 2020

I think you're right, @domfarolino! Thanks for looking into it.

it seems like Chrome is the only impl (I'm not counting Edge) that actually fails any before-prepare tests

Safari too, right?

@domfarolino
Copy link
Member

Safari too, right?

That's correct, not sure why I thought otherwise.

domfarolino added a commit to web-platform-tests/wpt that referenced this issue May 27, 2020
This PR adds empty-src tests, which moves <script src=""> elements
between Documents before #prepare-a-script. If the parser document
check is done in #prepare-a-script, the <script>'s error event is not fired,
and thus we can more-completely test whether the parser document check is done.

This PR also does some minor refactoring / documentation clean-up, as well as
adds a TODO to remove some of the tests once whatwg/html#2137
is closed and whatwg/html#5575 lands.

Co-authored-by: Dominic Farolino <domfarolino@gmail.com>
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 1, 2020
…e in #prepare-a-script, a=testonly

Automatic update from web-platform-tests
Test whether parse document check is done in #prepare-a-script (#23162)

This PR adds empty-src tests, which moves <script src=""> elements
between Documents before #prepare-a-script. If the parser document
check is done in #prepare-a-script, the <script>'s error event is not fired,
and thus we can more-completely test whether the parser document check is done.

This PR also does some minor refactoring / documentation clean-up, as well as
adds a TODO to remove some of the tests once whatwg/html#2137
is closed and whatwg/html#5575 lands.

Co-authored-by: Dominic Farolino <domfarolino@gmail.com>
--

wpt-commits: eccee3ede4f1debb1ad5bd34e6122bcb2cadce48
wpt-pr: 23162
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Jun 2, 2020
…e in #prepare-a-script, a=testonly

Automatic update from web-platform-tests
Test whether parse document check is done in #prepare-a-script (#23162)

This PR adds empty-src tests, which moves <script src=""> elements
between Documents before #prepare-a-script. If the parser document
check is done in #prepare-a-script, the <script>'s error event is not fired,
and thus we can more-completely test whether the parser document check is done.

This PR also does some minor refactoring / documentation clean-up, as well as
adds a TODO to remove some of the tests once whatwg/html#2137
is closed and whatwg/html#5575 lands.

Co-authored-by: Dominic Farolino <domfarolino@gmail.com>
--

wpt-commits: eccee3ede4f1debb1ad5bd34e6122bcb2cadce48
wpt-pr: 23162
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Jun 11, 2020
The #prepare-a-script algorithm [1] includes a check asserting that a
script element's parser document == its node document, for
parser-inserted scripts. Spec discussion has been happening around this
at [2], and WPTs have been added for this in [3] and [4] as part of an
overall effort to test and better-specify the behavior of script elements
that move between documents.

Before this CL, Chromium had no concept of a parser document, and
therefore would execute scripts that were moved to another document
before ScriptLoader::PrepareScript was invoked.

This CL introduces a |parser_document_| to ScriptLoader, which is
populated from CreateElementFlags::ParserDocument(), similar to the
|parser_inserted_| flag.

[1]: https://html.spec.whatwg.org/C/#prepare-a-script
[2]: whatwg/html#2137
[2]: web-platform-tests/wpt#19632
[3]: web-platform-tests/wpt#23162

Bug: 721914, 1086507
Change-Id: I7a0980afb47be93f8ed9948658b2cc8e4fa04669
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2214301
Commit-Queue: Dominic Farolino <dom@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Cr-Commit-Position: refs/heads/master@{#777203}
@domfarolino
Copy link
Member

Given the fact that Firefox already ships this, and the UseCounter for scripts that were moved between preparation and execution was very low in Chromium, I've gone ahead and shipped this in Chrome. I think we can close this issue now?

@domenic
Copy link
Member

domenic commented Jun 15, 2020

Excellent. I'll merge #5575 then.

mfreed7 pushed a commit to mfreed7/html that referenced this issue Sep 11, 2020
This remove notes pointing to whatwg#2137, and thus closes whatwg#2137. We'll proceed with
preventing scripts from executing if they've moved between documents, as this is
now shipping in two engines.
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
The #prepare-a-script algorithm [1] includes a check asserting that a
script element's parser document == its node document, for
parser-inserted scripts. Spec discussion has been happening around this
at [2], and WPTs have been added for this in [3] and [4] as part of an
overall effort to test and better-specify the behavior of script elements
that move between documents.

Before this CL, Chromium had no concept of a parser document, and
therefore would execute scripts that were moved to another document
before ScriptLoader::PrepareScript was invoked.

This CL introduces a |parser_document_| to ScriptLoader, which is
populated from CreateElementFlags::ParserDocument(), similar to the
|parser_inserted_| flag.

[1]: https://html.spec.whatwg.org/C/#prepare-a-script
[2]: whatwg/html#2137
[2]: web-platform-tests/wpt#19632
[3]: web-platform-tests/wpt#23162

Bug: 721914, 1086507
Change-Id: I7a0980afb47be93f8ed9948658b2cc8e4fa04669
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2214301
Commit-Queue: Dominic Farolino <dom@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#777203}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: f104795245d1a32fde965862578baeb6e75961be
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 security/privacy There are security or privacy implications topic: parser topic: script
Development

No branches or pull requests

7 participants