Revert of Revalidate using the same Resource, attempt #2

Revert of Revalidate using the same Resource, attempt #2 (patchset #1 id:1 of https://codereview.chromium.org/1237983003/)

Reason for revert:
This caused several different regressions that are not quick and easy to fix.

https://code.google.com/p/chromium/issues/detail?id=512459
https://code.google.com/p/chromium/issues/detail?id=512405
https://code.google.com/p/chromium/issues/detail?id=511939
https://code.google.com/p/chromium/issues/detail?id=513572

Original issue's description:
> Revalidate using the same Resource, attempt #2
> 
> Currently, when we perform a revalidation, we create a new resource and put it in the MemoryCache in place of the resource being revalidated. If the revalidation results in a 304, we silently swap all of the clients of the revalidation to the now-revalidated resource and swap which Resource is in the MemoryCache.
> 
> This changes the behavior to use the existing resource to revalidate itself, which simplifies the logic quite a bit. It also gives us the option of removing ResourcePtr (which is used to implement the silent client swap in the 304 case) and make Resources simply RefCounted in a future change.
> 
> BUG=
> 
> Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199145

TBR=mkwst@chromium.org,yoav@yoav.ws
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=

Committed: https://crrev.com/89cd85cccd999cf98e112821b54bf05660575b3b
git-svn-id: svn://svn.chromium.org/blink/trunk@199677 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[Nate Chapin, 2015-07-29 18:01:52]

Created Revert of Revalidate using the same Resource, attempt #2

[commit-bot: I haz the power, 2015-07-29 18:02:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1269563003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1269563003/1

[commit-bot: I haz the power, 2015-07-29 18:02:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-29 18:02:59]

Failed to apply patch for Source/core/fetch/Resource.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/fetch/Resource.cpp
  Hunk #1 succeeded at 167 (offset -1 lines).
  Hunk #2 succeeded at 192 (offset -1 lines).
  Hunk #3 succeeded at 214 (offset -1 lines).
  Hunk #4 succeeded at 226 (offset -1 lines).
  Hunk #5 succeeded at 240 (offset -1 lines).
  Hunk #6 succeeded at 261 (offset -1 lines).
  Hunk #7 succeeded at 274 (offset -1 lines).
  Hunk #8 succeeded at 290 (offset -1 lines).
  Hunk #9 succeeded at 312 (offset -1 lines).
  Hunk #10 succeeded at 439 (offset -1 lines).
  Hunk #11 succeeded at 453 (offset -1 lines).
  Hunk #12 succeeded at 473 (offset -1 lines).
  Hunk #13 FAILED at 515.
  Hunk #14 succeeded at 580 (offset -2 lines).
  Hunk #15 succeeded at 724 (offset -2 lines).
  Hunk #16 succeeded at 820 (offset -2 lines).
  Hunk #17 succeeded at 863 (offset -2 lines).
  1 out of 17 hunks FAILED -- saving rejects to file
Source/core/fetch/Resource.cpp.rej

Patch:       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
313409c3c87d8806e79808de6a47b7c5067460ad..1987b14e1cd09756f642ee12a8bdc1db9adc4bad
100644
--- a/Source/core/fetch/Resource.cpp
+++ b/Source/core/fetch/Resource.cpp
@@ -168,6 +168,8 @@
 #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);
@@ -191,6 +193,7 @@
 
 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));
