Refactor Script(Loader|Runner): don't access Resources all over the place...

... but use proper layering instead.

PendingScript holds a Resource, and many places in the code are using both the
PendingScript and the underlying Resource. For example, ScriptRunner holds a
collection of PendingScripts, and ScriptLoaders then listen to the underlying
Resources. This is confusing and error prone.

The Resource - ScriptLoader - PendingScript - Element love quadrangle is messy
in general.  Both ScriptLoader and PendingScript refer to the Resource. The way
to get ScriptLoader from PendingScript is via Element which PendingScript also
holds. But there's no way to get PendingScript from ScriptLoader.

This CL makes ScriptRunner access the ScriptLoaders directly and moves the
PendingScript to ScriptLoader. In addition, it makes ScriptRunner not access the
Resource of the ScriptLoader directly.

This CL also cancels the hack from https://bugs.webkit.org/show_bug.cgi?id=92211
(r139942) where it was speculated that a crash can be caused by ScriptLoader
getting the notifyFinished notification twice because it doesn't unsubscribe yet
when it gets the first one. The speculative fix was to made it nullify
ScriptLoader::m_resource and check for the nullity later. This is pretty
surprising, since the obvious solution would've been to just unsubscribe early
enough (which is done by this CL).

Note a behavioral change: It's OK to call Resource::addClient even if the
Resource is finished, and in that case, the client will be notified
immediately. But PendingScript used to ASSERT that the Resource is not
finished. This CL removes that restriction (the underlying Resource already
handles adding clients to finished resources + made ScriptStreamer handle it
too).

Review URL: https://codereview.chromium.org/656113002

git-svn-id: svn://svn.chromium.org/blink/trunk@183873 bbb929c8-8fbe-4397-9dbb-9b2b20218538

third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.h

@@ -66,8 +66,8 @@ public:
     void addClient(ScriptResourceClient* client)
     {
         ASSERT(!m_client);
-        ASSERT(!isFinished());
         m_client = client;
+        notifyFinishedToClient();
     }
 
     void removeClient(ScriptResourceClient* client)

third_party/WebKit/Source/core/dom/PendingScript.cpp

