Make Resource RefCountedWillBeGarbageCollectedFinalized, attempt #2

third_party/WebKit/Source/core/dom/PendingScript.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 24 matching lines @@
 
 PassOwnPtrWillBeRawPtr<PendingScript> PendingScript::create(Element* element, ScriptResource* resource)
 {
     return adoptPtrWillBeNoop(new PendingScript(element, resource));
 }
 
 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)
         return *this;
 
     m_watchingForLoad = other.m_watchingForLoad;
     m_element = other.m_element;
     m_startingPosition = other.m_startingPosition;
     m_integrityFailure = other.m_integrityFailure;
     m_streamer = other.m_streamer;
     this->ResourceOwner<ScriptResource, ScriptResourceClient>::operator=(other);
     return *this;
 }
 
 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.

 {
     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;
 }
 
 PassRefPtrWillBeRawPtr<Element> PendingScript::releaseElementAndClear()
 {
     setScriptResource(0);
     m_watchingForLoad = false;
     m_startingPosition = TextPosition::belowRangePosition();
@@ skipping 91 matching lines @@
 bool PendingScript::isReady() const
 {
     if (resource() && !resource()->isLoaded())
         return false;
     if (m_streamer && !m_streamer->isFinished())
         return false;
     return true;
 }
 
 } // namespace blink