Oilpan: have PendingScripts trace their script elements.

Source/core/html/parser/HTMLScriptRunner.cpp

Index: Source/core/html/parser/HTMLScriptRunner.cpp
diff --git a/Source/core/html/parser/HTMLScriptRunner.cpp b/Source/core/html/parser/HTMLScriptRunner.cpp
index 91e008b4ca370d5714c448ded81c215f60c1d77e..cb5179f195424f85e30672dc0e2a3bf239c6642b 100644
--- a/Source/core/html/parser/HTMLScriptRunner.cpp
+++ b/Source/core/html/parser/HTMLScriptRunner.cpp
@@ -54,7 +54,23 @@ HTMLScriptRunner::HTMLScriptRunner(Document* document, HTMLScriptRunnerHost* hos
 
 HTMLScriptRunner::~HTMLScriptRunner()
 {
-    // FIXME: Should we be passed a "done loading/parsing" callback sooner than destruction?
+#if ENABLE(OILPAN)
+    // If the document is destructed without having explicitly
+    // detached the parser (and this script runner object), perform
+    // detach steps now. This will happen if the Document, the parser
+    // and this script runner object are swept out in the same GC.

[haraken, 2014/05/29 11:32:08]

Understood. Then I get back to the eseidel's question: why can't the detach()
code just stay in the destructor?

[sof, 2014/05/29 20:39:54]

We have to have Document::detachParser() behave as synchronously as in a
non-Oilpan setting -- it currently calls detach on the parser, followed by
deleting it (non-Oilpan.) Parser detaching happens either from the Document dtor
or when cancelling the parser for other reasons.

If we were to delay stopping these pending scripts until the next time the
Oilpan GC runs (and ~HTMLScriptRunner is invoked when sweeping), that would just
be wrong. Risking either scripts to complete loading and touching the dead or
detached parser.

That's why we have moved this dtor code into detach(). The only wrinkle with
that is if we GC the Document and parser at the same time. We cannot detach the
parser from the Document finalizer, as the parser is another heap object, so we
have to arrange for the effects of that detach to happen by other ways for this
HTMLScriptRunner. Which is done by calling detach() from its
destructor/finalizer.

Does that help explaining why? 

See also tkent's comment on why this is needed:
https://codereview.chromium.org/298863010/#msg7
Similarly ager's: https://codereview.chromium.org/298863010/#msg14

[haraken, 2014/05/30 01:28:51]

Makes sense, sorry I was behind what you've already discussed.

+    detach();
+#else
+    // Verify that detach() has been called.
+    ASSERT(!m_document);
+#endif
+}
+
+void HTMLScriptRunner::detach()
+{
+    if (!m_document)
+        return;
+
     if (m_parserBlockingScript.resource() && m_parserBlockingScript.watchingForLoad())
         stopWatchingForLoad(m_parserBlockingScript);
 
@@ -63,11 +79,7 @@ HTMLScriptRunner::~HTMLScriptRunner()
         if (pendingScript.resource() && pendingScript.watchingForLoad())
             stopWatchingForLoad(pendingScript);
     }
-}
-
-void HTMLScriptRunner::detach()
-{
-    m_document = 0;
+    m_document = nullptr;
 }
 
 static KURL documentURLForScriptExecution(Document* document)
@@ -142,7 +154,7 @@ void HTMLScriptRunner::executePendingScriptAndDispatchEvent(PendingScript& pendi
     }
 
     // Clear the pending script before possible rentrancy from executeScript()
-    RefPtr<Element> element = pendingScript.releaseElementAndClear();
+    RefPtrWillBeRawPtr<Element> element = pendingScript.releaseElementAndClear();
     if (ScriptLoader* scriptLoader = toScriptLoaderIfPossible(element.get())) {
         NestingLevelIncrementer nestingLevelIncrementer(m_scriptNestingLevel);
         IgnoreDestructiveWriteCountIncrementer ignoreDestructiveWriteCountIncrementer(m_document);
@@ -160,21 +172,30 @@ void HTMLScriptRunner::executePendingScriptAndDispatchEvent(PendingScript& pendi
 void HTMLScriptRunner::watchForLoad(PendingScript& pendingScript)
 {
     ASSERT(!pendingScript.watchingForLoad());
-    m_host->watchForLoad(pendingScript.resource());
+    ASSERT(!pendingScript.resource()->isLoaded());
+    // 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.
+    pendingScript.resource()->addClient(this);
     pendingScript.setWatchingForLoad(true);
 }
 
 void HTMLScriptRunner::stopWatchingForLoad(PendingScript& pendingScript)
 {
     ASSERT(pendingScript.watchingForLoad());
-    m_host->stopWatchingForLoad(pendingScript.resource());
+    pendingScript.resource()->removeClient(this);
     pendingScript.setWatchingForLoad(false);
 }
 
+void HTMLScriptRunner::notifyFinished(Resource* cachedResource)
+{
+    m_host->notifyScriptLoaded(cachedResource);
+}
+
 // Implements the steps for 'An end tag whose tag name is "script"'
 // http://whatwg.org/html#scriptEndTag
 // Script handling lives outside the tree builder to keep each class simple.
-void HTMLScriptRunner::execute(PassRefPtr<Element> scriptElement, const TextPosition& scriptStartPosition)
+void HTMLScriptRunner::execute(PassRefPtrWillBeRawPtr<Element> scriptElement, const TextPosition& scriptStartPosition)
 {
     ASSERT(scriptElement);
     // FIXME: If scripting is disabled, always just return.
@@ -328,4 +349,11 @@ void HTMLScriptRunner::runScript(Element* script, const TextPosition& scriptStar
     }
 }
 
+void HTMLScriptRunner::trace(Visitor* visitor)
+{
+    visitor->trace(m_document);
+    visitor->trace(m_parserBlockingScript);
+    visitor->trace(m_scriptsToExecuteAfterParsing);
+}
+
 }