-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Prevent execution of (some) scripts moving between documents #2673
Conversation
This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.
This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.
4d941ba
to
1e038c9
Compare
Related to this, when we move scripts across documents, the following things (associated with Document) are not changed and thus, for example, set-of-scripts-that-will-execute-as-soon-as-possible of Document A can contain script elements that now belongs to another Document B, right? https://html.spec.whatwg.org/#list-of-scripts-that-will-execute-when-the-document-has-finished-parsing |
Also we might want to update the following non-normative notes: |
In general let me know what you think. I am not 100% confident in my interpretation of when we should fire the error event, but I think the latest is pretty good. Happy to hear your thoughts.
I looked into these, and most of them are not a problem. The lists will indeed contain scripts from the other document, but that's OK, it just means when we go to execute them, we do nothing. The pending parsing-blocking script is trickier, and I am not sure if maybe it is a problem. It seems like it might be OK to leave the spec as-is, and let it continue blocking until it is ready to execute, even though if it gets moved elsewhere, executing will be a no-op. What do you think?
If we go with the above (no change), then I think this note is still accurate.
This section seems to match the new spec text, but not match the old spec text, which is interesting. Did you have any change in mind? |
Talked with @hiroshige-g in person to help me page all this back into my head. Things to do:
|
ccff6c5
to
f19bef0
Compare
@hiroshige-g This should work better now; would love your review here and on web-platform-tests/wpt#5911 . I think our plan is still to not merge the spec change until we have shown this is web-compatible. However we can merge the test changes with a .tentative.html extension if that helps Blink's implementation. |
source
Outdated
<li> | ||
<p>If the element is flagged as <span>"parser-inserted"</span>, but <var>document</var> is | ||
not the <code>Document</code> of the parser that created the element, then return.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this the same check as the one about the settings object above, except this one is restricted to "parser-inserted" elements? (And this one isn't really grounded in proper primitives.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, as the note below explains, it can occur in a slightly different scenario. The script's script is created during "prepare a script", but the Document of the parser that created the element is determined before "prepare a script" runs. I think there were tests for this difference in the linked WPT PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems Firefox doesn't restrict this to "parser-inserted" scripts as I found in #3486. I also think that if we need to keep this it would be nice to clean it up by actually having a place to store that document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can a script have a parser that creates it, but not be parser-inserted?
This is essentially a null-check, i.e.
if (script.parserThisWasCreatedBy !== null && script.parserThisWasCreatedBy.document !== script.nodeDocument) {
return;
}
Without the first clause the second clause would "crash".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. What I meant was that Firefox seems to store a creator document on all script
elements and no longer execute the script
element if its node document does not match its creator document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual logic in Firefox is that we store the creator parser and then don't execute if that parser's doc does not match the owner doc. See https://searchfox.org/mozilla-central/rev/47cb352984bac15c476dcd75f8360f902673cb98/dom/script/ScriptElement.cpp#134-144
If you have a non-parser-inserted testcase that's showing the behavior you mention, I can take a look to see what's actually going on with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, we have an additional thing at https://searchfox.org/mozilla-central/source/dom/script/ScriptLoader.cpp#2182-2185
#3486 (comment) has more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And in particular, we store a "creator document" on the queued-up thing, not the script itself.
f19bef0
to
7348615
Compare
BTW given results at web-platform-tests/wpt#5911 (comment) I am not sure why I wanted to wait on Chrome before merging this. Roughly matching 2/4 browsers with a better behavior seems pretty reasonable, especially given Chrome's expressed interest in aligning with this spec change. |
This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.
I'm pretty sure that's what Gecko does on adoption, @bzbarsky can correct me. If it didn't it'd still leak the old realm and the goal of changing the prototype and such on adoption is to not leak that. |
In #3486 you state this would also fix that, but my results (note that I'm not testing the parser) have three browsers stacked against Firefox (and I haven't really tested external scripts yet). |
So I think we could still merge this and then have additional fixes for #3486 on top. But maybe you want to get the full story fixed first? |
source
Outdated
|
||
<p class="note">This can occur if, while the script was being fetched inside the <span>prepare a | ||
script</span> algorithm, the element was moved to a document without a <span | ||
data-x="concept-document-bc">browsing context</span>.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer it if we changed the "connected" check earlier to "browsing-context connected" and had a separate "scripting is disabled" check here with a qualifying example (and test). Since this example is addressed by improving the check earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to make that change separately, since scripting being disabled is a separate concept from being browsing-context connected. There are other cases where scripting can be disabled but still be in a browsing context, e.g. if the user has disabled JavaScript.
I think I would prefer the full story since when we ask implementers to make changes it's better to have a full accounting of the changes we are asking them to make. |
If that's the case I hope you can take over this PR and tests; I've done my best to converge on a behavior that has multi-implementer support, but I don't have time to do the full archeology here. I'd still prefer landing this as a series of well-layered changes. |
@annevk Chrome would like to ship this soon, at which point we would have 3/4 browsers matching this change. As I stated previously I would prefer to land this and then someone can work on #3486 on top of it. Do you still object to aligning with 3/4 browsers? @cdumez any interest in aligning Safari on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still a little unsure about making changes here that don't match anyone their model. But I guess none of it was well-defined to begin with...
No, wait, it is not redundant, I think... The question is whether we fetch the script or not if there's a parser/prepare-time mismatch. I.e. we'd need to test whether the server gets hit. Anyway, this is all more about #2137... |
95a1bba
to
29cd353
Compare
This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.
Note on Safari's behavior (probably not related to moving between Documents):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Content scripts which are from extension are getting blocked if they are specified to run at document start. What can we do to execute the content scripts at document start from extension.
29cd353
to
f2e2ad9
Compare
@hiroshige-g @annevk I've updated this rather-old pull request. It is now a one-line addition on top of #5154, so it should be easy to review. I've also updated the OP to use the modern HTML Standard pull request template, so we can have a clear sense of how this matches browsers. Please take another look! |
This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
According to the latest run of web-platform-tests/wpt#19632, the behavior diffs of current browsers are (after-prepare* cases):
- Firefox and Safari fires script or window error events
- Safari evaluates classic scripts
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 <[email protected]>
Edited the OP with Firefox and Safari bugs. Yay! |
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 <[email protected]> -- wpt-commits: 299dd665683c2c5a4896bfb7727d8666aa6bba89 wpt-pr: 19632
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 <[email protected]> -- wpt-commits: 299dd665683c2c5a4896bfb7727d8666aa6bba89 wpt-pr: 19632
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 <[email protected]> -- wpt-commits: 299dd665683c2c5a4896bfb7727d8666aa6bba89 wpt-pr: 19632
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 <[email protected]> -- wpt-commits: 299dd665683c2c5a4896bfb7727d8666aa6bba89 wpt-pr: 19632
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
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
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
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
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
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
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
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
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.
(See WHATWG Working Mode: Changes for more details.)
/scripting.html ( diff )