Do not re-initialize PendingScript in HTMLParserScriptRunner

third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp

Index: third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp
diff --git a/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp b/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp
index 3e25498464fb2d31de3e0953a636f297a7c036ba..ff592f12d2271bcc8c57915e8e3121b1b3833fdd 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 @@ HTMLParserScriptRunner::HTMLParserScriptRunner(
     : m_reentryPermit(reentryPermit),
       m_document(document),
       m_host(host),
-      m_parserBlockingScript(PendingScript::create(nullptr, nullptr)) {
+      m_parserBlockingScript(nullptr) {

[sof, 2017/02/16 08:15:16]

I don't mind terribly, but null-initializing Member<>s isn't needed.

[hiroshige, 2017/02/17 23:15:15]

I'd like to leave this to make it more explicitly that now
|m_parserBlockingScript| is initialized to nullptr.

   DCHECK(m_host);
 }
 
@@ -170,7 +170,9 @@ void HTMLParserScriptRunner::detach() {
   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 @@ void HTMLParserScriptRunner::detach() {
 }
 
 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 @@ void HTMLParserScriptRunner::executePendingScriptAndDispatchEvent(
   //     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
@@ -315,13 +323,14 @@ void HTMLParserScriptRunner::possiblyFetchBlockedDocWriteScript(
   TextPosition startingPosition;

[sof, 2017/02/16 08:15:16]

nit: could you move these decls down to where they're initially bound?

[hiroshige, 2017/02/17 23:15:15]

Done.

   bool isParserInserted = false;
 
-  if (m_parserBlockingScript != pendingScript)
+  if (parserBlockingScript() != pendingScript)
     return;
 
-  Element* element = m_parserBlockingScript->element();
-  if (!element)
+  if (!parserBlockingScript())

[sof, 2017/02/16 08:15:16]

I think it makes sense to have this check first.

[hiroshige, 2017/02/17 23:15:15]

Done.

     return;
 
+  Element* element = parserBlockingScript()->element();
+
   ScriptLoader* scriptLoader = toScriptLoaderIfPossible(element);
   if (!scriptLoader || !scriptLoader->disallowedFetchForDocWrittenScript())
     return;
@@ -338,7 +347,7 @@ void HTMLParserScriptRunner::possiblyFetchBlockedDocWriteScript(
 
   emitErrorForDocWriteScripts(pendingScript->resource()->url().getString(),
                               *m_document);
-  startingPosition = m_parserBlockingScript->startingPosition();
+  startingPosition = parserBlockingScript()->startingPosition();
   isParserInserted = scriptLoader->isParserInserted();
   // Remove this resource entry from memory cache as the new request
   // should not join onto this existing entry.
@@ -401,7 +410,7 @@ void HTMLParserScriptRunner::processScriptElement(
 
     // - "Otherwise":
 
-    traceParserBlockingScript(m_parserBlockingScript.get(),
+    traceParserBlockingScript(parserBlockingScript(),
                               !m_document->isScriptExecutionReady());
     m_parserBlockingScript->markParserBlockingLoadStartTime();
 
@@ -415,7 +424,7 @@ void HTMLParserScriptRunner::processScriptElement(
 }
 
 bool HTMLParserScriptRunner::hasParserBlockingScript() const {
-  return !!m_parserBlockingScript->element();
+  return parserBlockingScript();
 }
 
 // Implements the "Otherwise" Clause of 'An end tag whose tag name is "script"'
@@ -440,7 +449,7 @@ void HTMLParserScriptRunner::executeParsingBlockingScripts() {
     InsertionPointRecord insertionPointRecord(m_host->inputStream());
 
     // 1., 7.--9.
-    executePendingScriptAndDispatchEvent(m_parserBlockingScript.get(),
+    executePendingScriptAndDispatchEvent(m_parserBlockingScript,

[sof, 2017/02/16 08:15:16]

parserBlockingScript() for consistency.

[hiroshige, 2017/02/17 23:15:15]

I used parserBlockingScript() for const ref and m_parserBlockingScript only for
non-const ref.
Here non-const ref is needed and thus I used m_parserBlockingScript.

                                          ScriptStreamer::ParsingBlocking);
 
     // 10. "Let the insertion point be undefined again."
@@ -456,8 +465,8 @@ void HTMLParserScriptRunner::executeScriptsWaitingForLoad(
   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();
 }
 
@@ -520,20 +529,22 @@ void HTMLParserScriptRunner::requestParsingBlockingScript(Element* element) {
   // "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));
       }
@@ -545,8 +556,8 @@ void HTMLParserScriptRunner::requestParsingBlockingScript(Element* element) {
 
 // 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()) {
@@ -567,18 +578,16 @@ void HTMLParserScriptRunner::requestDeferredScript(Element* element) {
   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) {

[kouhei (in TOK), 2017/02/16 00:55:09]

Do we ever get here?

[hiroshige, 2017/02/16 01:13:15]

Probably no, because fetchScript() should have returned false.
I'll create a separate CL to replace this if statement with CHECK().

[hiroshige, 2017/02/17 23:15:15]

Created https://codereview.chromium.org/2692863013.

     DVLOG(1) << "Not implemented.";  // Dispatch error event.
-    return false;
+    return nullptr;
   }
-  pendingScript->setScriptResource(resource);
-  return true;
+
+  return PendingScript::create(element, resource);
 }
 
 // Implements the initial steps for 'An end tag whose tag name is "script"'
@@ -635,15 +644,18 @@ void HTMLParserScriptRunner::processScriptElementInternal(
         // "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);