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..aaf6f2d36164f086500e0b846afbac80936270a7 100644
--- a/Source/core/html/parser/HTMLScriptRunner.cpp
+++ b/Source/core/html/parser/HTMLScriptRunner.cpp
@@ -54,20 +54,30 @@ HTMLScriptRunner::HTMLScriptRunner(Document* document, HTMLScriptRunnerHost* hos
 
 HTMLScriptRunner::~HTMLScriptRunner()
 {
-    // FIXME: Should we be passed a "done loading/parsing" callback sooner than destruction?
-    if (m_parserBlockingScript.resource() && m_parserBlockingScript.watchingForLoad())
-        stopWatchingForLoad(m_parserBlockingScript);
-
-    while (!m_scriptsToExecuteAfterParsing.isEmpty()) {
-        PendingScript pendingScript = m_scriptsToExecuteAfterParsing.takeFirst();
-        if (pendingScript.resource() && pendingScript.watchingForLoad())
-            stopWatchingForLoad(pendingScript);
-    }
+#if ENABLE(OILPAN)
+    // If the document is destructed without having explicitly
+    // detached the parser (and this script runner object),
+    // perform detach steps now.
+    detach();

[eseidel, 2014/05/28 22:43:38]

Isn't this a bug in ~Document()?

[haraken, 2014/05/29 01:46:08]

Agreed, I guess the Document should always call explicit detach() before being
destructed.

(In general, it's not a good thing to call explicit detach() in a destructor,
since the timing when the destructor is called is not deterministic.)

[sof, 2014/05/29 06:02:07]

I've tried to improve the comment to clarify. If the Document, parser and script
runner object die in the same GC, no explicit detach will happen.

+#else
+    // Verify that detach() has been called.
+    ASSERT(!m_document);
+#endif
 }
 
 void HTMLScriptRunner::detach()
 {
-    m_document = 0;
+    if (m_document) {

[eseidel, 2014/05/28 22:43:38]

Early return is better.

[haraken, 2014/05/29 01:46:08]

Does this check mean that detach() can be called twice?

[sof, 2014/05/29 06:02:07]

Yes it does. How would you distinguish between an explicitly detached script
runner object that's later finalized and one that's finalized along with its
parser (hence no detach() happens)?

[sof, 2014/05/29 06:02:07]

Thanks, done.

+        if (m_parserBlockingScript.resource() && m_parserBlockingScript.watchingForLoad())
+            stopWatchingForLoad(m_parserBlockingScript);
+
+        while (!m_scriptsToExecuteAfterParsing.isEmpty()) {
+            PendingScript pendingScript = m_scriptsToExecuteAfterParsing.takeFirst();

[eseidel, 2014/05/28 22:43:38]

Why can't PendingScript just do this in its destructor?

[haraken, 2014/05/29 01:46:08]

That's a good question.

I think that in this CL we can just call this in the destructor, but I don't
think it's a right fix. I guess a right fix would be:

- Make the Document always call explicit HTMLScriptRunner::detach() before the
HTMLScriptRunner destructed.

- Make m_scriptsToExecuteAfterParsing a HeapDeque.

- Make the destructor empty.

What do you think?

[sof, 2014/05/29 06:02:07]

Can't ever guarantee the "always" bit, destruction may happen without explicit
detachment.
How would you refer to the HTMLScriptRunner object from the objects that heap
deque contains?
Not possible given the above.

+            if (pendingScript.resource() && pendingScript.watchingForLoad())
+                stopWatchingForLoad(pendingScript);
+        }
+        m_document = nullptr;
+    }
 }
 
 static KURL documentURLForScriptExecution(Document* document)
@@ -142,7 +152,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 +170,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);

[eseidel, 2014/05/28 22:43:38]

Bleh.  Manual add/remove calls are nasty.  They're just bugs waiting to happen.

     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 +347,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);
+}
+
 }