2010-06-25 Tony Gentilcore <tonyg@chromium.org>
authortonyg@chromium.org <tonyg@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 26 Jun 2010 05:27:37 +0000 (05:27 +0000)
committertonyg@chromium.org <tonyg@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 26 Jun 2010 05:27:37 +0000 (05:27 +0000)
        Reviewed by Eric Seidel.

        Make PendingScript hold a CachedResourceClient open for its lifetime
        https://bugs.webkit.org/show_bug.cgi?id=40968

        This replaces the mechanism introduced in r61374 with a simpler
        appraoch for preventing unexpected purges: always keep a client open.
        This approach will allow deferred scripts to add a client after
        the resource may have already been loaded without having to worry about
        the buffer being purged in the meantime.

        No new tests because making a CachedResource purse itself is not
        testable from a LayoutTest.

        * html/HTMLDocumentParser.cpp:
        (WebCore::HTMLDocumentParser::watchForLoad):
        (WebCore::HTMLDocumentParser::notifyFinished):
        * html/HTMLScriptRunner.cpp:
        (WebCore::HTMLScriptRunner::~HTMLScriptRunner):
        (WebCore::HTMLScriptRunner::sourceFromPendingScript):
        (WebCore::HTMLScriptRunner::isPendingScriptReady):
        (WebCore::HTMLScriptRunner::executePendingScript):
        (WebCore::HTMLScriptRunner::watchForLoad):
        (WebCore::HTMLScriptRunner::stopWatchingForLoad):
        (WebCore::HTMLScriptRunner::executeScriptsWaitingForLoad):
        (WebCore::HTMLScriptRunner::requestScript):
        (WebCore::HTMLScriptRunner::PendingScript::~PendingScript):
        (WebCore::HTMLScriptRunner::PendingScript::releaseElementAndClear):
        (WebCore::HTMLScriptRunner::PendingScript::setCachedScript):
        (WebCore::HTMLScriptRunner::PendingScript::cachedScript):
        * html/HTMLScriptRunner.h:
        (WebCore::HTMLScriptRunner::PendingScript::PendingScript):
        (WebCore::HTMLScriptRunner::PendingScript::watchingForLoad):
        (WebCore::HTMLScriptRunner::PendingScript::setWatchingForLoad):
        (WebCore::HTMLScriptRunner::PendingScript::notifyFinished):
        * html/HTMLScriptRunnerHost.h:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@61940 268f45cc-cd09-0410-ab3c-d52691b4dbfc

WebCore/ChangeLog
WebCore/html/HTMLDocumentParser.cpp
WebCore/html/HTMLScriptRunner.cpp
WebCore/html/HTMLScriptRunner.h
WebCore/html/HTMLScriptRunnerHost.h

index 510c93e3e76843c42332e4c5ab3d7869e920770c..1f30ea5e94f1b7bba711746b885806c5ff78e1b3 100644 (file)
@@ -1,3 +1,42 @@
+2010-06-25  Tony Gentilcore  <tonyg@chromium.org>
+
+        Reviewed by Eric Seidel.
+
+        Make PendingScript hold a CachedResourceClient open for its lifetime
+        https://bugs.webkit.org/show_bug.cgi?id=40968
+
+        This replaces the mechanism introduced in r61374 with a simpler
+        appraoch for preventing unexpected purges: always keep a client open.
+        This approach will allow deferred scripts to add a client after
+        the resource may have already been loaded without having to worry about
+        the buffer being purged in the meantime.
+
+        No new tests because making a CachedResource purse itself is not
+        testable from a LayoutTest.
+
+        * html/HTMLDocumentParser.cpp:
+        (WebCore::HTMLDocumentParser::watchForLoad):
+        (WebCore::HTMLDocumentParser::notifyFinished):
+        * html/HTMLScriptRunner.cpp:
+        (WebCore::HTMLScriptRunner::~HTMLScriptRunner):
+        (WebCore::HTMLScriptRunner::sourceFromPendingScript):
+        (WebCore::HTMLScriptRunner::isPendingScriptReady):
+        (WebCore::HTMLScriptRunner::executePendingScript):
+        (WebCore::HTMLScriptRunner::watchForLoad):
+        (WebCore::HTMLScriptRunner::stopWatchingForLoad):
+        (WebCore::HTMLScriptRunner::executeScriptsWaitingForLoad):
+        (WebCore::HTMLScriptRunner::requestScript):
+        (WebCore::HTMLScriptRunner::PendingScript::~PendingScript):
+        (WebCore::HTMLScriptRunner::PendingScript::releaseElementAndClear):
+        (WebCore::HTMLScriptRunner::PendingScript::setCachedScript):
+        (WebCore::HTMLScriptRunner::PendingScript::cachedScript):
+        * html/HTMLScriptRunner.h:
+        (WebCore::HTMLScriptRunner::PendingScript::PendingScript):
+        (WebCore::HTMLScriptRunner::PendingScript::watchingForLoad):
+        (WebCore::HTMLScriptRunner::PendingScript::setWatchingForLoad):
+        (WebCore::HTMLScriptRunner::PendingScript::notifyFinished):
+        * html/HTMLScriptRunnerHost.h:
+
 2010-06-25  Zhenyao Mo  <zmo@google.com>
 
         Reviewed by Dimitri Glazkov.
