From: japhet@chromium.org Date: Wed, 18 Jan 2012 02:06:42 +0000 (+0000) Subject: Ensure we don't cancel revalidation of a CachedResource X-Git-Tag: 070512121124~15146 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=2aae2996d29a485e1b801af3f0b7d33b736ba7e5;p=profile%2Fivi%2Fwebkit-efl.git 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 --- a/Source/WebCore/ChangeLog +++ b/Source/WebCore/ChangeLog @@ -1,3 +1,25 @@ +2012-01-17 Nate Chapin + + 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 NULL ptr in WebCore::RenderSVGInlineText::localCaretRect diff --git a/Source/WebCore/loader/SubresourceLoader.cpp b/Source/WebCore/loader/SubresourceLoader.cpp index 7f40261..76583e8 100644 --- 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 --- 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 --- 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::iterator end = m_handlesToRevalidate.end(); for (HashSet::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 --- 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 --- 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(); }