Don't keep a separate m_revalidatingRequest on Resource

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 279 matching lines @@
 
     for (const auto& resource : resources)
         resource->finishPendingClients();
 }
 
 Resource::Resource(const ResourceRequest& request, Type type, const ResourceLoaderOptions& options)
     : m_resourceRequest(request)
     , m_options(options)
     , m_responseTimestamp(currentTime())
     , m_cancelTimer(this, &Resource::cancelTimerFired)
+    , m_isRevalidating(false)
     , m_loadFinishTime(0)
     , m_identifier(0)
     , m_encodedSize(0)
     , m_decodedSize(0)
     , m_overheadSize(calculateOverheadSize())
     , m_preloadCount(0)
     , m_cacheIdentifier(MemoryCache::defaultCacheIdentifier())
     , m_preloadResult(PreloadNotReferenced)
     , m_type(type)
     , m_status(NotStarted)
@@ skipping 30 matching lines @@
 {
     // TOOD(japhet): Temporary, out of place hack to stop a top crasher.
     // Make this more organic.
     if (!fetcher->loadingTaskRunner())
         return;
 
     RELEASE_ASSERT(!m_loader);
     ASSERT(stillNeedsLoad());
     m_status = Pending;
 
-    ResourceRequest& request(m_revalidatingRequest.isNull() ? m_resourceRequest : m_revalidatingRequest);
-    KURL url = request.url();
-    request.setAllowStoredCredentials(m_options.allowCredentials == AllowStoredCredentials);
+    KURL url = m_resourceRequest.url();
+    m_resourceRequest.setAllowStoredCredentials(m_options.allowCredentials == AllowStoredCredentials);
 
     m_fetcherSecurityOrigin = fetcher->context().getSecurityOrigin();
     m_loader = ResourceLoader::create(fetcher, this);
-    m_loader->start(request);
-    // If the request reference is null (i.e., a synchronous revalidation will
-    // null the request), don't make the request non-null by setting the url.
-    if (!request.isNull())
-        request.setURL(url);
+    m_loader->start(m_resourceRequest);
+    m_resourceRequest.setURL(url);
 }
 
 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, size_t length)
 {
     TRACE_EVENT0("blink", "Resource::appendData");
-    ASSERT(m_revalidatingRequest.isNull());
+    DCHECK(!m_isRevalidating);
     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_revalidatingRequest.isNull());
+    DCHECK(!m_isRevalidating);
     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::markClientsAndObserversFinished()
 {
     while (!m_clients.isEmpty()) {
         HashCountedSet<ResourceClient*>::iterator it = m_clients.begin();
         for (int i = it->value; i; i--) {
             m_finishedClients.add(it->key);
             m_clients.remove(it);
         }
     }
 }
 
 void Resource::error(const ResourceError& error)
 {
     ASSERT(!error.isNull());
     m_error = error;
-    if (!m_revalidatingRequest.isNull())
-        m_revalidatingRequest = ResourceRequest();
+    m_isRevalidating = false;
 
     if (m_error.isCancellation() || !isPreloaded())
         memoryCache()->remove(this);
 
     setStatus(LoadError);
     ASSERT(errorOccurred());
     m_data.clear();
     m_loader = nullptr;
     checkNotify();
     markClientsAndObserversFinished();
 }
 
 void Resource::finish(double loadFinishTime)
 {
-    ASSERT(m_revalidatingRequest.isNull());
+    DCHECK(!m_isRevalidating);
     m_loadFinishTime = loadFinishTime;
     if (!errorOccurred())
         m_status = Cached;
     m_loader = nullptr;
     checkNotify();
     markClientsAndObserversFinished();
 }
 
 AtomicString Resource::httpContentType() const
 {
@@ skipping 99 matching lines @@
     return currentAge(response, responseTimestamp) <= freshnessLifetime(response, responseTimestamp);
 }
 
 const ResourceRequest& Resource::lastResourceRequest() const
 {
     if (!m_redirectChain.size())
         return m_resourceRequest;
     return m_redirectChain.last().m_request;
 }
 
 void Resource::setRevalidatingRequest(const ResourceRequest& request)

[yhirano, 2016/05/23 11:40:38]

DCHECK(!request.isNull())?

[Nate Chapin, 2016/05/26 21:43:02]