@@ -40,13 +40,12 @@ PendingScript::~PendingScript()
 void PendingScript::watchForLoad(ScriptResourceClient* client)
 {
     ASSERT(!m_watchingForLoad);
-    ASSERT(!isReady());
+    // addClient() will call notifyFinished() if the load is complete. Callers
+    // who do not expect to be re-entered from this call should not call
+    // watchForLoad for a PendingScript which isReady.
     if (m_streamer) {
         m_streamer->addClient(client);
     } else {
-        // addClient() will call notifyFinished() if the load is
-        // complete. Callers do not expect to be re-entered from this call, so
-        // they should not become a client of an already-loaded Resource.
         resource()->addClient(client);
     }
     m_watchingForLoad = true;

third_party/WebKit/Source/core/dom/ScriptLoader.cpp

@@ -75,7 +75,7 @@ ScriptLoader::ScriptLoader(Element* element, bool parserInserted, bool alreadySt
 
 ScriptLoader::~ScriptLoader()
 {
-    stopLoadRequest();
+    m_pendingScript.stopWatchingForLoad(this);
 }
 
 void ScriptLoader::didNotifySubtreeInsertionsToDocument()
@@ -235,11 +235,15 @@ bool ScriptLoader::prepareScript(const TextPosition& scriptStartPosition, Legacy
         m_readyToBeParserExecuted = true;
     } else if (client->hasSourceAttribute() && !client->asyncAttributeValue() && !m_forceAsync) {
         m_willExecuteInOrder = true;
-        contextDocument->scriptRunner()->queueScriptForExecution(this, m_resource, ScriptRunner::IN_ORDER_EXECUTION);
-        m_resource->addClient(this);
+        m_pendingScript = PendingScript(m_element, m_resource.get());
+        contextDocument->scriptRunner()->queueScriptForExecution(this, ScriptRunner::IN_ORDER_EXECUTION);
+        // Note that watchForLoad can immediately call notifyFinished.
+        m_pendingScript.watchForLoad(this);
     } else if (client->hasSourceAttribute()) {
-        contextDocument->scriptRunner()->queueScriptForExecution(this, m_resource, ScriptRunner::ASYNC_EXECUTION);
-        m_resource->addClient(this);
+        m_pendingScript = PendingScript(m_element, m_resource.get());
+        contextDocument->scriptRunner()->queueScriptForExecution(this, ScriptRunner::ASYNC_EXECUTION);
+        // Note that watchForLoad can immediately call notifyFinished.
+        m_pendingScript.watchForLoad(this);
     } else {
         // Reset line numbering for nested writes.
         TextPosition position = elementDocument.isInDocumentWrite() ? TextPosition() : scriptStartPosition;
@@ -356,26 +360,20 @@ void ScriptLoader::executeScript(const ScriptSourceCode& sourceCode, double* com
     }
 }
 
-void ScriptLoader::stopLoadRequest()
-{
-    if (m_resource) {
-        if (!m_willBeParserExecuted)
-            m_resource->removeClient(this);
-        m_resource = 0;
-    }
-}
-
-void ScriptLoader::execute(ScriptResource* resource)
+void ScriptLoader::execute()
 {
     ASSERT(!m_willBeParserExecuted);
-    ASSERT(resource);
-    if (resource->errorOccurred()) {
+    ASSERT(m_pendingScript.resource());
+    bool errorOccurred = false;
+    ScriptSourceCode source = m_pendingScript.getSource(KURL(), errorOccurred);
+    RefPtr<Element> element = m_pendingScript.releaseElementAndClear();
+    if (errorOccurred) {
         dispatchErrorEvent();
-    } else if (!resource->wasCanceled()) {
-        executeScript(ScriptSourceCode(resource));
+    } else if (!m_resource->wasCanceled()) {
+        executeScript(source);
         dispatchLoadEvent();
     }
-    resource->removeClient(this);
+    m_resource = 0;
 }
 
 void ScriptLoader::notifyFinished(Resource* resource)
@@ -387,13 +385,8 @@ void ScriptLoader::notifyFinished(Resource* resource)
     if (!contextDocument)
         return;
 
-    // Resource possibly invokes this notifyFinished() more than
-    // once because ScriptLoader doesn't unsubscribe itself from
-    // Resource here and does it in execute() instead.
-    // We use m_resource to check if this function is already called.
     ASSERT_UNUSED(resource, resource == m_resource);
-    if (!m_resource)
-        return;
+
     if (m_resource->errorOccurred()) {
         dispatchErrorEvent();
         contextDocument->scriptRunner()->notifyScriptLoadError(this, m_willExecuteInOrder ? ScriptRunner::IN_ORDER_EXECUTION : ScriptRunner::ASYNC_EXECUTION);
@@ -404,7 +397,7 @@ void ScriptLoader::notifyFinished(Resource* resource)
     else
         contextDocument->scriptRunner()->notifyScriptReady(this, ScriptRunner::ASYNC_EXECUTION);
 
-    m_resource = 0;
+    m_pendingScript.stopWatchingForLoad(this);
 }
 
 bool ScriptLoader::ignoresLoadRequest() const

third_party/WebKit/Source/core/dom/ScriptLoader.h

@@ -21,6 +21,7 @@
 #ifndef ScriptLoader_h
 #define ScriptLoader_h
 
+#include "core/dom/PendingScript.h"
 #include "core/fetch/FetchRequest.h"
 #include "core/fetch/ResourceClient.h"
 #include "core/fetch/ResourcePtr.h"
@@ -48,7 +49,7 @@ public:
     String scriptCharset() const { return m_characterEncoding; }
     String scriptContent() const;
     void executeScript(const ScriptSourceCode&, double* compilationFinishTime = 0);
-    void execute(ScriptResource*);
+    void execute();
 
     // XML parser calls these
     void dispatchLoadEvent();
@@ -72,6 +73,8 @@ public:
     void handleSourceAttribute(const String& sourceUrl);
     void handleAsyncAttribute();
 
+    bool isReady() const { return m_pendingScript.isReady(); }
+
 private:
     ScriptLoader(Element*, bool createdByParser, bool isEvaluated);
 
@@ -79,7 +82,6 @@ private:
     bool isScriptForEventSupported() const;
 
     bool fetchScript(const String& sourceUrl, FetchRequest::DeferOption);
-    void stopLoadRequest();
 
     ScriptLoaderClient* client() const;
 
@@ -101,6 +103,8 @@ private:
     bool m_willExecuteInOrder : 1;
     String m_characterEncoding;
     String m_fallbackCharacterEncoding;
+
+    PendingScript m_pendingScript;
 };
 
 ScriptLoader* toScriptLoaderIfPossible(Element*);

third_party/WebKit/Source/core/dom/ScriptRunner.cpp

@@ -28,9 +28,7 @@
 
 #include "core/dom/Document.h"
 #include "core/dom/Element.h"
-#include "core/dom/PendingScript.h"
 #include "core/dom/ScriptLoader.h"
-#include "core/fetch/ScriptResource.h"
 #include "platform/heap/Handle.h"
 
 namespace blink {
@@ -46,29 +44,24 @@ ScriptRunner::~ScriptRunner()
 {
 }
 
-void ScriptRunner::addPendingAsyncScript(ScriptLoader* scriptLoader, const PendingScript& pendingScript)
+void ScriptRunner::addPendingAsyncScript(ScriptLoader* scriptLoader)
 {
     m_document->incrementLoadEventDelayCount();
-    m_pendingAsyncScripts.add(scriptLoader, pendingScript);
+    m_pendingAsyncScripts.add(scriptLoader);
 }
 
-void ScriptRunner::queueScriptForExecution(ScriptLoader* scriptLoader, ResourcePtr<ScriptResource> resource, ExecutionType executionType)
+void ScriptRunner::queueScriptForExecution(ScriptLoader* scriptLoader, ExecutionType executionType)
 {
     ASSERT(scriptLoader);
-    ASSERT(resource.get());
-
-    Element* element = scriptLoader->element();
-    ASSERT(element);
-    ASSERT(element->inDocument());
 
     switch (executionType) {
     case ASYNC_EXECUTION:
-        addPendingAsyncScript(scriptLoader, PendingScript(element, resource.get()));
+        addPendingAsyncScript(scriptLoader);
         break;
 
     case IN_ORDER_EXECUTION:
         m_document->incrementLoadEventDelayCount();
-        m_scriptsToExecuteInOrder.append(PendingScript(element, resource.get()));
+        m_scriptsToExecuteInOrder.append(scriptLoader);
         break;
     }
 }
@@ -89,7 +82,8 @@ void ScriptRunner::notifyScriptReady(ScriptLoader* scriptLoader, ExecutionType e
     switch (executionType) {
     case ASYNC_EXECUTION:
         ASSERT(m_pendingAsyncScripts.contains(scriptLoader));
-        m_scriptsToExecuteSoon.append(m_pendingAsyncScripts.take(scriptLoader));
+        m_scriptsToExecuteSoon.append(scriptLoader);
+        m_pendingAsyncScripts.remove(scriptLoader);
         break;
 
     case IN_ORDER_EXECUTION:
@@ -117,7 +111,8 @@ void ScriptRunner::notifyScriptLoadError(ScriptLoader* scriptLoader, ExecutionTy
 void ScriptRunner::movePendingAsyncScript(ScriptRunner* newRunner, ScriptLoader* scriptLoader)
 {
     if (m_pendingAsyncScripts.contains(scriptLoader)) {
-        newRunner->addPendingAsyncScript(scriptLoader, m_pendingAsyncScripts.take(scriptLoader));
+        newRunner->addPendingAsyncScript(scriptLoader);
+        m_pendingAsyncScripts.remove(scriptLoader);
         m_document->decrementLoadEventDelayCount();
     }
 }
@@ -128,20 +123,18 @@ void ScriptRunner::timerFired(Timer<ScriptRunner>* timer)
 
     RefPtrWillBeRawPtr<Document> protect(m_document.get());
 
-    Vector<PendingScript> scripts;
-    scripts.swap(m_scriptsToExecuteSoon);
+    Vector<ScriptLoader*> scriptLoaders;
+    scriptLoaders.swap(m_scriptsToExecuteSoon);
 
     size_t numInOrderScriptsToExecute = 0;
-    for (; numInOrderScriptsToExecute < m_scriptsToExecuteInOrder.size() && m_scriptsToExecuteInOrder[numInOrderScriptsToExecute].resource()->isLoaded(); ++numInOrderScriptsToExecute)
-        scripts.append(m_scriptsToExecuteInOrder[numInOrderScriptsToExecute]);
+    for (; numInOrderScriptsToExecute < m_scriptsToExecuteInOrder.size() && m_scriptsToExecuteInOrder[numInOrderScriptsToExecute]->isReady(); ++numInOrderScriptsToExecute)
+        scriptLoaders.append(m_scriptsToExecuteInOrder[numInOrderScriptsToExecute]);
     if (numInOrderScriptsToExecute)
         m_scriptsToExecuteInOrder.remove(0, numInOrderScriptsToExecute);
 
-    size_t size = scripts.size();
+    size_t size = scriptLoaders.size();
     for (size_t i = 0; i < size; ++i) {
-        ScriptResource* resource = scripts[i].resource();
-        RefPtrWillBeRawPtr<Element> element = scripts[i].releaseElementAndClear();
-        toScriptLoaderIfPossible(element.get())->execute(resource);
+        scriptLoaders[i]->execute();
         m_document->decrementLoadEventDelayCount();
     }
 }

third_party/WebKit/Source/core/dom/ScriptRunner.h

@@ -38,7 +38,6 @@ namespace blink {
 
 class ScriptResource;
 class Document;
-class PendingScript;
 class ScriptLoader;
 
 class ScriptRunner final : public NoBaseWillBeGarbageCollectedFinalized<ScriptRunner> {
@@ -51,7 +50,7 @@ public:
     ~ScriptRunner();
 
     enum ExecutionType { ASYNC_EXECUTION, IN_ORDER_EXECUTION };
-    void queueScriptForExecution(ScriptLoader*, ResourcePtr<ScriptResource>, ExecutionType);
+    void queueScriptForExecution(ScriptLoader*, ExecutionType);
     bool hasPendingScripts() const { return !m_scriptsToExecuteSoon.isEmpty() || !m_scriptsToExecuteInOrder.isEmpty() || !m_pendingAsyncScripts.isEmpty(); }
     void suspend();
     void resume();
@@ -67,14 +66,12 @@ private:
 
     void timerFired(Timer<ScriptRunner>*);
 
-    void addPendingAsyncScript(ScriptLoader*, const PendingScript&);
+    void addPendingAsyncScript(ScriptLoader*);
 
     RawPtrWillBeMember<Document> m_document;
-    // FIXME: Oilpan: consider using heap vectors and hash map here;
-    // PendingScript does have a (trivial) destructor, however.
-    Vector<PendingScript> m_scriptsToExecuteInOrder;
-    Vector<PendingScript> m_scriptsToExecuteSoon; // http://www.whatwg.org/specs/web-apps/current-work/#set-of-scripts-that-will-execute-as-soon-as-possible
-    WillBeHeapHashMap<ScriptLoader*, PendingScript> m_pendingAsyncScripts;
+    Vector<ScriptLoader*> m_scriptsToExecuteInOrder;
+    Vector<ScriptLoader*> m_scriptsToExecuteSoon; // http://www.whatwg.org/specs/web-apps/current-work/#set-of-scripts-that-will-execute-as-soon-as-possible
+    HashSet<ScriptLoader*> m_pendingAsyncScripts;
     Timer<ScriptRunner> m_timer;
 };