Revalidate using the same Resource, attempt #2

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 150 matching lines @@
     , m_requestedFromNetworkingLayer(false)
     , m_loading(false)
     , m_switchingClientsToRevalidatedResource(false)
     , m_type(type)
     , m_status(Pending)
     , m_wasPurged(false)
     , m_needsSynchronousCacheHit(false)
 #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);
 #ifndef NDEBUG
     cachedResourceLeakCounter.increment();
 #endif
     memoryCache()->registerLiveResource(*this);
 
     // Currently we support the metadata caching only for HTTP family.
     if (m_resourceRequest.url().protocolIsInHTTPFamily())
         m_cacheHandler = CacheHandler::create(this);
 
     if (!m_resourceRequest.url().hasFragmentIdentifier())
         return;
     KURL urlForCache = MemoryCache::removeFragmentIdentifierIfNeeded(m_resourceRequest.url());
     if (urlForCache.hasFragmentIdentifier())
         return;
     m_fragmentIdentifierForRequest = m_resourceRequest.url().fragmentIdentifier();
     m_resourceRequest.setURL(urlForCache);
 }
 
 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));
     assertAlive();
 
 #ifdef ENABLE_RESOURCE_IS_DELETED_CHECK
     m_deleted = true;
 #endif
 #ifndef NDEBUG
     cachedResourceLeakCounter.decrement();
 #endif
     InstanceCounters::decrementCounter(InstanceCounters::ResourceCounter);
 }
 
 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
 }
 
 void Resource::load(ResourceFetcher* fetcher, const ResourceLoaderOptions& options)
 {
     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);
         request.setURL(url);
         m_fragmentIdentifierForRequest = String();
     }
     m_status = Pending;
     if (m_loader) {
+        ASSERT(m_revalidatingRequest.isNull());
         RELEASE_ASSERT(m_options.synchronousPolicy == RequestSynchronously);
         m_loader->changeToSynchronous();
         return;
     }
     m_loader = ResourceLoader::create(fetcher, this, request, options);
     m_loader->start();
 }
 
 void Resource::checkNotify()
 {
     if (isLoading())
         return;
 
     ResourceClientWalker<ResourceClient> w(m_clients);
     while (ResourceClient* c = w.next())
         c->notifyFinished(this);
 }
 
 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;
     if (m_data)
         m_data->append(data, length);
     else
         m_data = SharedBuffer::createPurgeable(data, length);
     setEncodedSize(m_data->size());
 }
 
 void Resource::setResourceBuffer(PassRefPtr<SharedBuffer> resourceBuffer)
 {
-    ASSERT(!m_resourceToRevalidate);
+    ASSERT(m_revalidatingRequest.isNull());
     ASSERT(!errorOccurred());
     ASSERT(m_options.dataBufferingPolicy == BufferData);
     m_data = resourceBuffer;
     setEncodedSize(m_data->size());
 }
 
 void Resource::setDataBufferingPolicy(DataBufferingPolicy dataBufferingPolicy)
 {
     m_options.dataBufferingPolicy = dataBufferingPolicy;
     m_data.clear();
     setEncodedSize(0);
 }
 
 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);
 
     setStatus(status);
     ASSERT(errorOccurred());
     m_data.clear();
 
     setLoading(false);
     checkNotify();
 }
 
 void Resource::finishOnePart()
 {
     setLoading(false);
     checkNotify();
 }
 
 void Resource::finish()
 {
-    ASSERT(!m_resourceToRevalidate);
+    ASSERT(m_revalidatingRequest.isNull());
     ASSERT(!errorOccurred());
     finishOnePart();
     if (!errorOccurred())
         m_status = Cached;
 }
 
 bool Resource::passesAccessControlCheck(SecurityOrigin* securityOrigin) const
 {
     String ignoredErrorDescription;
     return passesAccessControlCheck(securityOrigin, ignoredErrorDescription);
@@ skipping 106 matching lines @@
 }
 
 bool Resource::unlock()
 {
     if (!m_data)
         return false;
 
     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();
     return true;
 }
 
 bool Resource::hasRightHandleCountApartFromCache(unsigned targetCount) const
 {
     return m_handleCount == targetCount + (memoryCache()->contains(this) ? 1 : 0);
 }
 
 void Resource::responseReceived(const ResourceResponse& response, PassOwnPtr<WebDataConsumerHandle>)
 {
+    m_responseTimestamp = currentTime();
+
+    if (!m_revalidatingRequest.isNull()) {
+        if (response.httpStatusCode() == 304) {
+            revalidationSucceeded(response);
+            return;
+        }
+        revalidationFailed();
+    }
+
     setResponse(response);
-    m_responseTimestamp = currentTime();
     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)
 {
     // We only expect to receive cached metadata from the platform once.
     // 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);
 }
 
 CachedMetadataHandler* Resource::cacheHandler()
 {
     return m_cacheHandler.get();
 }
 
 void Resource::setCachedMetadata(unsigned dataTypeID, const char* data, size_t size, CachedMetadataHandler::CacheType cacheType)