Done.

 {
-    m_revalidatingRequest = request;
+    m_isRevalidating = true;
+    m_resourceRequest = request;
     m_status = NotStarted;
 }
 
 void Resource::willFollowRedirect(ResourceRequest& newRequest, const ResourceResponse& redirectResponse)
 {
     newRequest.setAllowStoredCredentials(m_options.allowCredentials == AllowStoredCredentials);
     m_redirectChain.append(RedirectPair(newRequest, redirectResponse));
 }
 
 void Resource::setResponse(const ResourceResponse& response)
 {
     m_response = response;
     if (m_response.wasFetchedViaServiceWorker())
         m_cacheHandler = ServiceWorkerResponseCachedMetadataHandler::create(this, m_fetcherSecurityOrigin.get());
 }
 
 bool Resource::unlock()
 {
     if (!m_data)
         return false;
 
     if (!m_data->isLocked())
         return true;
 
-    if (!memoryCache()->contains(this) || hasClientsOrObservers() || !m_revalidatingRequest.isNull() || !m_loadFinishTime || !isSafeToUnlock())

[Nate Chapin, 2016/05/19 22:18:39]

Switched this to !isLoaded(), which I think is what these bits are trying to
express.

+    if (!memoryCache()->contains(this) || hasClientsOrObservers() || !isLoaded() || !isSafeToUnlock())
         return false;
 
     m_data->unlock();
     return true;
 }
 
 void Resource::responseReceived(const ResourceResponse& response, PassOwnPtr<WebDataConsumerHandle>)
 {
     m_responseTimestamp = currentTime();
 
-    if (!m_revalidatingRequest.isNull()) {
+    if (m_isRevalidating) {
+        m_isRevalidating = false;
         if (response.httpStatusCode() == 304) {
             revalidationSucceeded(response);
             return;
         }
         revalidationFailed();
     }
     setResponse(response);
     String encoding = response.textEncodingName();
     if (!encoding.isNull())
         setEncoding(encoding);
 }
 
 void Resource::setSerializedCachedMetadata(const char* data, size_t size)
 {
-    ASSERT(m_revalidatingRequest.isNull());
+    DCHECK(!m_isRevalidating);
     ASSERT(!m_response.isNull());
     if (m_cacheHandler)
         m_cacheHandler->setSerializedCachedMetadata(data, size);
 }
 
 CachedMetadataHandler* Resource::cacheHandler()
 {
     return m_cacheHandler.get();
 }
 
@@ skipping 76 matching lines @@
             m_preloadResult = PreloadReferenced;
     }
     if (!hasClientsOrObservers())
         memoryCache()->makeLive(this);
 }
 
 void Resource::addClient(ResourceClient* client)
 {
     willAddClientOrObserver();
 
-    if (!m_revalidatingRequest.isNull()) {
+    if (m_isRevalidating) {
         m_clients.add(client);

[yhirano, 2016/05/23 11:40:38]

Not related to this CL but it looks a |didAddClient| call is missing.

[Nate Chapin, 2016/05/26 21:43:02]

I don't think we want didAddClient here: that would cause the client to get the
data from the old load, not the revalidation.

[yhirano, 2016/05/27 01:55:56]

Ah, I see, thank you.

         return;
     }
 
     // If we have existing data to send to the new client and the resource type supprts it, send it asynchronously.
     if (!m_response.isNull() && !shouldSendCachedDataSynchronouslyForType(getType()) && !m_needsSynchronousCacheHit) {
         m_clientsAwaitingCallback.add(client);
         ResourceCallback::callbackHandler().schedule(this);
         return;
     }
 
@@ skipping 195 matching lines @@
     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);
     }

[yhirano, 2016/05/23 11:40:38]

m_isRevalidating = false;

[Nate Chapin, 2016/05/26 21:43:03]

This is now set in responseReceived(), just before
revalidationSucceeded/revalidationFailed is called.

-
-    m_resourceRequest = m_revalidatingRequest;
-    m_revalidatingRequest = ResourceRequest();
 }
 
 void Resource::revalidationFailed()
 {
-    m_resourceRequest = m_revalidatingRequest;
-    m_revalidatingRequest = ResourceRequest();
     m_redirectChain.clear();

[yhirano, 2016/05/23 11:40:38]

ditto

[Nate Chapin, 2016/05/26 21:43:02]

ditto :)

     m_data.clear();
     m_cacheHandler.clear();
     destroyDecodedDataForFailedRevalidation();
 }
 
 bool Resource::canReuseRedirectChain()
 {
     for (auto& redirect : m_redirectChain) {
         if (!canUseResponse(redirect.m_redirectResponse, m_responseTimestamp))
             return false;
@@ skipping 187 matching lines @@
     case Resource::Media:
         return "Media";
     case Resource::Manifest:
         return "Manifest";
     }
     ASSERT_NOT_REACHED();
     return "Unknown";
 }
 
 } // namespace blink