CachedResource leak in validation code
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 12 Aug 2015 21:07:49 +0000 (21:07 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 12 Aug 2015 21:07:49 +0000 (21:07 +0000)
https://bugs.webkit.org/show_bug.cgi?id=147941

Reviewed by Chris Dumez.

Source/WebCore:

While adding test coverage I discovered a way to hit ASSERT(!resource->m_proxyResource) in CachedResource::setResourceToRevalidate.
I think this ends up leaking a resource too.

Test: http/tests/cache/recursive-validation.html

* loader/cache/CachedRawResource.cpp:
(WebCore::CachedRawResource::didAddClient):

    Tighten the condition.

* loader/cache/CachedResource.cpp:
(WebCore::CachedResource::setResourceToRevalidate):
(WebCore::CachedResource::clearResourceToRevalidate):

    Replace workaround for this bug with an assert.

* loader/cache/CachedResource.h:
(WebCore::CachedResource::validationInProgress):
(WebCore::CachedResource::validationCompleting):
(WebCore::CachedResource::didSendData):
* loader/cache/CachedResourceLoader.cpp:
(WebCore::CachedResourceLoader::revalidateResource):
(WebCore::CachedResourceLoader::determineRevalidationPolicy):

    Fix the bug by using (instead of revalidating) resource that we are just finishing revalidating.
    This can happen when a succesful revalidation synchronously triggers another load for the same resource.

LayoutTests:

* http/tests/cache/recursive-validation.html: Added.
* http/tests/cache/resources/no-cache-with-validation.php: Added.

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

LayoutTests/ChangeLog
LayoutTests/http/tests/cache/recursive-validation-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/cache/recursive-validation.html [new file with mode: 0644]
LayoutTests/http/tests/cache/resources/no-cache-with-validation.php [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/loader/cache/CachedRawResource.cpp
Source/WebCore/loader/cache/CachedResource.cpp
Source/WebCore/loader/cache/CachedResource.h
Source/WebCore/loader/cache/CachedResourceLoader.cpp

index e3ff599..9cb6411 100644 (file)
@@ -1,3 +1,13 @@
+2015-08-12  Antti Koivisto  <antti@apple.com>
+
+        CachedResource leak in validation code
+        https://bugs.webkit.org/show_bug.cgi?id=147941
+
+        Reviewed by Chris Dumez.
+
+        * http/tests/cache/recursive-validation.html: Added.
+        * http/tests/cache/resources/no-cache-with-validation.php: Added.
+
 2015-08-12  Joseph Pecoraro  <pecoraro@apple.com>
 
         Web Inspector: Not receiving responses for async request IndexedDB.requestDatabaseNames
diff --git a/LayoutTests/http/tests/cache/recursive-validation-expected.txt b/LayoutTests/http/tests/cache/recursive-validation-expected.txt
new file mode 100644 (file)
index 0000000..37c9f33
--- /dev/null
@@ -0,0 +1,9 @@
+Test that recursively loading resource that requires validation does not assert or crash
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/http/tests/cache/recursive-validation.html b/LayoutTests/http/tests/cache/recursive-validation.html
new file mode 100644 (file)
index 0000000..3c92f99
--- /dev/null
@@ -0,0 +1,24 @@
+<script src="/js-test-resources/js-test-pre.js"></script>
+<script>
+jsTestIsAsync = true;
+
+description("Test that recursively loading resource that requires validation does not assert or crash");
+
+function load(completion) {
+    var xhr = new XMLHttpRequest();
+    xhr.open('GET', "resources/no-cache-with-validation.php");
+    xhr.onload = completion;
+    xhr.send();
+}
+
+window.onload = load(function () {
+    load(function () {
+        load(function () {
+            load(function () {
+                 finishJSTest();
+            });
+        });
+    });
+});
+</script>
+<script src="/js-test-resources/js-test-post.js"></script>
diff --git a/LayoutTests/http/tests/cache/resources/no-cache-with-validation.php b/LayoutTests/http/tests/cache/resources/no-cache-with-validation.php
new file mode 100644 (file)
index 0000000..eb81bfb
--- /dev/null
@@ -0,0 +1,10 @@
+<?php
+if ($_SERVER["HTTP_IF_MODIFIED_SINCE"]) {
+    header("HTTP/1.0 304 Not Modified");
+    exit();
+}
+header("Cache-control: no-cache");
+header("Last-Modified: Thu, 01 Jan 1970 00:00:00 GMT");
+header("Content-Type: text/plain");
+?>
+foo
index 2b2ad02..3164654 100644 (file)
@@ -1,3 +1,37 @@
+2015-08-12  Antti Koivisto  <antti@apple.com>
+
+        CachedResource leak in validation code
+        https://bugs.webkit.org/show_bug.cgi?id=147941
+
+        Reviewed by Chris Dumez.
+
+        While adding test coverage I discovered a way to hit ASSERT(!resource->m_proxyResource) in CachedResource::setResourceToRevalidate.
+        I think this ends up leaking a resource too.
+
+        Test: http/tests/cache/recursive-validation.html
+
+        * loader/cache/CachedRawResource.cpp:
+        (WebCore::CachedRawResource::didAddClient):
+
+            Tighten the condition.
+
+        * loader/cache/CachedResource.cpp:
+        (WebCore::CachedResource::setResourceToRevalidate):
+        (WebCore::CachedResource::clearResourceToRevalidate):
+
+            Replace workaround for this bug with an assert.
+
+        * loader/cache/CachedResource.h:
+        (WebCore::CachedResource::validationInProgress):
+        (WebCore::CachedResource::validationCompleting):
+        (WebCore::CachedResource::didSendData):
+        * loader/cache/CachedResourceLoader.cpp:
+        (WebCore::CachedResourceLoader::revalidateResource):
+        (WebCore::CachedResourceLoader::determineRevalidationPolicy):
+
+            Fix the bug by using (instead of revalidating) resource that we are just finishing revalidating.
+            This can happen when a succesful revalidation synchronously triggers another load for the same resource.
+
 2015-08-12  Matthew Daiter  <mdaiter@apple.com>
 
         Need to add stubs to enumerateDevices
index f5f5823..c78c3d4 100644 (file)
@@ -141,10 +141,12 @@ void CachedRawResource::didAddClient(CachedResourceClient* c)
 
     if (!m_response.isNull()) {
         ResourceResponse response(m_response);
-        if (validationInProgress())
+        if (validationCompleting())
             response.setSource(ResourceResponse::Source::MemoryCacheAfterValidation);
-        else
+        else {
+            ASSERT(!validationInProgress());
             response.setSource(ResourceResponse::Source::MemoryCache);
+        }
         client->responseReceived(this, response);
     }
     if (!hasClient(c))
index 0f1837d..da44cc8 100644 (file)
@@ -592,29 +592,25 @@ void CachedResource::setResourceToRevalidate(CachedResource* resource)
     ASSERT(resource != this);
     ASSERT(m_handlesToRevalidate.isEmpty());
     ASSERT(resource->type() == type());
+    ASSERT(!resource->m_proxyResource);
 
     LOG(ResourceLoading, "CachedResource %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 CachedResource::clearResourceToRevalidate.
-    ASSERT(!resource->m_proxyResource);
-
     resource->m_proxyResource = this;
     m_resourceToRevalidate = resource;
 }
 
 void CachedResource::clearResourceToRevalidate() 
-{ 
+{
     ASSERT(m_resourceToRevalidate);
+    ASSERT(m_resourceToRevalidate->m_proxyResource == this);
+
     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;
-        m_resourceToRevalidate->deleteIfPossible();
-    }
+    m_resourceToRevalidate->m_proxyResource = nullptr;
+    m_resourceToRevalidate->deleteIfPossible();
+
     m_handlesToRevalidate.clear();
     m_resourceToRevalidate = 0;
     deleteIfPossible();
index da846e2..01ebb9b 100644 (file)
@@ -245,6 +245,7 @@ public:
     void clearResourceToRevalidate();
     void updateResponseAfterRevalidation(const ResourceResponse& validatingResponse);
     bool validationInProgress() const { return m_proxyResource; }
+    bool validationCompleting() const { return m_proxyResource && m_proxyResource->m_switchingClientsToRevalidatedResource; }
 
     virtual void didSendData(unsigned long long /* bytesSent */, unsigned long long /* totalBytesToBeSent */) { }
 
index 14b9547..e639dfc 100644 (file)
@@ -605,7 +605,7 @@ CachedResourceHandle<CachedResource> CachedResourceLoader::revalidateResource(co
     ASSERT(resource->sessionID() == sessionID());
 
     CachedResourceHandle<CachedResource> newResource = createResource(resource->type(), resource->resourceRequest(), resource->encoding(), resource->sessionID());
-    
+
     LOG(ResourceLoading, "Resource %p created to revalidate %p", newResource.get(), resource);
     newResource->setResourceToRevalidate(resource);
     
@@ -711,10 +711,15 @@ CachedResourceLoader::RevalidationPolicy CachedResourceLoader::determineRevalida
     if (m_allowStaleResources)
         return Use;
     
-    // Alwaus use preloads.
+    // Always use preloads.
     if (existingResource->isPreloaded())
         return Use;
 
+    // We can find resources that are being validated from cache only when validation is just successfully completing.
+    if (existingResource->validationCompleting())
+        return Use;
+    ASSERT(!existingResource->validationInProgress());
+
     // Validate the redirect chain.
     bool cachePolicyIsHistoryBuffer = cachePolicy(type) == CachePolicyHistoryBuffer;
     if (!existingResource->redirectChainAllowsReuse(cachePolicyIsHistoryBuffer ? ReuseExpiredRedirection : DoNotReuseExpiredRedirection)) {