Diff
Modified: trunk/LayoutTests/ChangeLog (188357 => 188358)
--- trunk/LayoutTests/ChangeLog 2015-08-12 21:01:15 UTC (rev 188357)
+++ trunk/LayoutTests/ChangeLog 2015-08-12 21:07:49 UTC (rev 188358)
@@ -1,3 +1,13 @@
+2015-08-12 Antti Koivisto <[email protected]>
+
+ 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 <[email protected]>
Web Inspector: Not receiving responses for async request IndexedDB.requestDatabaseNames
Added: trunk/LayoutTests/http/tests/cache/recursive-validation-expected.txt (0 => 188358)
--- trunk/LayoutTests/http/tests/cache/recursive-validation-expected.txt (rev 0)
+++ trunk/LayoutTests/http/tests/cache/recursive-validation-expected.txt 2015-08-12 21:07:49 UTC (rev 188358)
@@ -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
+
Added: trunk/LayoutTests/http/tests/cache/recursive-validation.html (0 => 188358)
--- trunk/LayoutTests/http/tests/cache/recursive-validation.html (rev 0)
+++ trunk/LayoutTests/http/tests/cache/recursive-validation.html 2015-08-12 21:07:49 UTC (rev 188358)
@@ -0,0 +1,24 @@
+<script src=""
+<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=""
Added: trunk/LayoutTests/http/tests/cache/resources/no-cache-with-validation.php (0 => 188358)
--- trunk/LayoutTests/http/tests/cache/resources/no-cache-with-validation.php (rev 0)
+++ trunk/LayoutTests/http/tests/cache/resources/no-cache-with-validation.php 2015-08-12 21:07:49 UTC (rev 188358)
@@ -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
Modified: trunk/Source/WebCore/ChangeLog (188357 => 188358)
--- trunk/Source/WebCore/ChangeLog 2015-08-12 21:01:15 UTC (rev 188357)
+++ trunk/Source/WebCore/ChangeLog 2015-08-12 21:07:49 UTC (rev 188358)
@@ -1,3 +1,37 @@
+2015-08-12 Antti Koivisto <[email protected]>
+
+ 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 <[email protected]>
Need to add stubs to enumerateDevices
Modified: trunk/Source/WebCore/loader/cache/CachedRawResource.cpp (188357 => 188358)
--- trunk/Source/WebCore/loader/cache/CachedRawResource.cpp 2015-08-12 21:01:15 UTC (rev 188357)
+++ trunk/Source/WebCore/loader/cache/CachedRawResource.cpp 2015-08-12 21:07:49 UTC (rev 188358)
@@ -141,10 +141,12 @@
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))
Modified: trunk/Source/WebCore/loader/cache/CachedResource.cpp (188357 => 188358)
--- trunk/Source/WebCore/loader/cache/CachedResource.cpp 2015-08-12 21:01:15 UTC (rev 188357)
+++ trunk/Source/WebCore/loader/cache/CachedResource.cpp 2015-08-12 21:07:49 UTC (rev 188358)
@@ -592,29 +592,25 @@
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();
Modified: trunk/Source/WebCore/loader/cache/CachedResource.h (188357 => 188358)
--- trunk/Source/WebCore/loader/cache/CachedResource.h 2015-08-12 21:01:15 UTC (rev 188357)
+++ trunk/Source/WebCore/loader/cache/CachedResource.h 2015-08-12 21:07:49 UTC (rev 188358)
@@ -245,6 +245,7 @@
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 */) { }
Modified: trunk/Source/WebCore/loader/cache/CachedResourceLoader.cpp (188357 => 188358)
--- trunk/Source/WebCore/loader/cache/CachedResourceLoader.cpp 2015-08-12 21:01:15 UTC (rev 188357)
+++ trunk/Source/WebCore/loader/cache/CachedResourceLoader.cpp 2015-08-12 21:07:49 UTC (rev 188358)
@@ -605,7 +605,7 @@
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 @@
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)) {
