De-Element ScriptLoader

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 37 matching lines @@
 #include "public/platform/Platform.h"
 #include <inttypes.h>
 #include <memory>
 
 namespace blink {
 
 namespace {
 
 // TODO(bmcquade): move this to a shared location if we find ourselves wanting
 // to trace similar data elsewhere in the codebase.
-std::unique_ptr<TracedValue> getTraceArgsForScriptElement(
-    Element* element,
+std::unique_ptr<TracedValue> getTraceArgsForScriptLoaderClient(
+    ScriptLoaderClient* client,
     const TextPosition& textPosition) {
   std::unique_ptr<TracedValue> value = TracedValue::create();
-  ScriptLoader* scriptLoader = toScriptLoaderIfPossible(element);
+  ScriptLoader* scriptLoader = client->loader();
   if (scriptLoader && scriptLoader->resource())
     value->setString("url", scriptLoader->resource()->url().getString());
-  if (element->ownerDocument() && element->ownerDocument()->frame()) {
+  if (client->document().frame()) {

[hiroshige, 2017/03/01 00:58:15]

This changes the behavior: ownerDocument() -> document().

[Nate Chapin, 2017/03/01 21:34:59]

Node::document() and Node::ownerDocument() are equivalent if node->document() !=
node. So long as the Element* in the old code was in fact a HTMLScriptElement or
SVGScriptElement, this isn't a behavior change.

[hiroshige, 2017/03/06 22:54:03]

Acknowledged.

     value->setString(
         "frame",
         String::format("0x%" PRIx64,
                        static_cast<uint64_t>(reinterpret_cast<intptr_t>(
-                           element->ownerDocument()->frame()))));
+                           client->document().frame()))));
   }
   if (textPosition.m_line.zeroBasedInt() > 0 ||
       textPosition.m_column.zeroBasedInt() > 0) {
     value->setInteger("lineNumber", textPosition.m_line.oneBasedInt());
     value->setInteger("columnNumber", textPosition.m_column.oneBasedInt());
   }
   return value;
 }
 
-bool doExecuteScript(Element* scriptElement,
+bool doExecuteScript(ScriptLoaderClient* client,
                      const ScriptSourceCode& sourceCode,
                      const TextPosition& textPosition) {
-  ScriptLoader* scriptLoader = toScriptLoaderIfPossible(scriptElement);
+  ScriptLoader* scriptLoader = client->loader();
   DCHECK(scriptLoader);
   TRACE_EVENT_WITH_FLOW1(
-      "blink", "HTMLParserScriptRunner ExecuteScript", scriptElement,
+      "blink", "HTMLParserScriptRunner ExecuteScript", client,

[Nate Chapin, 2017/02/28 21:39:29]

Here and below: these TRACE_EVENT changes are probably wrong. I'll get an expert
involved if the rest of the change seems good.

       TRACE_EVENT_FLAG_FLOW_IN, "data",
-      getTraceArgsForScriptElement(scriptElement, textPosition));
+      getTraceArgsForScriptLoaderClient(client, textPosition));
   return scriptLoader->executeScript(sourceCode);
 }
 
 void traceParserBlockingScript(const PendingScript* pendingScript,
                                bool waitingForResources) {
   // The HTML parser must yield before executing script in the following
   // cases:
   // * the script's execution is blocked on the completed load of the script
   //   resource
   //   (https://html.spec.whatwg.org/multipage/scripting.html#pending-parsing-blocking-script)
   // * the script's execution is blocked on the load of a style sheet or other
   //   resources that are blocking scripts
   //   (https://html.spec.whatwg.org/multipage/semantics.html#a-style-sheet-that-is-blocking-scripts)
   //
   // Both of these cases can introduce significant latency when loading a
   // web page, especially for users on slow connections, since the HTML parser
   // must yield until the blocking resources finish loading.
   //
   // We trace these parser yields here using flow events, so we can track
   // both when these yields occur, as well as how long the parser had
   // to yield. The connecting flow events are traced once the parser becomes
   // unblocked when the script actually executes, in doExecuteScript.
-  Element* element = pendingScript->element();
-  if (!element)
+  ScriptLoaderClient* client = pendingScript->scriptLoaderClient();
+  if (!client)
     return;
   TextPosition scriptStartPosition = pendingScript->startingPosition();
   if (!pendingScript->isReady()) {
     if (waitingForResources) {
       TRACE_EVENT_WITH_FLOW1(
-          "blink", "YieldParserForScriptLoadAndBlockingResources", element,
+          "blink", "YieldParserForScriptLoadAndBlockingResources", client,
           TRACE_EVENT_FLAG_FLOW_OUT, "data",
-          getTraceArgsForScriptElement(element, scriptStartPosition));
+          getTraceArgsForScriptLoaderClient(client, scriptStartPosition));
     } else {
       TRACE_EVENT_WITH_FLOW1(
-          "blink", "YieldParserForScriptLoad", element,
+          "blink", "YieldParserForScriptLoad", client,
           TRACE_EVENT_FLAG_FLOW_OUT, "data",
-          getTraceArgsForScriptElement(element, scriptStartPosition));
+          getTraceArgsForScriptLoaderClient(client, scriptStartPosition));
     }
   } else if (waitingForResources) {
     TRACE_EVENT_WITH_FLOW1(
-        "blink", "YieldParserForScriptBlockingResources", element,
+        "blink", "YieldParserForScriptBlockingResources", client,
         TRACE_EVENT_FLAG_FLOW_OUT, "data",
-        getTraceArgsForScriptElement(element, scriptStartPosition));
+        getTraceArgsForScriptLoaderClient(client, scriptStartPosition));
   }
 }
 
 static KURL documentURLForScriptExecution(Document* document) {
   if (!document)
     return KURL();
 
   if (!document->frame()) {
     if (document->importsController())
       return document->url();
@@ skipping 76 matching lines @@
       // The parser cannot be unblocked as a microtask requested another
       // resource
       if (!m_document->isScriptExecutionReady())
         return;
     }
   }
 
   TextPosition scriptStartPosition = pendingScript->startingPosition();
   double scriptParserBlockingTime =
       pendingScript->parserBlockingLoadStartTime();
