Oilpan: have PendingScripts trace their script elements.

Source/core/html/parser/HTMLScriptRunner.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 36 matching lines @@
     : m_document(document)
     , m_host(host)
     , m_scriptNestingLevel(0)
     , m_hasScriptsWaitingForResources(false)
 {
     ASSERT(m_host);
 }
 
 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)
 {
     if (!document)
         return KURL();
 
     if (!document->frame()) {
         if (document->importsController())
             return document->url();
@@ skipping 54 matching lines @@
         Microtask::performCheckpoint();
         if (pendingScriptType == PendingScriptBlockingParser) {
             m_hasScriptsWaitingForResources = !m_document->isScriptExecutionReady();
             // The parser cannot be unblocked as a microtask requested another resource
             if (m_hasScriptsWaitingForResources)
                 return;
         }
     }
 
     // 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);
         if (errorOccurred)
             scriptLoader->dispatchErrorEvent();
         else {
             ASSERT(isExecutingScript());
             scriptLoader->executeScript(sourceCode);
             element->dispatchEvent(createScriptLoadEvent());
         }
     }
     ASSERT(!isExecutingScript());
 }
 
 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.
 
     bool hadPreloadScanner = m_host->hasPreloadScanner();
 
     // Try to execute the script given to us.
     runScript(scriptElement.get(), scriptStartPosition);
 
     if (hasParserBlockingScript()) {
@@ skipping 133 matching lines @@
             } else {
                 ScriptSourceCode sourceCode(script->textContent(), documentURLForScriptExecution(m_document), scriptStartPosition);
                 scriptLoader->executeScript(sourceCode);
             }
         } else {
             requestParsingBlockingScript(script);
         }
     }
 }
 
+void HTMLScriptRunner::trace(Visitor* visitor)
+{
+    visitor->trace(m_document);
+    visitor->trace(m_parserBlockingScript);
+    visitor->trace(m_scriptsToExecuteAfterParsing);
 }
+
+}