Revalidate using the same Resource, attempt #2

Source/core/fetch/Resource.cpp

Index: Source/core/fetch/Resource.cpp
diff --git a/Source/core/fetch/Resource.cpp b/Source/core/fetch/Resource.cpp
index 1987b14e1cd09756f642ee12a8bdc1db9adc4bad..313409c3c87d8806e79808de6a47b7c5067460ad 100644
--- a/Source/core/fetch/Resource.cpp
+++ b/Source/core/fetch/Resource.cpp
@@ -168,8 +168,6 @@ Resource::Resource(const ResourceRequest& request, Type type)
 #ifdef ENABLE_RESOURCE_IS_DELETED_CHECK
     , m_deleted(false)
 #endif
-    , m_resourceToRevalidate(nullptr)
-    , m_proxyResource(nullptr)
 {
     ASSERT(m_type == unsigned(type)); // m_type is a bitfield, so this tests careless updates of the enum.
     InstanceCounters::incrementCounter(InstanceCounters::ResourceCounter);
@@ -193,7 +191,6 @@ Resource::Resource(const ResourceRequest& request, Type type)
 
 Resource::~Resource()
 {
-    ASSERT(!m_resourceToRevalidate); // Should be true because canDelete() checks this.
     ASSERT(canDelete());
     RELEASE_ASSERT(!memoryCache()->contains(this));
     RELEASE_ASSERT(!ResourceCallback::callbackHandler()->isScheduled(this));
@@ -215,8 +212,6 @@ void Resource::dispose()
 DEFINE_TRACE(Resource)
 {
     visitor->trace(m_loader);
-    visitor->trace(m_resourceToRevalidate);
-    visitor->trace(m_proxyResource);
 #if ENABLE(OILPAN)
     visitor->trace(m_cacheHandler);
 #endif
@@ -227,12 +222,12 @@ void Resource::load(ResourceFetcher* fetcher, const ResourceLoaderOptions& optio
     m_options = options;
     m_loading = true;
 
+    ResourceRequest request(m_revalidatingRequest.isNull() ? m_resourceRequest : m_revalidatingRequest);
     if (!accept().isEmpty())
-        m_resourceRequest.setHTTPAccept(accept());
+        request.setHTTPAccept(accept());
 
     // FIXME: It's unfortunate that the cache layer and below get to know anything about fragment identifiers.
     // We should look into removing the expectation of that knowledge from the platform network stacks.
-    ResourceRequest request(m_resourceRequest);
     if (!m_fragmentIdentifierForRequest.isNull()) {
         KURL url = request.url();
         url.setFragmentIdentifier(m_fragmentIdentifierForRequest);
@@ -241,6 +236,7 @@ void Resource::load(ResourceFetcher* fetcher, const ResourceLoaderOptions& optio
     }
     m_status = Pending;
     if (m_loader) {
+        ASSERT(m_revalidatingRequest.isNull());
         RELEASE_ASSERT(m_options.synchronousPolicy == RequestSynchronously);
         m_loader->changeToSynchronous();
         return;
@@ -262,7 +258,7 @@ void Resource::checkNotify()
 void Resource::appendData(const char* data, unsigned length)
 {
     TRACE_EVENT0("blink", "Resource::appendData");
-    ASSERT(!m_resourceToRevalidate);
+    ASSERT(m_revalidatingRequest.isNull());
     ASSERT(!errorOccurred());
     if (m_options.dataBufferingPolicy == DoNotBufferData)
         return;
@@ -275,7 +271,7 @@ void Resource::appendData(const char* data, unsigned length)
 
 void Resource::setResourceBuffer(PassRefPtr<SharedBuffer> resourceBuffer)
 {
-    ASSERT(!m_resourceToRevalidate);
+    ASSERT(m_revalidatingRequest.isNull());
     ASSERT(!errorOccurred());
     ASSERT(m_options.dataBufferingPolicy == BufferData);
     m_data = resourceBuffer;
@@ -291,8 +287,8 @@ void Resource::setDataBufferingPolicy(DataBufferingPolicy dataBufferingPolicy)
 
 void Resource::error(Resource::Status status)
 {
-    if (m_resourceToRevalidate)
-        revalidationFailed();
+    if (!m_revalidatingRequest.isNull())
+        m_revalidatingRequest = ResourceRequest();

[Nate Chapin, 2015/07/16 23:35:27]

The only difference between this and the original CL is that this was an early
return. I misread the logic of what we did for failed revalidations before this
change and incorrectly replicated it. Currently, we replace the old, valid
resource with the revalidating resource, and when the revalidating resource
fails, we evict it from the cache and there is no resource for the url in the
cache when we are done.

This patch now replicates that behavior, except the part where we remove the
valid resource from the cache and replace it with a new one. We just keep the
same resource, use it for the revalidation, and evict it on failure.

[Yoav Weiss, 2015/07/17 08:57:03]

Do we have tests that caught this logic change?

 
     if (!m_error.isNull() && (m_error.isCancellation() || !isPreloaded()))
         memoryCache()->remove(this);
@@ -313,7 +309,7 @@ void Resource::finishOnePart()
 
 void Resource::finish()
 {
-    ASSERT(!m_resourceToRevalidate);
+    ASSERT(m_revalidatingRequest.isNull());
     ASSERT(!errorOccurred());
     finishOnePart();
     if (!errorOccurred())
@@ -440,7 +436,7 @@ bool Resource::unlock()
     if (!m_data->isLocked())
         return true;
 
-    if (!memoryCache()->contains(this) || hasClients() || m_handleCount > 1 || m_proxyResource || m_resourceToRevalidate || !m_loadFinishTime || !isSafeToUnlock())
+    if (!memoryCache()->contains(this) || hasClients() || m_handleCount > 1 || !m_revalidatingRequest.isNull() || !m_loadFinishTime || !isSafeToUnlock())
         return false;
 
     m_data->unlock();
@@ -454,18 +450,20 @@ bool Resource::hasRightHandleCountApartFromCache(unsigned targetCount) const
 
 void Resource::responseReceived(const ResourceResponse& response, PassOwnPtr<WebDataConsumerHandle>)
 {
-    setResponse(response);
     m_responseTimestamp = currentTime();
+
+    if (!m_revalidatingRequest.isNull()) {
+        if (response.httpStatusCode() == 304) {
+            revalidationSucceeded(response);
+            return;
+        }
+        revalidationFailed();
+    }
+
+    setResponse(response);
     String encoding = response.textEncodingName();
     if (!encoding.isNull())
         setEncoding(encoding);
-
-    if (!m_resourceToRevalidate)
-        return;
-    if (response.httpStatusCode() == 304)
-        revalidationSucceeded(response);
-    else
-        revalidationFailed();
 }
 
 void Resource::setSerializedCachedMetadata(const char* data, size_t size)
@@ -474,7 +472,7 @@ void Resource::setSerializedCachedMetadata(const char* data, size_t size)
     // If this triggers, it indicates an efficiency problem which is most
     // likely unexpected in code designed to improve performance.
     ASSERT(!m_cachedMetadata);
-    ASSERT(!m_resourceToRevalidate);
+    ASSERT(m_revalidatingRequest.isNull());
 
     m_cachedMetadata = CachedMetadata::deserialize(data, size);
 }
@@ -515,7 +513,7 @@ void Resource::clearCachedMetadata(CachedMetadataHandler::CacheType cacheType)
 bool Resource::canDelete() const
 {
     return !hasClients() && !m_loader && !m_preloadCount && hasRightHandleCountApartFromCache(0)
-        && !m_protectorCount && !m_resourceToRevalidate && !m_proxyResource;
+        && !m_protectorCount;
 }
 
 bool Resource::hasOneHandle() const
@@ -582,7 +580,7 @@ bool Resource::addClientToSet(ResourceClient* client)
         memoryCache()->makeLive(this);
 
     // If we have existing data to send to the new client and the resource type supprts it, send it asynchronously.
-    if (!m_response.isNull() && !m_proxyResource && !shouldSendCachedDataSynchronouslyForType(type()) && !m_needsSynchronousCacheHit) {
+    if (!m_response.isNull() && !shouldSendCachedDataSynchronouslyForType(type()) && !m_needsSynchronousCacheHit) {
         m_clientsAwaitingCallback.add(client);
         ResourceCallback::callbackHandler()->schedule(this);
         return false;
@@ -726,88 +724,16 @@ void Resource::prune()
     unlock();
 }
 
-void Resource::setResourceToRevalidate(Resource* resource)
-{
-    ASSERT(resource);
-    ASSERT(!m_resourceToRevalidate);
-    ASSERT(resource != this);
-    ASSERT(m_handlesToRevalidate.isEmpty());
-    ASSERT(resource->type() == type());
-
-    WTF_LOG(ResourceLoading, "Resource %p setResourceToRevalidate %p", this, resource);
-
-    // The following assert should be investigated whenever it occurs. Although it should never fire, it currently does in rare circumstances.
-    // https://bugs.webkit.org/show_bug.cgi?id=28604.
-    // So the code needs to be robust to this assert failing thus the "if (m_resourceToRevalidate->m_proxyResource == this)" in Resource::clearResourceToRevalidate.
-    ASSERT(!resource->m_proxyResource);
-
-    resource->m_proxyResource = this;
-    m_resourceToRevalidate = resource;
-}
-
-void Resource::clearResourceToRevalidate()
-{
-    ASSERT(m_resourceToRevalidate);
-    if (m_switchingClientsToRevalidatedResource)
-        return;
-
-    // A resource may start revalidation before this method has been called, so check that this resource is still the proxy resource before clearing it out.
-    if (m_resourceToRevalidate->m_proxyResource == this) {
-        m_resourceToRevalidate->m_proxyResource = nullptr;
-        m_resourceToRevalidate->deleteIfPossible();
-    }
-    m_handlesToRevalidate.clear();
-    m_resourceToRevalidate = nullptr;
-    deleteIfPossible();
-}
 
-void Resource::switchClientsToRevalidatedResource()
+void Resource::revalidationSucceeded(const ResourceResponse& validatingResponse)
 {
-    ASSERT(m_resourceToRevalidate);
-    ASSERT(memoryCache()->contains(m_resourceToRevalidate));
-    ASSERT(!memoryCache()->contains(this));
-
-    WTF_LOG(ResourceLoading, "Resource %p switchClientsToRevalidatedResource %p", this, m_resourceToRevalidate.get());
-
-    m_resourceToRevalidate->m_identifier = m_identifier;
-
-    m_switchingClientsToRevalidatedResource = true;
-    for (ResourcePtrBase* handle : m_handlesToRevalidate) {
-        handle->m_resource = m_resourceToRevalidate;
-        m_resourceToRevalidate->registerHandle(handle);
-        --m_handleCount;
-    }
-    ASSERT(!m_handleCount);
-    m_handlesToRevalidate.clear();
-
-    Vector<ResourceClient*> clientsToMove;
-    for (const auto& clientHashEntry : m_clients) {
-        unsigned count = clientHashEntry.value;
-        while (count--)
-            clientsToMove.append(clientHashEntry.key);
-    }
-
-    unsigned moveCount = clientsToMove.size();
-    for (unsigned n = 0; n < moveCount; ++n)
-        removeClient(clientsToMove[n]);
-    ASSERT(m_clients.isEmpty());
-
-    for (unsigned n = 0; n < moveCount; ++n)
-        m_resourceToRevalidate->addClientToSet(clientsToMove[n]);
-    for (unsigned n = 0; n < moveCount; ++n) {
-        // Calling didAddClient may do anything, including trying to cancel revalidation.
-        // Assert that it didn't succeed.
-        ASSERT(m_resourceToRevalidate);
-        // Calling didAddClient for a client may end up removing another client. In that case it won't be in the set anymore.
-        if (m_resourceToRevalidate->m_clients.contains(clientsToMove[n]))
-            m_resourceToRevalidate->didAddClient(clientsToMove[n]);
-    }
-    m_switchingClientsToRevalidatedResource = false;
-}
+    // Calling evict() can potentially delete revalidatingResource, which we use
+    // below. This mustn't be the case since revalidation means it is loaded
+    // and so canDelete() is false.
+    ASSERT(!canDelete());
 
-void Resource::updateResponseAfterRevalidation(const ResourceResponse& validatingResponse)
-{
     m_responseTimestamp = currentTime();
+    m_response.setResourceLoadTiming(validatingResponse.resourceLoadTiming());
 
     // RFC2616 10.3.5
     // Update cached headers from the 304 response
@@ -822,42 +748,24 @@ void Resource::updateResponseAfterRevalidation(const ResourceResponse& validatin
             continue;
         m_response.setHTTPHeaderField(header.key, header.value);
     }
-}
-
-void Resource::revalidationSucceeded(const ResourceResponse& response)
-{
-    ASSERT(m_resourceToRevalidate);
-    ASSERT(!memoryCache()->contains(m_resourceToRevalidate));
-    ASSERT(m_resourceToRevalidate->isLoaded());
-
-    // Calling evict() can potentially delete revalidatingResource, which we use
-    // below. This mustn't be the case since revalidation means it is loaded
-    // and so canDelete() is false.
-    ASSERT(!canDelete());
 
-    m_resourceToRevalidate->updateResponseAfterRevalidation(response);
-    memoryCache()->replace(m_resourceToRevalidate, this);
-
-    switchClientsToRevalidatedResource();
     assertAlive();
-    // clearResourceToRevalidate deletes this.
-    clearResourceToRevalidate();
+    m_resourceRequest = m_revalidatingRequest;
+    m_revalidatingRequest = ResourceRequest();
 }
 
 void Resource::revalidationFailed()
 {
-    ASSERT(WTF::isMainThread());
-    WTF_LOG(ResourceLoading, "Revalidation failed for %p", this);
-    ASSERT(resourceToRevalidate());
-    clearResourceToRevalidate();
+    m_resourceRequest = m_revalidatingRequest;
+    m_revalidatingRequest = ResourceRequest();
+    m_data.clear();
+    destroyDecodedDataForFailedRevalidation();
 }
 
 void Resource::registerHandle(ResourcePtrBase* h)
 {
     assertAlive();
     ++m_handleCount;
-    if (m_resourceToRevalidate)
-        m_handlesToRevalidate.add(h);
 }
 
 void Resource::unregisterHandle(ResourcePtrBase* h)
@@ -866,9 +774,6 @@ void Resource::unregisterHandle(ResourcePtrBase* h)
     ASSERT(m_handleCount > 0);
     --m_handleCount;
 
-    if (m_resourceToRevalidate)
-        m_handlesToRevalidate.remove(h);
-
     if (!m_handleCount) {
         if (deleteIfPossible())
             return;