Make ResourceFetcher::m_documentResources use WeakPtr

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

Index: third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
diff --git a/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp b/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
index d51f1939f3844f9c5ae88f087b026986413dde8b..2f977aaa005a5b07e0ec185138a1faa1af20fd9a 100644
--- a/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
+++ b/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
@@ -166,7 +166,6 @@ static WebURLRequest::RequestContext requestContextFromType(bool isMainFrame, Re
 
 ResourceFetcher::ResourceFetcher(FetchContext* context)
     : m_context(context)
-    , m_garbageCollectDocumentResourcesTimer(this, &ResourceFetcher::garbageCollectDocumentResourcesTimerFired)
     , m_resourceTimingReportTimer(this, &ResourceFetcher::resourceTimingReportTimerFired)
     , m_autoLoadImages(true)
     , m_imagesEnabled(true)
@@ -195,7 +194,8 @@ WebTaskRunner* ResourceFetcher::loadingTaskRunner()
 Resource* ResourceFetcher::cachedResource(const KURL& resourceURL) const
 {
     KURL url = MemoryCache::removeFragmentIdentifierIfNeeded(resourceURL);
-    return m_documentResources.get(url).get();
+    const WeakPtrWillBeWeakMember<Resource>& resource = m_documentResources.get(url);

[Nate Chapin, 2015/12/14 23:12:16]

WeakMember is smarter than WeakPtr about HashMap return values and actually
returns a Resource* from get(url). I wasn't able to find a single-line return
statement that compiled both with and without Oilpan.

[sof, 2015/12/15 10:08:01]

Traits are in place to unwrap the WeakMember on get(); not worth #if'ing for,
the transition type will be removed soon enough.

+    return resource.get();
 }
 
 bool ResourceFetcher::canAccessResource(Resource* resource, SecurityOrigin* sourceOrigin, const KURL& url, AccessControlLoggingDecision logErrorsDecision) const
@@ -321,7 +321,6 @@ void ResourceFetcher::preCacheData(const FetchRequest& request, const ResourceFa
     resource->setCacheIdentifier(cacheIdentifier);
     resource->finish();
     memoryCache()->add(resource.get());
-    scheduleDocumentResourcesGC();
 }
 
 void ResourceFetcher::moveCachedNonBlockingResourceToBlocking(Resource* resource)
@@ -458,7 +457,7 @@ ResourcePtr<Resource> ResourceFetcher::requestResource(FetchRequest& request, co
     requestLoadStarted(resource.get(), request, policy == Use ? ResourceLoadingFromCache : ResourceLoadingFromNetwork, isStaticData);
 
     ASSERT(resource->url() == url.string());
-    m_documentResources.set(resource->url(), resource);
+    m_documentResources.set(resource->url(), resource->asWeakPtr());
     return resource;
 }
 
@@ -772,7 +771,7 @@ void ResourceFetcher::reloadImagesIfNotDeferred()
 {
     for (const auto& documentResource : m_documentResources) {

[sof, 2015/12/15 12:29:45]

This can be simplified some once we can assume Oilpan; add a TODO here also?

[Nate Chapin, 2015/12/15 20:30:59]

Done.

         Resource* resource = documentResource.value.get();
-        if (resource->type() == Resource::Image && resource->stillNeedsLoad() && !clientDefersImage(resource->url()))
+        if (resource && resource->type() == Resource::Image && resource->stillNeedsLoad() && !clientDefersImage(resource->url()))
             const_cast<Resource*>(resource)->load(this, defaultResourceOptions());
     }
 }
@@ -786,41 +785,9 @@ void ResourceFetcher::redirectReceived(Resource* resource, const ResourceRespons
 
 void ResourceFetcher::didLoadResource()
 {
-    scheduleDocumentResourcesGC();
     context().didLoadResource();
 }
 
-void ResourceFetcher::scheduleDocumentResourcesGC()
-{
-    if (!m_garbageCollectDocumentResourcesTimer.isActive())
-        m_garbageCollectDocumentResourcesTimer.startOneShot(0, BLINK_FROM_HERE);
-}
-
-// Garbage collecting m_documentResources is a workaround for the
-// ResourcePtrs on the RHS being strong references. Ideally this
-// would be a weak map, however ResourcePtrs perform additional
-// bookkeeping on Resources, so instead pseudo-GC them -- when the
-// reference count reaches 1, m_documentResources is the only reference, so
-// remove it from the map.
-void ResourceFetcher::garbageCollectDocumentResourcesTimerFired(Timer<ResourceFetcher>* timer)
-{
-    ASSERT_UNUSED(timer, timer == &m_garbageCollectDocumentResourcesTimer);
-    garbageCollectDocumentResources();
-}
-
-void ResourceFetcher::garbageCollectDocumentResources()
-{
-    typedef Vector<String, 10> StringVector;
-    StringVector resourcesToDelete;
-
-    for (const auto& documentResource : m_documentResources) {
-        if (documentResource.value->hasOneHandle())
-            resourcesToDelete.append(documentResource.key);
-    }
-
-    m_documentResources.removeAll(resourcesToDelete);
-}
-
 int ResourceFetcher::requestCount() const
 {
     return m_loaders ? m_loaders->size() : 0;
@@ -1182,6 +1149,7 @@ DEFINE_TRACE(ResourceFetcher)
     visitor->trace(m_loaders);
     visitor->trace(m_nonBlockingLoaders);
 #if ENABLE(OILPAN)
+    visitor->trace(m_documentResources);
     visitor->trace(m_preloads);
     visitor->trace(m_resourceTimingInfoMap);
 #endif