Do not re-initialize PendingScript in HTMLParserScriptRunner
Previously, |HTMLParserScriptRunner::m_parserBlockingScript| was
re-initialized every time it processes a new script tag
(i.e. a new ScriptLoader), and therefore one PendingScript can be
reused for multiple ScriptLoaders.
This CL makes |m_parserBlockingScript| to be newly created rather than
re-initialized with the same object.
This clarifies that a PendingScript corresponds to a ScriptLoader,
and also enables creating ModulePendingScript in a cleaner way in
https://codereview.chromium.org/2653923008/.
BUG=686281
Review-Url: https://codereview.chromium.org/2693423002
Cr-Commit-Position: refs/heads/master@{#451911}
diff --git a/third_party/WebKit/Source/bindings/core/v8/ScriptStreamerTest.cpp b/third_party/WebKit/Source/bindings/core/v8/ScriptStreamerTest.cpp
index 52ec735..ee31bc6 100644
--- a/third_party/WebKit/Source/bindings/core/v8/ScriptStreamerTest.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/ScriptStreamerTest.cpp
@@ -34,9 +34,9 @@
m_settings(Settings::create()),
m_resourceRequest("http://www.streaming-test.com/"),
m_resource(ScriptResource::create(m_resourceRequest, "UTF-8")),
- m_pendingScript(PendingScript::create(0, m_resource.get())) {
+ m_pendingScript(PendingScript::createForTesting(m_resource.get())) {
m_resource->setStatus(ResourceStatus::Pending);
- m_pendingScript = PendingScript::create(0, m_resource.get());
+ m_pendingScript = PendingScript::createForTesting(m_resource.get());
ScriptStreamer::setSmallScriptThresholdForTesting(0);
}
diff --git a/third_party/WebKit/Source/core/dom/PendingScript.cpp b/third_party/WebKit/Source/core/dom/PendingScript.cpp
index 4507e23..aef2998 100644
--- a/third_party/WebKit/Source/core/dom/PendingScript.cpp
+++ b/third_party/WebKit/Source/core/dom/PendingScript.cpp
@@ -35,16 +35,31 @@
PendingScript* PendingScript::create(Element* element,
ScriptResource* resource) {
- return new PendingScript(element, resource);
+ return new PendingScript(element, resource, TextPosition());
}
-PendingScript::PendingScript(Element* element, ScriptResource* resource)
+PendingScript* PendingScript::create(Element* element,
+ const TextPosition& startingPosition) {
+ return new PendingScript(element, nullptr, startingPosition);
+}
+
+PendingScript* PendingScript::createForTesting(ScriptResource* resource) {
+ return new PendingScript(nullptr, resource, TextPosition(), true);
+}
+
+PendingScript::PendingScript(Element* element,
+ ScriptResource* resource,
+ const TextPosition& startingPosition,
+ bool isForTesting)
: m_watchingForLoad(false),
m_element(element),
+ m_startingPosition(startingPosition),
m_integrityFailure(false),
m_parserBlockingLoadStartTime(0),
- m_client(nullptr) {
- setScriptResource(resource);
+ m_client(nullptr),
+ m_isForTesting(isForTesting) {
+ CHECK(m_isForTesting || m_element);
+ setResource(resource);
MemoryCoordinator::instance().registerClient(this);
}
@@ -55,7 +70,7 @@
DCHECK(!m_client);
DCHECK(!m_watchingForLoad);
- setScriptResource(nullptr);
+ setResource(nullptr);
m_startingPosition = TextPosition::belowRangePosition();
m_integrityFailure = false;
m_parserBlockingLoadStartTime = 0;
@@ -86,20 +101,19 @@
m_watchingForLoad = false;
}
+Element* PendingScript::element() const {
+ // As mentioned in the comment at |m_element| declaration, |m_element|
+ // must points to the corresponding ScriptLoader's element.
+ CHECK(m_element);
+ return m_element.get();
+}
+
void PendingScript::streamingFinished() {
DCHECK(resource());
if (m_client)
m_client->pendingScriptFinished(this);
}
-void PendingScript::setElement(Element* element) {
- m_element = element;
-}
-
-void PendingScript::setScriptResource(ScriptResource* resource) {
- setResource(resource);
-}
-
void PendingScript::markParserBlockingLoadStartTime() {
DCHECK_EQ(m_parserBlockingLoadStartTime, 0.0);
m_parserBlockingLoadStartTime = monotonicallyIncreasingTime();
@@ -128,6 +142,7 @@
// objects (perhaps attached to identical Resource objects) per request.
//
// See https://crbug.com/500701 for more information.
+ CHECK(m_isForTesting || m_element);
if (m_element) {
DCHECK_EQ(resource->getType(), Resource::Script);
ScriptResource* scriptResource = toScriptResource(resource);
diff --git a/third_party/WebKit/Source/core/dom/PendingScript.h b/third_party/WebKit/Source/core/dom/PendingScript.h
index 6c3dfaf..6b8c0cb 100644
--- a/third_party/WebKit/Source/core/dom/PendingScript.h
+++ b/third_party/WebKit/Source/core/dom/PendingScript.h
@@ -70,13 +70,16 @@
WTF_MAKE_NONCOPYABLE(PendingScript);
public:
+ // For script from an external file.
static PendingScript* create(Element*, ScriptResource*);
+ // For inline script.
+ static PendingScript* create(Element*, const TextPosition&);
+
+ static PendingScript* createForTesting(ScriptResource*);
+
~PendingScript() override;
TextPosition startingPosition() const { return m_startingPosition; }
- void setStartingPosition(const TextPosition& position) {
- m_startingPosition = position;
- }
void markParserBlockingLoadStartTime();
// Returns the time the load of this script started blocking the parser, or
// zero if this script hasn't yet blocked the parser, in
@@ -88,10 +91,7 @@
void watchForLoad(PendingScriptClient*);
void stopWatchingForLoad();
- Element* element() const { return m_element.get(); }
- void setElement(Element*);
-
- void setScriptResource(ScriptResource*);
+ Element* element() const;
DECLARE_TRACE();
@@ -107,7 +107,10 @@
void dispose();
private:
- PendingScript(Element*, ScriptResource*);
+ PendingScript(Element*,
+ ScriptResource*,
+ const TextPosition&,
+ bool isForTesting = false);
PendingScript() = delete;
// ScriptResourceClient
@@ -119,13 +122,21 @@
void onPurgeMemory() override;
bool m_watchingForLoad;
+
+ // |m_element| must points to the corresponding ScriptLoader's element and
+ // thus must be non-null before dispose() is called (except for unit tests).
Member<Element> m_element;
+
TextPosition m_startingPosition; // Only used for inline script tags.
bool m_integrityFailure;
double m_parserBlockingLoadStartTime;
Member<ScriptStreamer> m_streamer;
Member<PendingScriptClient> m_client;
+
+ // This flag is used to skip non-null checks of |m_element| in unit tests,
+ // because |m_element| can be null in unit tests.
+ const bool m_isForTesting;
};
} // namespace blink
diff --git a/third_party/WebKit/Source/core/dom/ScriptLoader.cpp b/third_party/WebKit/Source/core/dom/ScriptLoader.cpp
index 59c1816..e32b451 100644
--- a/third_party/WebKit/Source/core/dom/ScriptLoader.cpp
+++ b/third_party/WebKit/Source/core/dom/ScriptLoader.cpp
@@ -142,7 +142,7 @@
m_nonBlocking = false;
}
-void ScriptLoader::detach() {
+void ScriptLoader::detachPendingScript() {
if (!m_pendingScript)
return;
m_pendingScript->dispose();
@@ -817,7 +817,7 @@
DCHECK(m_pendingScript->resource());
bool errorOccurred = false;
ScriptSourceCode source = m_pendingScript->getSource(KURL(), errorOccurred);
- m_pendingScript->dispose();
+ detachPendingScript();
if (errorOccurred) {
dispatchErrorEvent();
} else if (!m_resource->wasCanceled()) {
@@ -848,7 +848,7 @@
Document* contextDocument = m_element->document().contextDocument();
if (!contextDocument) {
- detach();
+ detachPendingScript();
return;
}
@@ -857,7 +857,7 @@
if (m_resource->errorOccurred()) {
contextDocument->scriptRunner()->notifyScriptLoadError(this,
m_asyncExecType);
- detach();
+ detachPendingScript();
dispatchErrorEvent();
return;
}
diff --git a/third_party/WebKit/Source/core/dom/ScriptLoader.h b/third_party/WebKit/Source/core/dom/ScriptLoader.h
index d483642b..295c12c 100644
--- a/third_party/WebKit/Source/core/dom/ScriptLoader.h
+++ b/third_party/WebKit/Source/core/dom/ScriptLoader.h
@@ -107,9 +107,6 @@
return m_pendingScript && m_pendingScript->errorOccurred();
}
- // Clears the connection to the PendingScript (and Element and Resource).
- void detach();
-
bool wasCreatedDuringDocumentWrite() { return m_createdDuringDocumentWrite; }
bool disallowedFetchForDocWrittenScript() {
@@ -136,6 +133,9 @@
ScriptLoaderClient* client() const;
+ // Clears the connection to the PendingScript.
+ void detachPendingScript();
+
// PendingScriptClient
void pendingScriptFinished(PendingScript*) override;
diff --git a/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp b/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp
index dd2b05ff..ec9bf4e 100644
--- a/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp
+++ b/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp
@@ -157,7 +157,7 @@
: m_reentryPermit(reentryPermit),
m_document(document),
m_host(host),
- m_parserBlockingScript(PendingScript::create(nullptr, nullptr)) {
+ m_parserBlockingScript(nullptr) {
DCHECK(m_host);
}
@@ -170,7 +170,9 @@
if (!m_document)
return;
- m_parserBlockingScript->dispose();
+ if (m_parserBlockingScript)
+ m_parserBlockingScript->dispose();
+ m_parserBlockingScript = nullptr;
while (!m_scriptsToExecuteAfterParsing.isEmpty()) {
PendingScript* pendingScript = m_scriptsToExecuteAfterParsing.takeFirst();
@@ -183,9 +185,10 @@
}
bool HTMLParserScriptRunner::isParserBlockingScriptReady() {
+ DCHECK(parserBlockingScript());
if (!m_document->isScriptExecutionReady())
return false;
- return m_parserBlockingScript->isReady();
+ return parserBlockingScript()->isReady();
}
// This has two callers and corresponds to different concepts in the spec:
@@ -228,6 +231,11 @@
// There is no longer a pending parsing-blocking script."
// Clear the pending script before possible re-entrancy from executeScript()
pendingScript->dispose();
+ pendingScript = nullptr;
+
+ if (pendingScriptType == ScriptStreamer::ParsingBlocking) {
+ m_parserBlockingScript = nullptr;
+ }
if (ScriptLoader* scriptLoader = toScriptLoaderIfPossible(element)) {
// 7. "Increment the parser's script nesting level by one (it should be
@@ -312,16 +320,15 @@
PendingScript* pendingScript) {
// If the script was blocked as part of document.write intervention,
// then send an asynchronous GET request with an interventions header.
- TextPosition startingPosition;
- bool isParserInserted = false;
- if (m_parserBlockingScript != pendingScript)
+ if (!parserBlockingScript())
return;
- Element* element = m_parserBlockingScript->element();
- if (!element)
+ if (parserBlockingScript() != pendingScript)
return;
+ Element* element = parserBlockingScript()->element();
+
ScriptLoader* scriptLoader = toScriptLoaderIfPossible(element);
if (!scriptLoader || !scriptLoader->disallowedFetchForDocWrittenScript())
return;
@@ -338,8 +345,8 @@
emitErrorForDocWriteScripts(pendingScript->resource()->url().getString(),
*m_document);
- startingPosition = m_parserBlockingScript->startingPosition();
- isParserInserted = scriptLoader->isParserInserted();
+ TextPosition startingPosition = parserBlockingScript()->startingPosition();
+ bool isParserInserted = scriptLoader->isParserInserted();
// Remove this resource entry from memory cache as the new request
// should not join onto this existing entry.
memoryCache()->remove(pendingScript->resource());
@@ -402,7 +409,7 @@
// - "Otherwise":
- traceParserBlockingScript(m_parserBlockingScript.get(),
+ traceParserBlockingScript(parserBlockingScript(),
!m_document->isScriptExecutionReady());
m_parserBlockingScript->markParserBlockingLoadStartTime();
@@ -416,7 +423,7 @@
}
bool HTMLParserScriptRunner::hasParserBlockingScript() const {
- return !!m_parserBlockingScript->element();
+ return parserBlockingScript();
}
// The "Otherwise" Clause of 'An end tag whose tag name is "script"'
@@ -442,7 +449,7 @@
InsertionPointRecord insertionPointRecord(m_host->inputStream());
// 1., 7.--9.
- executePendingScriptAndDispatchEvent(m_parserBlockingScript.get(),
+ executePendingScriptAndDispatchEvent(m_parserBlockingScript,
ScriptStreamer::ParsingBlocking);
// 10. "Let the insertion point be undefined again."
@@ -458,8 +465,8 @@
TRACE_EVENT0("blink", "HTMLParserScriptRunner::executeScriptsWaitingForLoad");
DCHECK(!isExecutingScript());
DCHECK(hasParserBlockingScript());
- DCHECK_EQ(pendingScript, m_parserBlockingScript);
- DCHECK(m_parserBlockingScript->isReady());
+ DCHECK_EQ(pendingScript, parserBlockingScript());
+ DCHECK(parserBlockingScript()->isReady());
executeParsingBlockingScripts();
}
@@ -522,20 +529,22 @@
// "The element is the pending parsing-blocking script of the Document of
// the parser that created the element.
// (There can only be one such script per Document at a time.)"
- if (!requestPendingScript(m_parserBlockingScript.get(), element))
+ CHECK(!parserBlockingScript());
+ m_parserBlockingScript = requestPendingScript(element);
+ if (!parserBlockingScript())
return;
- DCHECK(m_parserBlockingScript->resource());
+ DCHECK(parserBlockingScript()->resource());
// We only care about a load callback if resource is not already in the cache.
// Callers will attempt to run the m_parserBlockingScript if possible before
// returning control to the parser.
- if (!m_parserBlockingScript->isReady()) {
+ if (!parserBlockingScript()->isReady()) {
if (m_document->frame()) {
ScriptState* scriptState = ScriptState::forMainWorld(m_document->frame());
if (scriptState) {
ScriptStreamer::startStreaming(
- m_parserBlockingScript.get(), ScriptStreamer::ParsingBlocking,
+ m_parserBlockingScript, ScriptStreamer::ParsingBlocking,
m_document->frame()->settings(), scriptState,
TaskRunnerHelper::get(TaskType::Networking, m_document));
}
@@ -547,8 +556,8 @@
// 1st Clause, Step 23 of https://html.spec.whatwg.org/#prepare-a-script
void HTMLParserScriptRunner::requestDeferredScript(Element* element) {
- PendingScript* pendingScript = PendingScript::create(nullptr, nullptr);
- if (!requestPendingScript(pendingScript, element))
+ PendingScript* pendingScript = requestPendingScript(element);
+ if (!pendingScript)
return;
if (m_document->frame() && !pendingScript->isReady()) {
@@ -569,18 +578,16 @@
m_scriptsToExecuteAfterParsing.append(pendingScript);
}
-bool HTMLParserScriptRunner::requestPendingScript(PendingScript* pendingScript,
- Element* script) const {
- DCHECK(!pendingScript->element());
- pendingScript->setElement(script);
+PendingScript* HTMLParserScriptRunner::requestPendingScript(
+ Element* element) const {
// This should correctly return 0 for empty or invalid srcValues.
- ScriptResource* resource = toScriptLoaderIfPossible(script)->resource();
+ ScriptResource* resource = toScriptLoaderIfPossible(element)->resource();
if (!resource) {
DVLOG(1) << "Not implemented."; // Dispatch error event.
- return false;
+ return nullptr;
}
- pendingScript->setScriptResource(resource);
- return true;
+
+ return PendingScript::create(element, resource);
}
// The initial steps for 'An end tag whose tag name is "script"'
@@ -637,15 +644,18 @@
// "The element is the pending parsing-blocking script of the
// Document of the parser that created the element.
// (There can only be one such script per Document at a time.)"
- m_parserBlockingScript->setElement(script);
- m_parserBlockingScript->setStartingPosition(scriptStartPosition);
+ CHECK(!m_parserBlockingScript);
+ m_parserBlockingScript =
+ PendingScript::create(script, scriptStartPosition);
} else {
// 6th Clause of Step 23.
// "Immediately execute the script block,
// even if other scripts are already executing."
// TODO(hiroshige): Merge the block into ScriptLoader::prepareScript().
DCHECK_GT(m_reentryPermit->scriptNestingLevel(), 1u);
- m_parserBlockingScript->dispose();
+ if (m_parserBlockingScript)
+ m_parserBlockingScript->dispose();
+ m_parserBlockingScript = nullptr;
ScriptSourceCode sourceCode(script->textContent(),
documentURLForScriptExecution(m_document),
scriptStartPosition);
diff --git a/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.h b/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.h
index 2c9a3b9..69060e6 100644
--- a/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.h
+++ b/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.h
@@ -107,13 +107,17 @@
void requestParsingBlockingScript(Element*);
void requestDeferredScript(Element*);
- bool requestPendingScript(PendingScript*, Element*) const;
+ PendingScript* requestPendingScript(Element*) const;
// Processes the provided script element, but does not execute any
// parsing-blocking scripts that may remain after execution.
void processScriptElementInternal(Element*,
const TextPosition& scriptStartPosition);
+ const PendingScript* parserBlockingScript() const {
+ return m_parserBlockingScript;
+ }
+
bool isParserBlockingScriptReady();
void possiblyFetchBlockedDocWriteScript(PendingScript*);
