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..87dc7ac898e9c7190a169aa9424a862151ac3cb2 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)) {
+        // In this case, the client is swept from client lists because they
+        // hold clients as weak member.

[haraken, 2016/08/12 10:45:20]

// This code may be called in a pre-finalizer, where weak members in the
HashCountedSet are already swept out. In that case, we just need to call
didRemoveClientOrObserver().

[yhirano, 2016/08/16 08:57:24]

Done.

+        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,10 @@ 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;

[haraken, 2016/08/12 10:45:20]

Can we use the asVector?

[yhirano, 2016/08/16 08:57:24]

Using asVector means making clientsToNotify a
HeapVector<WeakMember<ResourceClient>>. I think keeping clientsToNotify's type
is more important.

[haraken, 2016/08/16 11:55:44]

asVector should convert HeapHashCountedSet<WeakMember> to HeapVector<Member>.
Remember that copyToVector is converting HeapHashSet<WeakMember> to
HeapVector<Member>.

Nit: I'd prefer introducing copyToVector(HeapHashCountedSet<WeakMember/Member>,
HeapVector<Member>) to make the type conversion explicit. Then the call site
needs to create HeapVector<Member> before calling copyToVector, which makes the
type conversion explicit.

I'm wondering why you want to keep the type as WeakMembers.

[yhirano, 2016/08/17 08:36:09]

I think HashCountedSet<T, ...>.asVector() should return Vector<T, ...>. WTF
containers don't know about Oilpan, so I feel it confusing if
HeapHashCountedSet<T, ...>.asVector() doesn't return Vector<T, ...>.

It's possible to introduce a type parameter, like 

clientsToNotify =
m_clientsAwaitingClalback.asVector<HeapVector<Member<ResourceClient>>>();

but it is too verbose.

PS11 uses copyToVector.

+    clientsToNotify.reserveCapacity(m_clientsAwaitingCallback.size());
+    for (const auto& keyvalue : m_clientsAwaitingCallback)
+        clientsToNotify.append(keyvalue.key);
 
     for (const auto& client : clientsToNotify) {
         // Handle case (2) to skip removed clients.