index 3eb1118ebb19307f0c0e361e39a141a590971bfb..9ce22abc6125443857ce42e545850e8c2d699ba8 100644 (file)
@@ -340,6 +340,10 @@ void HTMLDocumentParser::resumeParsingAfterScriptExecution()
 
 void HTMLDocumentParser::watchForLoad(CachedResource* cachedScript)
 {
+    ASSERT(!cachedScript->isLoaded());
+    // addClient would call notifyFinished if the load were complete.
+    // Callers do not expect to be re-entered from this call, so they should
+    // not an already-loaded CachedResource.
     cachedScript->addClient(this);
 }
 
@@ -358,14 +362,6 @@ bool HTMLDocumentParser::shouldLoadExternalScriptFromSrc(const AtomicString& src
 void HTMLDocumentParser::notifyFinished(CachedResource* cachedResource)
 {
     ASSERT(m_scriptRunner);
-    // Ignore calls unless we have a script blocking the parser waiting
-    // for its own load.  Otherwise this may be a load callback from
-    // CachedResource::addClient because the script was already in the cache.
-    // HTMLScriptRunner may not be ready to handle running that script yet.
-    if (!m_scriptRunner->hasScriptsWaitingForLoad()) {
-        ASSERT(m_scriptRunner->inScriptExecution());
-        return;
-    }
     ASSERT(!inScriptExecution());
     ASSERT(m_treeBuilder->isPaused());
     // Note: We only ever wait on one script at a time, so we always know this
index ab3544996cd4c81bb4ddf6d31df8012b0d8b3794..29790c056bd9459cac07e357c9c80b4d90434c76 100644 (file)
@@ -74,7 +74,7 @@ HTMLScriptRunner::HTMLScriptRunner(Document* document, HTMLScriptRunnerHost* hos
 HTMLScriptRunner::~HTMLScriptRunner()
 {
     // FIXME: Should we be passed a "done loading/parsing" callback sooner than destruction?
-    if (m_parsingBlockingScript.cachedScript && m_parsingBlockingScript.watchingForLoad())
+    if (m_parsingBlockingScript.cachedScript() && m_parsingBlockingScript.watchingForLoad())
         stopWatchingForLoad(m_parsingBlockingScript);
 }
 
@@ -99,10 +99,10 @@ inline PassRefPtr<Event> createScriptErrorEvent()
 
 ScriptSourceCode HTMLScriptRunner::sourceFromPendingScript(const PendingScript& script, bool& errorOccurred)
 {
-    if (script.cachedScript) {
-        errorOccurred = script.cachedScript->errorOccurred();
-        ASSERT(script.cachedScript->isLoaded());
-        return ScriptSourceCode(script.cachedScript.get());
+    if (script.cachedScript()) {
+        errorOccurred = script.cachedScript()->errorOccurred();
+        ASSERT(script.cachedScript()->isLoaded());
+        return ScriptSourceCode(script.cachedScript());
     }
     errorOccurred = false;
     return ScriptSourceCode(script.element->textContent(), documentURLForScriptExecution(m_document), script.startingLineNumber);
@@ -113,7 +113,7 @@ bool HTMLScriptRunner::isPendingScriptReady(const PendingScript& script)
     m_hasScriptsWaitingForStylesheets = !m_document->haveStylesheetsLoaded();
     if (m_hasScriptsWaitingForStylesheets)
         return false;
-    if (script.cachedScript && !script.cachedScript->isLoaded())
+    if (script.cachedScript() && !script.cachedScript()->isLoaded())
         return false;
     return true;
 }
@@ -127,12 +127,11 @@ void HTMLScriptRunner::executePendingScript()
     ScriptSourceCode sourceCode = sourceFromPendingScript(m_parsingBlockingScript, errorOccurred);
 
     // Stop watching loads before executeScript to prevent recursion if the script reloads itself.
-    if (m_parsingBlockingScript.cachedScript && m_parsingBlockingScript.watchingForLoad())
+    if (m_parsingBlockingScript.cachedScript() && m_parsingBlockingScript.watchingForLoad())
         stopWatchingForLoad(m_parsingBlockingScript);
 
     // Clear the pending script before possible rentrancy from executeScript()
-    RefPtr<Element> scriptElement = m_parsingBlockingScript.element.release();
-    m_parsingBlockingScript = PendingScript();
+    RefPtr<Element> scriptElement = m_parsingBlockingScript.releaseElementAndClear();
     {
         NestScript nestingLevel(m_scriptNestingLevel, m_host->inputStream());
         if (errorOccurred)
@@ -161,30 +160,18 @@ void HTMLScriptRunner::executeScript(Element* element, const ScriptSourceCode& s
     m_document->frame()->script()->executeScript(sourceCode);
 }
 
-bool HTMLScriptRunner::hasScriptsWaitingForLoad() const
-{
-    // We're only actually waiting for a load.  This allows us to ignore load
-    // callbacks when CachedResource::addClient calls notifyFinished because
-    // of a cache hit (not because of a load we were set up to wait for).
-    return m_parsingBlockingScript.watchingForLoadState == PendingScript::WatchingForLoad;
-}
-
 void HTMLScriptRunner::watchForLoad(PendingScript& pendingScript)
 {
     ASSERT(!pendingScript.watchingForLoad());
-    // CachedResource::addClient will call notifyFinished if the load is already
-    // complete.  We set watchingForLoadState to RegisteringForWatch so that we
-    // know to ignore any notifyFinished call during addClient.
-    pendingScript.watchingForLoadState = PendingScript::RegisteringForWatch;
-    m_host->watchForLoad(pendingScript.cachedScript.get());
-    pendingScript.watchingForLoadState = PendingScript::WatchingForLoad;
+    m_host->watchForLoad(pendingScript.cachedScript());
+    pendingScript.setWatchingForLoad(true);
 }
 
 void HTMLScriptRunner::stopWatchingForLoad(PendingScript& pendingScript)
 {
     ASSERT(pendingScript.watchingForLoad());
-    m_host->stopWatchingForLoad(pendingScript.cachedScript.get());
-    pendingScript.watchingForLoadState = PendingScript::NotWatchingForLoad;
+    m_host->stopWatchingForLoad(pendingScript.cachedScript());
+    pendingScript.setWatchingForLoad(false);
 }
 
 // This function should match 10.2.5.11 "An end tag whose tag name is 'script'"
@@ -224,14 +211,10 @@ bool HTMLScriptRunner::executeParsingBlockingScripts()
 
 bool HTMLScriptRunner::executeScriptsWaitingForLoad(CachedResource* cachedScript)
 {
-    // Callers should check hasScriptsWaitingForLoad() before calling
-    // to prevent parser or script re-entry during due to
-    // CachedResource::addClient calling notifyFinished on cache-hits.
-    ASSERT(hasScriptsWaitingForLoad());
     ASSERT(!m_scriptNestingLevel);
     ASSERT(haveParsingBlockingScript());
-    ASSERT_UNUSED(cachedScript, m_parsingBlockingScript.cachedScript == cachedScript);
-    ASSERT(m_parsingBlockingScript.cachedScript->isLoaded());
+    ASSERT_UNUSED(cachedScript, m_parsingBlockingScript.cachedScript() == cachedScript);
+    ASSERT(m_parsingBlockingScript.cachedScript()->isLoaded());
     return executeParsingBlockingScripts();
 }
 
@@ -262,16 +245,14 @@ void HTMLScriptRunner::requestScript(Element* script)
         notImplemented(); // Dispatch error event.
         return;
     }
-    m_parsingBlockingScript.cachedScript = cachedScript;
-
-    // Always call watchForLoad, even if the script is already loaded.
-    // CachedResource may purge its data if it has no clients, which would cause
-    // later script execution to fail.  watchForLoad sets m_parsingBlockingScript
-    // to the RegisteringForWatch state so we know to ignore any
-    // executeScriptsWaitingForLoad callbacks during the watchForLoad call.
-    watchForLoad(m_parsingBlockingScript);
-    // Callers will attempt to run the m_parsingBlockingScript if possible
-    // before returning control to the parser.
+
+    m_parsingBlockingScript.setCachedScript(cachedScript);
+
+    // We only care about a load callback if cachedScript is not already
+    // in the cache.  Callers will attempt to run the m_parsingBlockingScript
+    // if possible before returning control to the parser.
+    if (!m_parsingBlockingScript.cachedScript()->isLoaded())
+        watchForLoad(m_parsingBlockingScript);
 }
 
 // This method is meant to match the HTML5 definition of "running a script"
@@ -297,4 +278,34 @@ void HTMLScriptRunner::runScript(Element* script, int startingLineNumber)
     }
 }
 
+HTMLScriptRunner::PendingScript::~PendingScript()
+{
+    if (m_cachedScript)
+        m_cachedScript->removeClient(this);
+}
+
+PassRefPtr<Element> HTMLScriptRunner::PendingScript::releaseElementAndClear()
+{
+    setCachedScript(0);
+    startingLineNumber = 0;
+    m_watchingForLoad = false;
+    return element.release();
+}
+
+void HTMLScriptRunner::PendingScript::setCachedScript(CachedScript* cachedScript)
+{
+    if (m_cachedScript == cachedScript)
+        return;
+    if (m_cachedScript)
+        m_cachedScript->removeClient(this);
+    m_cachedScript = cachedScript;
+    if (m_cachedScript)
+        m_cachedScript->addClient(this);
+}
+
+CachedScript* HTMLScriptRunner::PendingScript::cachedScript() const
+{
+    return m_cachedScript.get();
+}
+
 }
