Make Resource RefCountedWillBeGarbageCollectedFinalized, attempt #2

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

Index: third_party/WebKit/Source/core/dom/PendingScript.cpp
diff --git a/third_party/WebKit/Source/core/dom/PendingScript.cpp b/third_party/WebKit/Source/core/dom/PendingScript.cpp
index 8b3ca3a15d83bc71a05b987ed471057cc4e28f1c..7a2033545ee0d842c140a496ec781f86545eb6b0 100644
--- a/third_party/WebKit/Source/core/dom/PendingScript.cpp
+++ b/third_party/WebKit/Source/core/dom/PendingScript.cpp
@@ -42,14 +42,26 @@ PendingScript::PendingScript(Element* element, ScriptResource* resource)
     : m_watchingForLoad(false)
     , m_element(element)
     , m_integrityFailure(false)
+    , m_client(nullptr)
 {
     setScriptResource(resource);
+#if ENABLE(OILPAN)
+    ThreadState::current()->registerPreFinalizer(this);
+#endif
 }
 
 PendingScript::~PendingScript()
 {
 }
 
+void PendingScript::dispose()

[Nate Chapin, 2016/02/04 23:24:38]

Moving the guts of ScriptLoader::detach here fixes
https://code.google.com/p/chromium/issues/detail?id=583624 (which is probably
the root cause of https://code.google.com/p/chromium/issues/detail?id=583630
too).

https://code.google.com/p/chromium/issues/detail?id=583624#c5 describes the
pre-finalizer ordering problem that causes the crash. This guarantees the old
ordering: ScriptLoader has its client removed before the superclass
ResourceOwner's pre-finalizer runs, which will remove this as a client and clear
resource().

+{
+    if (!m_client)
+        return;
+    stopWatchingForLoad(m_client);
+    releaseElementAndClear();
+}
+
 PendingScript& PendingScript::operator=(const PendingScript& other)
 {
     if (this == &other)
@@ -67,17 +79,15 @@ PendingScript& PendingScript::operator=(const PendingScript& other)
 void PendingScript::watchForLoad(ScriptResourceClient* client)
 {
     ASSERT(!m_watchingForLoad);
-    // addClient() will call notifyFinished() if the load is complete. Callers
+    // addClient() will call streamingFinished() 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. We also need to set
     // m_watchingForLoad early, since addClient() can result in calling
     // notifyFinished and further stopWatchingForLoad().
     m_watchingForLoad = true;
-    if (m_streamer) {
-        m_streamer->addClient(client);
-    } else {
+    m_client = client;
+    if (!m_streamer)
         resource()->addClient(client);
-    }
 }
 
 void PendingScript::stopWatchingForLoad(ScriptResourceClient* client)

[haraken, 2016/02/04 23:56:30]

You won't need to pass |client| since PendingScript already knows m_client.

[Nate Chapin, 2016/02/06 00:34:07]

Agreed, removed. I could see the argument for keeping |client| and asserting
m_client == client, but that's probably not necessary.

@@ -85,14 +95,19 @@ void PendingScript::stopWatchingForLoad(ScriptResourceClient* client)
     if (!m_watchingForLoad)
         return;
     ASSERT(resource());
-    if (m_streamer) {
-        m_streamer->removeClient(client);
-    } else {
+    m_client = nullptr;
+    if (!m_streamer)
         resource()->removeClient(client);
-    }
     m_watchingForLoad = false;
 }
 
+void PendingScript::streamingFinished()
+{
+    ASSERT(resource());
+    if (m_client)
+        m_client->notifyFinished(resource());
+}
+
 void PendingScript::setElement(Element* element)
 {
     m_element = element;