Ensure we don't cancel revalidation of a CachedResource

in the middle of successful revalidation.
It's more reliable to enforce this in CachedResource than in
SubresourceLoader.
https://bugs.webkit.org/show_bug.cgi?id=75713

Reviewed by Adam Barth.

No new test, the buggy case requires a non-stubbed window.print().

* loader/SubresourceLoader.cpp:
(WebCore::SubresourceLoader::didReceiveResponse):
(WebCore::SubresourceLoader::didFinishLoading):
* loader/SubresourceLoader.h:
* loader/cache/CachedResource.cpp:
(WebCore::CachedResource::CachedResource):
(WebCore::CachedResource::clearResourceToRevalidate):
(WebCore::CachedResource::switchClientsToRevalidatedResource):
* loader/cache/CachedResource.h:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@105226 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 6b98a2d..3acc1e7 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,25 @@
+2012-01-17  Nate Chapin  <japhet@chromium.org>
+
+        Ensure we don't cancel revalidation of a CachedResource
+        in the middle of successful revalidation.
+        It's more reliable to enforce this in CachedResource than in
+        SubresourceLoader.
+        https://bugs.webkit.org/show_bug.cgi?id=75713
+
+        Reviewed by Adam Barth.
+
+        No new test, the buggy case requires a non-stubbed window.print().
+
+        * loader/SubresourceLoader.cpp:
+        (WebCore::SubresourceLoader::didReceiveResponse):
+        (WebCore::SubresourceLoader::didFinishLoading):
+        * loader/SubresourceLoader.h:
+        * loader/cache/CachedResource.cpp:
+        (WebCore::CachedResource::CachedResource):
+        (WebCore::CachedResource::clearResourceToRevalidate):
+        (WebCore::CachedResource::switchClientsToRevalidatedResource):
+        * loader/cache/CachedResource.h:
+
 2012-01-17  Stephen Chenney  <schenney@chromium.org>
 
         NULL ptr in WebCore::RenderSVGInlineText::localCaretRect

diff --git a/Source/WebCore/loader/SubresourceLoader.cpp b/Source/WebCore/loader/SubresourceLoader.cpp
index 7f40261..76583e8 100644 (file)
--- a/Source/WebCore/loader/SubresourceLoader.cpp
+++ b/Source/WebCore/loader/SubresourceLoader.cpp
@@ -175,12 +175,9 @@ void SubresourceLoader::didReceiveResponse(const ResourceResponse& response)
         if (response.httpStatusCode() == 304) {
             // 304 Not modified / Use local copy
             // Existing resource is ok, just use it updating the expiration time.
-            m_state = Revalidating;
             memoryCache()->revalidationSucceeded(m_resource, response);
-            if (!reachedTerminalState()) {
+            if (!reachedTerminalState())
                 ResourceLoader::didReceiveResponse(response);
-                didFinishLoading(monotonicallyIncreasingTime());
-            }
             return;
         }
         // Did not get 304 response, continue as a regular resource load.
@@ -265,7 +262,7 @@ void SubresourceLoader::didReceiveCachedMetadata(const char* data, int length)
 
 void SubresourceLoader::didFinishLoading(double finishTime)
 {
-    if (m_state != Initialized && m_state != Revalidating)
+    if (m_state != Initialized)
         return;
     ASSERT(!reachedTerminalState());
     ASSERT(!m_resource->resourceToRevalidate());

diff --git a/Source/WebCore/loader/SubresourceLoader.h b/Source/WebCore/loader/SubresourceLoader.h
index 3840223..f7ba149 100644 (file)
--- a/Source/WebCore/loader/SubresourceLoader.h
+++ b/Source/WebCore/loader/SubresourceLoader.h
@@ -78,7 +78,6 @@ private:
     enum SubresourceLoaderState {
         Uninitialized,
         Initialized,
-        Revalidating,
         Finishing
     };
 

diff --git a/Source/WebCore/loader/cache/CachedResource.cpp b/Source/WebCore/loader/cache/CachedResource.cpp
index 70935c7..80d373c 100644 (file)
--- a/Source/WebCore/loader/cache/CachedResource.cpp
+++ b/Source/WebCore/loader/cache/CachedResource.cpp
@@ -140,6 +140,7 @@ CachedResource::CachedResource(const ResourceRequest& request, Type type)
     , m_requestedFromNetworkingLayer(false)
     , m_inCache(false)
     , m_loading(false)
+    , m_switchingClientsToRevalidatedResource(false)
     , m_type(type)
     , m_status(Pending)
 #ifndef NDEBUG
@@ -520,6 +521,9 @@ void CachedResource::setResourceToRevalidate(CachedResource* resource)
 void CachedResource::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 = 0;
@@ -538,6 +542,7 @@ void CachedResource::switchClientsToRevalidatedResource()
 
     LOG(ResourceLoading, "CachedResource %p switchClientsToRevalidatedResource %p", this, m_resourceToRevalidate);
 
+    m_switchingClientsToRevalidatedResource = true;
     HashSet<CachedResourceHandleBase*>::iterator end = m_handlesToRevalidate.end();
     for (HashSet<CachedResourceHandleBase*>::iterator it = m_handlesToRevalidate.begin(); it != end; ++it) {
         CachedResourceHandleBase* handle = *it;
@@ -565,10 +570,14 @@ void CachedResource::switchClientsToRevalidatedResource()
     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 CachedResource::updateResponseAfterRevalidation(const ResourceResponse& validatingResponse)

diff --git a/Source/WebCore/loader/cache/CachedResource.h b/Source/WebCore/loader/cache/CachedResource.h
index eac2621..b87da71 100644 (file)
--- a/Source/WebCore/loader/cache/CachedResource.h
+++ b/Source/WebCore/loader/cache/CachedResource.h
@@ -293,6 +293,8 @@ private:
     bool m_inCache : 1;
     bool m_loading : 1;
 
+    bool m_switchingClientsToRevalidatedResource : 1;
+
     unsigned m_type : 4; // Type
     unsigned m_status : 3; // Status
 

diff --git a/Source/WebCore/loader/cache/MemoryCache.cpp b/Source/WebCore/loader/cache/MemoryCache.cpp
index 7de315e..f46889f 100644 (file)
--- a/Source/WebCore/loader/cache/MemoryCache.cpp
+++ b/Source/WebCore/loader/cache/MemoryCache.cpp
@@ -125,6 +125,7 @@ void MemoryCache::revalidationSucceeded(CachedResource* revalidatingResource, co
         adjustSize(resource->hasClients(), delta);
     
     revalidatingResource->switchClientsToRevalidatedResource();
+    ASSERT(!revalidatingResource->m_deleted);
     // this deletes the revalidating resource
     revalidatingResource->clearResourceToRevalidate();
 }