Skip to content

Commit

Permalink
Script: Implement the parser document check
Browse files Browse the repository at this point in the history
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
  • Loading branch information
domfarolino authored and Commit Bot committed Jun 11, 2020
1 parent ca66a16 commit bdc15d6
Show file tree
Hide file tree
Showing 19 changed files with 34 additions and 153 deletions.
18 changes: 13 additions & 5 deletions blink/renderer/core/script/script_loader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,17 @@ ScriptLoader::ScriptLoader(ScriptElementBase* element,
already_started_ = true;

if (flags.IsCreatedByParser()) {
// <spec href="https://html.spec.whatwg.org/C/#parser-inserted">... It is
// <spec href="https://html.spec.whatwg.org/C/#parser-inserted">script
// elements with non-null parser documents are known as
// "parser-inserted".</spec>
// For more information on why this is not implemented in terms of a
// non-null parser document, see the documentation in the header file.
parser_inserted_ = true;

// <spec href="https://html.spec.whatwg.org/C/#parser-document">... It is
// set by the HTML parser and the XML parser on script elements they insert
// ...</spec>
parser_inserted_ = true;
parser_document_ = flags.ParserDocument();

// <spec href="https://html.spec.whatwg.org/C/#non-blocking">... It is unset
// by the HTML parser and the XML parser on script elements they insert.
Expand All @@ -107,6 +114,7 @@ ScriptLoader::~ScriptLoader() {}

void ScriptLoader::Trace(Visitor* visitor) const {
visitor->Trace(element_);
visitor->Trace(parser_document_);
visitor->Trace(pending_script_);
visitor->Trace(prepared_pending_script_);
visitor->Trace(resource_keep_alive_);
Expand Down Expand Up @@ -338,9 +346,9 @@ bool ScriptLoader::PrepareScript(const TextPosition& script_start_position,
// <spec step="10">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 return.</spec>
//
// FIXME: If script is parser inserted, verify it's still in the original
// document.
if (parser_inserted_ && parser_document_ != &element_->GetDocument()) {
return false;
}

// <spec step="11">If scripting is disabled for the script element, then
// return. The script is not executed.</spec>
Expand Down
18 changes: 16 additions & 2 deletions blink/renderer/core/script/script_loader.h
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,22 @@ class CORE_EXPORT ScriptLoader final : public GarbageCollected<ScriptLoader>,
// script elements must have this flag unset ...</spec>
bool already_started_ = false;

// <spec href="https://html.spec.whatwg.org/C/#parser-inserted">... Initially,
// script elements must have this flag unset. ...</spec>
// <spec href="https://html.spec.whatwg.org/C/#parser-document">... Initially,
// its value must be null. It is set by the HTML parser and the XML parser on
// script elements they insert ...</spec>
// We use a WeakMember here because we're keeping the parser-inserted
// information separately from the parser document, so ScriptLoader doesn't
// need to keep the parser document alive.
WeakMember<Document> parser_document_;

// <spec href="https://html.spec.whatwg.org/C/#parser-inserted">script
// elements with non-null parser documents are known as
// "parser-inserted".</spec>
// Note that we don't actually implement "parser inserted" in terms of a
// non-null |parser_document_| like the spec, because it is possible for
// |CreateElementFlags::created_by_parser_| to be true even when
// |CreateElementFlags::parser_document_| is null. Therefore, we have to
// store this information separately.
bool parser_inserted_ = false;

// <spec href="https://html.spec.whatwg.org/C/#non-blocking">... Initially,
Expand Down

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

11 changes: 0 additions & 11 deletions blink/web_tests/fast/parser/move-during-parsing-expected.txt

This file was deleted.

13 changes: 0 additions & 13 deletions blink/web_tests/fast/parser/move-during-parsing.html

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@
<body>
<div>
<script>
if (parent.document.adoptNode)
parent.document.body.appendChild(parent.document.adoptNode(document.getElementsByTagName("div")[0]));
else
parent.document.body.appendChild(document.getElementsByTagName("div")[0]);
parent.document.body.appendChild(document.querySelector("div"));
</script>
<script>
// By the time #prepare-a-script is called, this script will be inserted into
// the outer document. At this time, the script's parser document and
// preparation-time document are different, and as per #prepare-a-script step
// 12, the script will not be executed.
alert('should be outer: ' + document.URL.match(/parser.*/));
</script>
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
ALERT: should be outer: parser/script-modify-page-outer.html
ALERT: should be inner: parser/resources/script-modify-page-inner.html
DONE

0 comments on commit bdc15d6

Please sign in to comment.