-  Element* element = pendingScript->element();
+  ScriptLoaderClient* client = pendingScript->scriptLoaderClient();
 
   // 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)) {
+  if (ScriptLoader* scriptLoader = client->loader()) {
     // 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);
 
     // 8. "Execute the script."
     if (errorOccurred) {
       TRACE_EVENT_WITH_FLOW1(
-          "blink", "HTMLParserScriptRunner ExecuteScriptFailed", element,
+          "blink", "HTMLParserScriptRunner ExecuteScriptFailed", client,
           TRACE_EVENT_FLAG_FLOW_IN, "data",
-          getTraceArgsForScriptElement(element, scriptStartPosition));
+          getTraceArgsForScriptLoaderClient(client, scriptStartPosition));
       scriptLoader->dispatchErrorEvent();
     } else {
       DCHECK(isExecutingScript());
       if (scriptParserBlockingTime > 0.0) {
         DocumentParserTiming::from(*m_document)
             .recordParserBlockedOnScriptLoadDuration(
                 monotonicallyIncreasingTime() - scriptParserBlockingTime,
                 scriptLoader->wasCreatedDuringDocumentWrite());
       }
-      if (!doExecuteScript(element, sourceCode, scriptStartPosition)) {
+      if (!doExecuteScript(client, sourceCode, scriptStartPosition)) {
         scriptLoader->dispatchErrorEvent();
       } else {
-        element->dispatchEvent(Event::create(EventTypeNames::load));
+        client->dispatchLoadEvent();

[hiroshige, 2017/03/01 00:58:15]

Hmm. Conceptually I think we should call scriptLoader->dispatchLoadEvent() here
but it slightly changes the behavior, so keeping this line as-is and modifying
it to call scriptLoader later in another CL is good. (I also found other slight
bugs around dispatchLoadEvent() not related to this CL...)

[Nate Chapin, 2017/03/01 21:34:59]

Yeah, I considered calling scriptLoader->dispatchLoadEvent(), but noticed the
slight behavior change, too.

       }
     }
 
     // 9. "Decrement the parser's script nesting level by one.
     //     If the parser's script nesting level is zero
     //     (which it always should be at this point),
     //     then set the parser pause flag to false."
     // This is implemented by ~ScriptNestingLevelIncrementer().
   }
 
   DCHECK(!isExecutingScript());
 }
 
-void fetchBlockedDocWriteScript(Element* script,
+void fetchBlockedDocWriteScript(ScriptLoaderClient* client,
                                 bool isParserInserted,
                                 const TextPosition& scriptStartPosition) {
-  DCHECK(script);
+  DCHECK(client);
 
   ScriptLoader* scriptLoader =
-      ScriptLoader::create(script, isParserInserted, false, false);
+      ScriptLoader::create(client, isParserInserted, false, false);
   DCHECK(scriptLoader);
   scriptLoader->setFetchDocWrittenScriptDeferIdle();
   scriptLoader->prepareScript(scriptStartPosition);
 }
 
 void emitWarningForDocWriteScripts(const String& url, Document& document) {
   String message =
       "The Parser-blocking, cross site (i.e. different eTLD+1) "
       "script, " +
       url +
@@ skipping 21 matching lines @@
     PendingScript* pendingScript) {
   // If the script was blocked as part of document.write intervention,
   // then send an asynchronous GET request with an interventions header.
 
   if (!parserBlockingScript())
     return;
 
   if (parserBlockingScript() != pendingScript)
     return;
 
-  Element* element = parserBlockingScript()->element();
+  ScriptLoaderClient* client = parserBlockingScript()->scriptLoaderClient();
 
-  ScriptLoader* scriptLoader = toScriptLoaderIfPossible(element);
+  ScriptLoader* scriptLoader = client->loader();
   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);
   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());