index 726000797c498f5d66bde69d02d54b528ac8e2b2..f93ae102b7d28e7215ce798a1daf7d2f4e7f90ad 100644 (file)
@@ -49,42 +49,50 @@ public:
     // Processes the passed in script and any pending scripts if possible.
     bool execute(PassRefPtr<Element> scriptToProcess, int scriptStartLine);
 
-    bool hasScriptsWaitingForLoad() const;
     bool executeScriptsWaitingForLoad(CachedResource*);
-
     bool hasScriptsWaitingForStylesheets() const { return m_hasScriptsWaitingForStylesheets; }
     bool executeScriptsWaitingForStylesheets();
 
     bool inScriptExecution() { return !!m_scriptNestingLevel; }
 
 private:
-    struct PendingScript {
-        // This state controls whether we need to do anything with this script
-        // when we get a executeScriptsWaitingForLoad callback.
-        // We ignore callbacks during RegisteringForWatch.
-        enum WatchingForLoadState {
-            NotWatchingForLoad,
-            RegisteringForWatch,
-            WatchingForLoad,
-        };
-
+    // A container for an external script which may be loaded and executed.
+    //
+    // A CachedResourceHandle alone does not prevent the underlying CachedResource
+    // from purging its data buffer. This class holds a dummy client open for its
+    // lifetime in order to guarantee that the data buffer will not be purged.
+    //
+    // FIXME: Finish turning this into a proper class.
+    class PendingScript : public CachedResourceClient, Noncopyable {
+    public:
         PendingScript()
-            : watchingForLoadState(NotWatchingForLoad)
-            , startingLineNumber(0)
+            : startingLineNumber(0)
+            , m_watchingForLoad(false)
         {
         }
 
-        bool watchingForLoad()
+        ~PendingScript();
+
+        PassRefPtr<Element> releaseElementAndClear();
+
+        bool watchingForLoad() const { return m_watchingForLoad; }
+        void setWatchingForLoad(bool b) { m_watchingForLoad = b; }
+
+        CachedScript* cachedScript() const;
+        void setCachedScript(CachedScript*);
+
+        virtual void notifyFinished(CachedResource*)
         {
-            return watchingForLoadState != NotWatchingForLoad;
         }
 
         RefPtr<Element> element;
-        CachedResourceHandle<CachedScript> cachedScript;
-        WatchingForLoadState watchingForLoadState;
         int startingLineNumber; // Only used for inline script tags.
         // HTML5 has an isReady parameter, however isReady ends up equivalent to
         // m_document->haveStylesheetsLoaded() && cachedScript->isLoaded()
+
+    private:
+        bool m_watchingForLoad;
+        CachedResourceHandle<CachedScript> m_cachedScript;
     };
 
     Frame* frame() const;
index 82ad70ddbc2098920871d8b8e6ff82b3b4c5d03c..a12952079f894671b461e6714165d786aae21713 100644 (file)
@@ -38,7 +38,7 @@ class HTMLScriptRunnerHost {
 public:
     virtual ~HTMLScriptRunnerHost() { }
 
-    // Implementors must call cachedResource->addClient() immediately.
+    // Implementors should call cachedResource->addClient() here or soon after.
     virtual void watchForLoad(CachedResource*) = 0;
     // Implementors must call cachedResource->removeClient() immediately.
     virtual void stopWatchingForLoad(CachedResource*) = 0;