Do not re-initialize PendingScript in HTMLParserScriptRunner

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

 /*
  * Copyright (C) 2010 Google, Inc. All Rights Reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 139 matching lines @@
 
 using namespace HTMLNames;
 
 HTMLParserScriptRunner::HTMLParserScriptRunner(
     HTMLParserReentryPermit* reentryPermit,
     Document* document,
     HTMLParserScriptRunnerHost* host)
     : 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);
 }
 
 HTMLParserScriptRunner::~HTMLParserScriptRunner() {
   // Verify that detach() has been called.
   DCHECK(!m_document);
 }
 
 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();
     pendingScript->dispose();
   }
   m_document = nullptr;
   // m_reentryPermit is not cleared here, because the script runner
   // may continue to run pending scripts after the parser has
   // detached.
 }
 
 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:
 // - When called from executeParsingBlockingScripts(), this corresponds to some
 //   steps of the "Otherwise" Clause of 'An end tag whose tag name is "script"'
 //   https://html.spec.whatwg.org/#scriptEndTag
 // - When called from executeScriptsWaitingForParsing(), this corresponds
 //   https://html.spec.whatwg.org/#execute-the-script-block
 //   and thus currently this function does more than specced.
 // TODO(hiroshige): Make the spec and implementation consistent.
@@ skipping 22 matching lines @@
 
   TextPosition scriptStartPosition = pendingScript->startingPosition();
   double scriptParserBlockingTime =
       pendingScript->parserBlockingLoadStartTime();
   Element* element = pendingScript->element();
 
   // 1. "Let the script be the pending parsing-blocking script.
   //     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
     //     zero before this step, so this sets it to one)."
     HTMLParserReentryPermit::ScriptNestingLevelIncrementer
         nestingLevelIncrementer =
             m_reentryPermit->incrementScriptNestingLevel();
 
     IgnoreDestructiveWriteCountIncrementer
         ignoreDestructiveWriteCountIncrementer(m_document);
@@ skipping 64 matching lines @@
       "network connectivity. ";
   document.addConsoleMessage(
       ConsoleMessage::create(JSMessageSource, ErrorMessageLevel, message));
   WTFLogAlways("%s", message.utf8().data());
 }
 
 void HTMLParserScriptRunner::possiblyFetchBlockedDocWriteScript(
     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;

[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;
 
   if (!pendingScript->errorOccurred()) {
     emitWarningForDocWriteScripts(pendingScript->resource()->url().getString(),
                                   *m_document);
     return;
   }
 
   // Due to dependency violation, not able to check the exact error to be
   // ERR_CACHE_MISS but other errors are rare with
   // WebCachePolicy::ReturnCacheDataDontLoad.
 
   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.
   memoryCache()->remove(pendingScript->resource());
   fetchBlockedDocWriteScript(element, isParserInserted, startingPosition);
 }
 
 void HTMLParserScriptRunner::pendingScriptFinished(
     PendingScript* pendingScript) {
   // Handle cancellations of parser-blocking script loads without
@@ skipping 42 matching lines @@
       // "outer" tree construction stage.)"
       // TODO(hiroshige): set the parser pause flag to true here.
 
       // Unwind to the outermost HTMLParserScriptRunner::processScriptElement
       // before continuing parsing.
       return;
     }
 
     // - "Otherwise":
 
-    traceParserBlockingScript(m_parserBlockingScript.get(),
+    traceParserBlockingScript(parserBlockingScript(),
                               !m_document->isScriptExecutionReady());
     m_parserBlockingScript->markParserBlockingLoadStartTime();
 
     // If preload scanner got created, it is missing the source after the
     // current insertion point. Append it and scan.
     if (!hadPreloadScanner && m_host->hasPreloadScanner())
       m_host->appendCurrentInputStreamToPreloadScannerAndScan();
 
     executeParsingBlockingScripts();
   }
 }
 
 bool HTMLParserScriptRunner::hasParserBlockingScript() const {
-  return !!m_parserBlockingScript->element();
+  return parserBlockingScript();
 }
 
 // Implements the "Otherwise" Clause of 'An end tag whose tag name is "script"'
 // https://html.spec.whatwg.org/#scriptEndTag
 void HTMLParserScriptRunner::executeParsingBlockingScripts() {
   // 3. "If (1) the parser's Document has a style sheet that is blocking scripts
   //     or (2) the script's "ready to be parser-executed" flag is not set:
   //     spin the event loop
   //     until the parser's Document has no style sheet that is blocking scripts
   //     and the script's "ready to be parser-executed" flag is set."
   // These conditions correspond to isParserBlockingScriptReady() and
   // if it is false, executeParsingBlockingScripts() will be called later
   // when isParserBlockingScriptReady() becomes true:
   // (1) from HTMLParserScriptRunner::executeScriptsWaitingForResources(), or
   // (2) from HTMLParserScriptRunner::executeScriptsWaitingForLoad().
   while (hasParserBlockingScript() && isParserBlockingScriptReady()) {
     DCHECK(m_document);
     DCHECK(!isExecutingScript());
     DCHECK(m_document->isScriptExecutionReady());
 
     // 6. "Let the insertion point be just before the next input character."
     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."
     // Implemented as scope out of InsertionPointRecord.
 
     // 11. "If there is once again a pending parsing-blocking script, then
     //      repeat these steps from step 1."
   }
 }
 
 void HTMLParserScriptRunner::executeScriptsWaitingForLoad(
     PendingScript* pendingScript) {
   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();
 }
 
 void HTMLParserScriptRunner::executeScriptsWaitingForResources() {
   TRACE_EVENT0("blink",
                "HTMLParserScriptRunner::executeScriptsWaitingForResources");
   DCHECK(m_document);
   DCHECK(!isExecutingScript());
   DCHECK(m_document->isScriptExecutionReady());
   executeParsingBlockingScripts();
@@ skipping 42 matching lines @@
     //     from substep 1."
   }
   return true;
 }
 
 // 2nd Clause, Step 23 of https://html.spec.whatwg.org/#prepare-a-script
 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));
       }
     }
 
     m_parserBlockingScript->watchForLoad(this);
   }
 }
 
 // 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()) {
     ScriptState* scriptState = ScriptState::forMainWorld(m_document->frame());
     if (scriptState) {
       ScriptStreamer::startStreaming(
           pendingScript, ScriptStreamer::Deferred,
           m_document->frame()->settings(), scriptState,
           TaskRunnerHelper::get(TaskType::Networking, m_document));
     }
   }
 
   DCHECK(pendingScript->resource());
 
   // "Add the element to the end of the list of scripts that will execute
   //  when the document has finished parsing associated with the Document
   //  of the parser that created the 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"'
 // https://html.spec.whatwg.org/#scriptEndTag
 void HTMLParserScriptRunner::processScriptElementInternal(
     Element* script,
     const TextPosition& scriptStartPosition) {
   DCHECK(m_document);
   DCHECK(!hasParserBlockingScript());
   {
@@ skipping 36 matching lines @@
       // 1st Clause of Step 23.
       requestDeferredScript(script);
     } else if (scriptLoader->readyToBeParserExecuted()) {
       // 5th Clause of Step 23.
       // "If ... it's an HTML parser
       //  whose script nesting level is not greater than one"
       if (m_reentryPermit->scriptNestingLevel() == 1u) {
         // "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);
         doExecuteScript(script, sourceCode, scriptStartPosition);
       }
     } else {
       // 2nd Clause of Step 23.
       requestParsingBlockingScript(script);
     }
 
     // "Decrement the parser's script nesting level by one.
     //  If the parser's script nesting level is zero, then set the parser
     //  pause flag to false."
     // Implemented by scope out of ScriptNestingLevelIncrementer.
 
     // "Let the insertion point have the value of the old insertion point."
     // Implemented by scope out of InsertionPointRecord.
   }
 }
 
 DEFINE_TRACE(HTMLParserScriptRunner) {
   visitor->trace(m_document);
   visitor->trace(m_host);
   visitor->trace(m_parserBlockingScript);
   visitor->trace(m_scriptsToExecuteAfterParsing);
   PendingScriptClient::trace(visitor);
 }
 
 }  // namespace blink