@@ -212,6 +215,8 @@
 DEFINE_TRACE(Resource)
 {
     visitor->trace(m_loader);
+    visitor->trace(m_resourceToRevalidate);
+    visitor->trace(m_proxyResource);
 #if ENABLE(OILPAN)
     visitor->trace(m_cacheHandler);
 #endif
@@ -222,12 +227,12 @@
     m_options = options;
     m_loading = true;
 
-    ResourceRequest request(m_revalidatingRequest.isNull() ? m_resourceRequest
: m_revalidatingRequest);
     if (!accept().isEmpty())
-        request.setHTTPAccept(accept());
+        m_resourceRequest.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);
@@ -236,7 +241,6 @@
     }
     m_status = Pending;
     if (m_loader) {
-        ASSERT(m_revalidatingRequest.isNull());
         RELEASE_ASSERT(m_options.synchronousPolicy == RequestSynchronously);
         m_loader->changeToSynchronous();
         return;
@@ -258,7 +262,7 @@
 void Resource::appendData(const char* data, unsigned length)
 {
     TRACE_EVENT0("blink", "Resource::appendData");
-    ASSERT(m_revalidatingRequest.isNull());
+    ASSERT(!m_resourceToRevalidate);
     ASSERT(!errorOccurred());
     if (m_options.dataBufferingPolicy == DoNotBufferData)
         return;
@@ -271,7 +275,7 @@
 
 void Resource::setResourceBuffer(PassRefPtr<SharedBuffer> resourceBuffer)
 {
-    ASSERT(m_revalidatingRequest.isNull());
+    ASSERT(!m_resourceToRevalidate);
     ASSERT(!errorOccurred());
     ASSERT(m_options.dataBufferingPolicy == BufferData);
     m_data = resourceBuffer;
@@ -287,8 +291,8 @@
 
 void Resource::error(Resource::Status status)
 {
-    if (!m_revalidatingRequest.isNull())
-        m_revalidatingRequest = ResourceRequest();
+    if (m_resourceToRevalidate)
+        revalidationFailed();
 
     if (!m_error.isNull() && (m_error.isCancellation() || !isPreloaded()))
         memoryCache()->remove(this);
@@ -309,7 +313,7 @@
 
 void Resource::finish()
 {
-    ASSERT(m_revalidatingRequest.isNull());
+    ASSERT(!m_resourceToRevalidate);
     ASSERT(!errorOccurred());
     finishOnePart();
     if (!errorOccurred())
@@ -436,7 +440,7 @@
     if (!m_data->isLocked())
         return true;
 
-    if (!memoryCache()->contains(this) || hasClients() || m_handleCount > 1 ||
!m_revalidatingRequest.isNull() || !m_loadFinishTime || !isSafeToUnlock())
+    if (!memoryCache()->contains(this) || hasClients() || m_handleCount > 1 ||
m_proxyResource || m_resourceToRevalidate || !m_loadFinishTime ||
!isSafeToUnlock())
         return false;
 
     m_data->unlock();
@@ -450,20 +454,18 @@
 
 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)
@@ -472,7 +474,7 @@
     // If this triggers, it indicates an efficiency problem which is most
     // likely unexpected in code designed to improve performance.
     ASSERT(!m_cachedMetadata);
-    ASSERT(m_revalidatingRequest.isNull());
+    ASSERT(!m_resourceToRevalidate);
 
     m_cachedMetadata = CachedMetadata::deserialize(data, size);
 }
@@ -513,7 +515,7 @@
 bool Resource::canDelete() const
 {
     return !hasClients() && !m_loader && !m_preloadCount &&
hasRightHandleCountApartFromCache(0)
-        && !m_protectorCount;
+        && !m_protectorCount && !m_resourceToRevalidate && !m_proxyResource;
 }
 
 bool Resource::hasOneHandle() const
@@ -580,7 +582,7 @@
         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() &&
!shouldSendCachedDataSynchronouslyForType(type()) &&
!m_needsSynchronousCacheHit) {
+    if (!m_response.isNull() && !m_proxyResource &&
!shouldSendCachedDataSynchronouslyForType(type()) &&
!m_needsSynchronousCacheHit) {
         m_clientsAwaitingCallback.add(client);
         ResourceCallback::callbackHandler()->schedule(this);
         return false;
@@ -724,16 +726,88 @@
     unlock();
 }
 
-
-void Resource::revalidationSucceeded(const ResourceResponse&
validatingResponse)
-{
-    // 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::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()
+{
+    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 anythin…
(message too large)

[commit-bot: I haz the power, 2015-07-29 18:03:06]

The CQ bit was unchecked by commit-bot@chromium.org

[Nate Chapin, 2015-07-29 19:58:47]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2015-07-29 19:58:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1269563003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1269563003/1

[commit-bot: I haz the power, 2015-07-29 19:59:34]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199677

[commit-bot: I haz the power, 2015-09-23 11:54:12]

Patchset 1 (id:??) landed as
https://crrev.com/89cd85cccd999cf98e112821b54bf05660575b3b