Move ResourceClient to Oilpan heap

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

 /*
     Copyright (C) 1998 Lars Knoll (knoll@mpi-hd.mpg.de)
     Copyright (C) 2001 Dirk Mueller (mueller@kde.org)
     Copyright (C) 2002 Waldo Bastian (bastian@kde.org)
     Copyright (C) 2006 Samuel Weinig (sam.weinig@gmail.com)
     Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
 
     This library is free software; you can redistribute it and/or
     modify it under the terms of the GNU Library General Public
     License as published by the Free Software Foundation; either
@@ skipping 10 matching lines @@
     Boston, MA 02110-1301, USA.
 */
 
 #include "core/fetch/Resource.h"
 
 #include "core/fetch/CachedMetadata.h"
 #include "core/fetch/CrossOriginAccessControl.h"
 #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"
 #include "platform/Logging.h"
 #include "platform/RuntimeEnabledFeatures.h"
 #include "platform/SharedBuffer.h"
 #include "platform/TraceEvent.h"
 #include "platform/network/HTTPParsers.h"
 #include "platform/weborigin/KURL.h"
 #include "public/platform/Platform.h"
@@ skipping 282 matching lines @@
 
 Resource::~Resource()
 {
     InstanceCounters::decrementCounter(InstanceCounters::ResourceCounter);
 }
 
 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)
 {
     RELEASE_ASSERT(!m_loader);
     ASSERT(stillNeedsLoad());
     m_loader = loader;
     m_status = Pending;
 }
 
@@ skipping 32 matching lines @@
 
 void Resource::setDataBufferingPolicy(DataBufferingPolicy dataBufferingPolicy)
 {
     m_options.dataBufferingPolicy = dataBufferingPolicy;
     m_data.clear();
     setEncodedSize(0);
 }
 
 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);
 }
 
 void Resource::error(const ResourceError& error)
 {
     ASSERT(!error.isNull());
     m_error = error;
     m_isRevalidating = false;
@@ skipping 315 matching lines @@
         return;
     }
 
     m_clients.add(client);
     didAddClient(client);
     return;
 }
 
 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))
         m_clientsAwaitingCallback.remove(client);
     else
         m_clients.remove(client);
 
     if (m_clientsAwaitingCallback.isEmpty())
         ResourceCallback::callbackHandler().cancel(this);
 
     didRemoveClientOrObserver();
-    // This object may be dead here.
 }
 
 void Resource::didRemoveClientOrObserver()
 {
     if (!hasClientsOrObservers()) {
         memoryCache()->makeDead(this);
         allClientsAndObserversRemoved();
 
         // RFC2616 14.9.2:
         // "no-store: ... MUST make a best-effort attempt to remove the information from volatile storage as promptly as possible"
@@ skipping 57 matching lines @@
 {
     // We're going to notify clients one by one. It is simple if the client does nothing.
     // However there are a couple other things that can happen.
     //
     // 1. Clients can be added during the loop. Make sure they are not processed.
     // 2. Clients can be removed during the loop. Make sure they are always available to be
     //    removed. Also don't call removed clients or add them back.
 
     // 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.
         if (!m_clientsAwaitingCallback.remove(client))
             continue;
         m_clients.add(client);
         didAddClient(client);
     }
 
     // It is still possible for the above loop to finish a new client synchronously.
@@ skipping 288 matching lines @@
     case Resource::TextTrack:
     case Resource::Media:
     case Resource::Manifest:
         return false;
     }
     ASSERT_NOT_REACHED();
     return false;
 }
 
 } // namespace blink