Move ResourceClient to Oilpan heap

third_party/WebKit/Source/core/fetch/Resource.cpp

Index: third_party/WebKit/Source/core/fetch/Resource.cpp
diff --git a/third_party/WebKit/Source/core/fetch/Resource.cpp b/third_party/WebKit/Source/core/fetch/Resource.cpp
index adf843dd7e9831f464f8fd72224979f566c5ea9f..d0ea83a07adf5a23cc137705c11a2b8d385ad486 100644
--- a/third_party/WebKit/Source/core/fetch/Resource.cpp
+++ b/third_party/WebKit/Source/core/fetch/Resource.cpp
@@ -28,7 +28,7 @@
 #include "core/fetch/FetchInitiatorTypeNames.h"
 #include "core/fetch/MemoryCache.h"
 #include "core/fetch/ResourceClient.h"
-#include "core/fetch/ResourceClientOrObserverWalker.h"
+#include "core/fetch/ResourceClientWalker.h"
 #include "core/fetch/ResourceLoader.h"
 #include "core/inspector/InstanceCounters.h"
 #include "platform/Histogram.h"
@@ -331,6 +331,9 @@ DEFINE_TRACE(Resource)
 {
     visitor->trace(m_loader);
     visitor->trace(m_cacheHandler);
+    visitor->trace(m_clients);
+    visitor->trace(m_clientsAwaitingCallback);
+    visitor->trace(m_finishedClients);
 }
 
 void Resource::setLoader(ResourceLoader* loader)
@@ -383,7 +386,7 @@ void Resource::setDataBufferingPolicy(DataBufferingPolicy dataBufferingPolicy)
 
 void Resource::markClientsAndObserversFinished()
 {
-    HashCountedSet<ResourceClient*> clients;
+    HeapHashCountedSet<WeakMember<ResourceClient>> clients;
     m_clients.swap(clients);
     for (const auto& it : clients)
         m_finishedClients.add(it.key, it.value);
@@ -719,7 +722,12 @@ void Resource::addClient(ResourceClient* client)
 
 void Resource::removeClient(ResourceClient* client)
 {
-    ASSERT(hasClient(client));
+    if (!hasClient(client)) {

[haraken, 2016/08/02 04:18:06]

Would you help me understand when this can happen? It seems problematic that the
client is already removed from the weak hash counted set while you're still
holding a reference to the client...

[yhirano, 2016/08/03 10:40:28]

This function can be called from a pre-finalizer (see ResourceOwner). In such a
case weak pointers are cleared just before the pre-finalization step.

[haraken, 2016/08/03 11:11:40]

Can we avoid calling Resource::removeClient() in the pre-finalizer? Alternately,
we can call didRemoveClientOrObserver() in the pre-finalizer.

[haraken, 2016/08/03 11:11:40]

Can you avoid calling Resource::removeClient in the pre-finalizer? You can call
didRemoveClientOrObserver() instead.

[yhirano, 2016/08/09 07:38:21]

It's hard because callers generally don't distinguish them. For example,
PendingScript::stopWatchingForLoad calls Resource::removeClient and there are 8
callers who call PendingScript::stopWatchingForLoad. At least one of them
(ScriptLoader::notifyFinished) can be called as a non-prefinalizer and at least
one of them (HTMLScriptRunner::detach) can be called in the prefinalization
step.

+        // In this case, the client is swept from client lists because they
+        // hold clients as weak member.
+        didRemoveClientOrObserver();
+        return;
+    }
     if (m_finishedClients.contains(client))
         m_finishedClients.remove(client);
     else if (m_clientsAwaitingCallback.contains(client))
@@ -731,7 +739,6 @@ void Resource::removeClient(ResourceClient* client)
         ResourceCallback::callbackHandler().cancel(this);
 
     didRemoveClientOrObserver();
-    // This object may be dead here.
 }
 
 void Resource::didRemoveClientOrObserver()
@@ -809,8 +816,13 @@ void Resource::finishPendingClients()
 
     // Handle case (1) by saving a list of clients to notify. A separate list also ensure
     // a client is either in m_clients or m_clientsAwaitingCallback.
-    Vector<ResourceClient*> clientsToNotify;
-    copyToVector(m_clientsAwaitingCallback, clientsToNotify);
+    HeapVector<Member<ResourceClient>> clientsToNotify;
+    clientsToNotify.reserveCapacity(m_clientsAwaitingCallback.size());

[haraken, 2016/08/02 04:18:06]

Can we implement copyToVector for a hash counted set?

[yhirano, 2016/08/03 10:40:28]

We already have it, but as I said in the other place, it's not usable to convert
HeapHashCountedSet<WeakMember<T>> to HeapVector<Member<T>>.

+    for (const auto& keyvalue : m_clientsAwaitingCallback) {
+        ResourceClient* client = keyvalue.key;
+        if (client)

[haraken, 2016/08/02 04:18:06]

I don't think this can happen. See my comment below.

[yhirano, 2016/08/03 10:40:28]

Done.

+            clientsToNotify.append(client);
+    }
 
     for (const auto& client : clientsToNotify) {
         // Handle case (2) to skip removed clients.