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 96c1129075a39e2484a4907b38b952ff3223b79b..0a83b2fb459c6191414e3ce744cff73cace24d68 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"
@@ -308,6 +308,7 @@ Resource::Resource(const ResourceRequest& request, Type type, const ResourceLoad
     , m_needsSynchronousCacheHit(false)
     , m_linkPreload(false)
     , m_isRevalidating(false)
+    , m_isAlive(false)
     , m_options(options)
     , m_responseTimestamp(currentTime())
     , m_cancelTimer(this, &Resource::cancelTimerFired)
@@ -330,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)
@@ -382,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);
@@ -691,8 +695,10 @@ void Resource::willAddClientOrObserver()
             preloadDiscoveryHistogram.count(timeSinceDiscovery);
         }
     }
-    if (!hasClientsOrObservers())
+    if (!hasClientsOrObservers()) {
+        m_isAlive = true;
         memoryCache()->makeLive(this);
+    }
 }
 
 void Resource::addClient(ResourceClient* client)
@@ -718,7 +724,13 @@ void Resource::addClient(ResourceClient* client)
 
 void Resource::removeClient(ResourceClient* client)
 {
-    ASSERT(hasClient(client));
+    if (!hasClient(client)) {

[haraken, 2016/08/18 08:06:17]

I begin to think this is problematic...

If Resource and ResoureClient die at the same time, the weak processing doesn't
run. However, pre-finalizer runs. Then hasClient() returns true and we end up
with executing line 734 - 744. Which should be problematic... :/

[yhirano, 2016/08/18 08:31:53]

Hmm, I did think the weak processing is executed in such a case...

Let me confirm, an entry |e| in a HeapVector<WeakMember<E>> |v| is cleared by
the weak processing when |e| is no longer reachable but |v| is still reachable,
is that correct?

[yhirano, 2016/08/18 08:53:15]

What problems do we have in such a case? I think L734-L739 (accessing
unreachable heap vectors in a prefinalizer) is safe to process and
didRemoveClientOrObserver is called in any case.

I'm not so sure about ResourceCallback::cancel but it looks consisting of
HeapHashSet operations and CancellableTaskFactory which is not related with
Oilpan.

[haraken, 2016/08/18 09:11:01]

Does it mean that it's always safe to run line 734 - 744 regardless of whether
we're in a pre-finalizer or not? Then maybe can we just simply the hasClient()
check entirely?

[yhirano, 2016/08/26 09:12:16]

Even without this change removeClient can be called in the pre-finalization
step. What this CL changes is that m_clients (and other heap collections) will
be swept automatically just before the pre-finalization step. L734-L744 is not
affected by the change because when |client| is swept this branch is taken.

[haraken, 2016/08/26 09:49:29]

My question is: Why do we need to have the branch at 727?

If Resource and ResourceClient die at the same time, the weak processing doesn't
run. Then the pre-finalizer will run line 734 - 744. Hence, line 734 - 744 must
be anyway written in a way that doesn't touch any other on-heap objects. If
that's the case, we can just unconditionally run line 734 - 744. Hence we don't
need the branch at 727.

[yhirano, 2016/08/26 10:08:55]

Because it's possible that a Resource is reachable but all ResourceClients
registered are unreachable and hence swept. Imagine a ResourceOwner |o| is
registered to a Resource |r| and the memory cache holds |r|. When |o| becomes
unreachable, this function will be called in |o|'s pre-finalizer, but |o| is
swept from |r->m_clients|. Note that |r| is still reachable via the memory
cache.

[haraken, 2016/08/26 10:19:51]

It will happen, but what's a problem of running line 734 - 744 then?

[yhirano, 2016/08/26 10:26:12]

Hm, I didn't think of that. It looks safe as HashCountedSet::remove is no-op in
such cases.

+        // 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().
+        didRemoveClientOrObserver();
+        return;
+    }
     if (m_finishedClients.contains(client))
         m_finishedClients.remove(client);
     else if (m_clientsAwaitingCallback.contains(client))
@@ -730,12 +742,12 @@ void Resource::removeClient(ResourceClient* client)
         ResourceCallback::callbackHandler().cancel(this);
 
     didRemoveClientOrObserver();
-    // This object may be dead here.
 }
 
 void Resource::didRemoveClientOrObserver()
 {
-    if (!hasClientsOrObservers()) {
+    if (!hasClientsOrObservers() && m_isAlive) {
+        m_isAlive = false;
         memoryCache()->makeDead(this);
         allClientsAndObserversRemoved();
 
@@ -808,7 +820,7 @@ 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;
+    HeapVector<Member<ResourceClient>> clientsToNotify;
     copyToVector(m_clientsAwaitingCallback, clientsToNotify);
 
     for (const auto& client : clientsToNotify) {