-  fetchBlockedDocWriteScript(element, isParserInserted, startingPosition);
+  fetchBlockedDocWriteScript(client, isParserInserted, startingPosition);
 }
 
 void HTMLParserScriptRunner::pendingScriptFinished(
     PendingScript* pendingScript) {
   // Handle cancellations of parser-blocking script loads without
   // notifying the host (i.e., parser) if these were initiated by nested
   // document.write()s. The cancellation may have been triggered by
   // script execution to signal an abrupt stop (e.g., window.close().)
   //
   // The parser is unprepared to be told, and doesn't need to be.
@@ skipping 10 matching lines @@
 }
 
 // 'An end tag whose tag name is "script"'
 // https://html.spec.whatwg.org/#scriptEndTag
 //
 // Script handling lives outside the tree builder to keep each class simple.
 void HTMLParserScriptRunner::processScriptElement(
     Element* scriptElement,
     const TextPosition& scriptStartPosition) {
   DCHECK(scriptElement);
-  TRACE_EVENT1(
-      "blink", "HTMLParserScriptRunner::execute", "data",
-      getTraceArgsForScriptElement(scriptElement, scriptStartPosition));
+
   // FIXME: If scripting is disabled, always just return.
 
   bool hadPreloadScanner = m_host->hasPreloadScanner();
 
   // Initial steps of 'An end tag whose tag name is "script"'.
   // Try to execute the script given to us.
   processScriptElementInternal(scriptElement, scriptStartPosition);
 
   // "At this stage, if there is a pending parsing-blocking script, then:"
   if (hasParserBlockingScript()) {
@@ skipping 176 matching lines @@
   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);
 }
 
 PendingScript* HTMLParserScriptRunner::requestPendingScript(
     Element* element) const {
-  ScriptResource* resource = toScriptLoaderIfPossible(element)->resource();
+  ScriptLoaderClient* client =
+      ScriptLoaderClient::fromElementIfPossible(element);

[hiroshige, 2017/03/01 00:58:15]

Probably we can remove fromElementIfPossible() call here by changing
the argument type of requestPendingScript() (and some callers of it) to
ScriptLoader*.

[Nate Chapin, 2017/03/01 21:35:00]

Possibly, but that requires changing PendingScript's constructor, too. I'm
inclined to leave that for a later PendingScript refactor.

[hiroshige, 2017/03/06 22:54:03]

Acknowledged.

+  ScriptResource* resource = client->loader()->resource();
   // Here |resource| should be non-null. If it were nullptr,
   // ScriptLoader::fetchScript() should have returned false and
   // thus the control shouldn't have reached here.
   CHECK(resource);
-  return PendingScript::create(element, resource);
+  return PendingScript::create(client, resource);
 }
 
 // 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());
   {
-    ScriptLoader* scriptLoader = toScriptLoaderIfPossible(script);
+    ScriptLoaderClient* client =
+        ScriptLoaderClient::fromElementIfPossible(script);
+    if (!client)

[hiroshige, 2017/03/01 00:58:15]

Shouldn't |client| always non-null because we expect |script| to be
HTMLScriptElement?

[Nate Chapin, 2017/03/01 21:34:59]

It should be, I was just copying the pattern of the early-exit below. But it
looks like that bug is pretty old, and it's quiet possibly bitrotted. Will
DCHECK instead of early exit.

+      return;
+    ScriptLoader* scriptLoader = client->loader();
 
     // This contains both a DCHECK and a null check since we should not
     // be getting into the case of a null script element, but seem to be from
     // time to time. The assertion is left in to help find those cases and
     // is being tracked by <https://bugs.webkit.org/show_bug.cgi?id=60559>.
     DCHECK(scriptLoader);
     if (!scriptLoader)
       return;
 
+    TRACE_EVENT1(
+        "blink", "HTMLParserScriptRunner::execute", "data",
+        getTraceArgsForScriptLoaderClient(client, scriptStartPosition));
     DCHECK(scriptLoader->isParserInserted());
 
     if (!isExecutingScript())
       Microtask::performCheckpoint(V8PerIsolateData::mainThreadIsolate());
 
     // "Let the old insertion point have the same value as the current
     //  insertion point.
     //  Let the insertion point be just before the next input character."
     InsertionPointRecord insertionPointRecord(m_host->inputStream());
 
@@ skipping 18 matching lines @@
     } 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.)"
         CHECK(!m_parserBlockingScript);
         m_parserBlockingScript =
-            PendingScript::create(script, scriptStartPosition);
+            PendingScript::create(client, 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);
         if (m_parserBlockingScript)
           m_parserBlockingScript->dispose();
         m_parserBlockingScript = nullptr;
         ScriptSourceCode sourceCode(script->textContent(),
                                     documentURLForScriptExecution(m_document),
                                     scriptStartPosition);
-        doExecuteScript(script, sourceCode, scriptStartPosition);
+        doExecuteScript(client, 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 ~ScriptNestingLevelIncrementer().
 
     // "Let the insertion point have the value of the old insertion point."
     // Implemented by ~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