@@ skipping 20 matching lines @@
 {
     m_cachedMetadata.clear();
 
     if (cacheType == CachedMetadataHandler::SendToPlatform)
         Platform::current()->cacheMetadata(m_response.url(), m_response.responseTime(), 0, 0);
 }
 
 bool Resource::canDelete() const
 {
     return !hasClients() && !m_loader && !m_preloadCount && hasRightHandleCountApartFromCache(0)
-        && !m_protectorCount && !m_resourceToRevalidate && !m_proxyResource;
+        && !m_protectorCount;
 }
 
 bool Resource::hasOneHandle() const
 {
     return hasRightHandleCountApartFromCache(1);
 }
 
 CachedMetadata* Resource::cachedMetadata(unsigned dataTypeID) const
 {
     if (!m_cachedMetadata || m_cachedMetadata->dataTypeID() != dataTypeID)
@@ skipping 46 matching lines @@
             m_preloadResult = PreloadReferencedWhileComplete;
         else if (m_requestedFromNetworkingLayer)
             m_preloadResult = PreloadReferencedWhileLoading;
         else
             m_preloadResult = PreloadReferenced;
     }
     if (!hasClients())
         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;
     }
 
     m_clients.add(client);
     return true;
 }
 
 void Resource::removeClient(ResourceClient* client)
@@ skipping 123 matching lines @@
     // Prevent the case when there are clients waiting but no callback scheduled.
     ASSERT(m_clientsAwaitingCallback.isEmpty() || scheduled);
 }
 
 void Resource::prune()
 {
     destroyDecodedDataIfPossible();
     unlock();
 }
 
-void Resource::setResourceToRevalidate(Resource* resource)
+
+void Resource::revalidationSucceeded(const ResourceResponse& validatingResponse)
 {
-    ASSERT(resource);
-    ASSERT(!m_resourceToRevalidate);
-    ASSERT(resource != this);
-    ASSERT(m_handlesToRevalidate.isEmpty());
-    ASSERT(resource->type() == type());
+    // 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());
 
-    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()
-{
-    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;
-}
-
-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
     const HTTPHeaderMap& newHeaders = validatingResponse.httpHeaderFields();
     for (const auto& header : newHeaders) {
         // Entity headers should not be sent by servers when generating a 304
         // response; misconfigured servers send them anyway. We shouldn't allow
         // such headers to update the original request. We'll base this on the
         // list defined by RFC2616 7.1, with a few additions for extension headers
         // we care about.
         if (!shouldUpdateHeaderAfterRevalidation(header.key))
             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)
 {
     assertAlive();
     ASSERT(m_handleCount > 0);
     --m_handleCount;
 
-    if (m_resourceToRevalidate)
-        m_handlesToRevalidate.remove(h);
-
     if (!m_handleCount) {
         if (deleteIfPossible())
             return;
         unlock();
     } else if (m_handleCount == 1 && memoryCache()->contains(this)) {
         unlock();
         if (!hasClients())
             memoryCache()->prune(this);
     }
 }
@@ skipping 208 matching lines @@
         return "ImportResource";
     case Resource::Media:
         return "Media";
     }
     ASSERT_NOT_REACHED();
     return "Unknown";
 }
 #endif // !LOG_DISABLED
 
 } // namespace blink