- Revision
- 199521
- Author
- cdu...@apple.com
- Date
- 2016-04-13 17:14:05 -0700 (Wed, 13 Apr 2016)
Log Message
We should not speculatively revalidate cached redirects
https://bugs.webkit.org/show_bug.cgi?id=156548
<rdar://problem/25583886>
Reviewed by Darin Adler.
Source/WebKit2:
Stop speculatively revalidating cached redirects. This matches matches
the behavior in NetworkCache's makeUseDecision() which reuses cached
redirects only if they do not need revalidation.
This was breaking fonts.css loading on stripe.com because the
SpeculativeLoadManager would wrongly speculatively revalidate the
redirect and then serve a 302 response the NetworkResourceLoader
when the actual request came in. This would cause us to not follow
the redirect.
* NetworkProcess/cache/NetworkCacheSpeculativeLoad.cpp:
(WebKit::NetworkCache::SpeculativeLoad::willSendRedirectedRequest):
Abort the speculative load if it hits a redirect. This is the safe thing
to do in this case, as we are supposed to do a hand-shake with WebCore
in such case.
(WebKit::NetworkCache::SpeculativeLoad::didReceiveResponse):
Let successful validations fall through instead of calling didComplete()
early. This matches what is not in NetworkResourceLoader. This way,
didFinishLoading() ends up getting called for both successful and
unsuccessful (i.e. did not return a 302 status code) network validation.
(WebKit::NetworkCache::SpeculativeLoad::didFinishLoading):
- Stop dealing with redirects as we abort the load as soon as we hit a
redirect now.
- Stop asserting that m_cacheEntryForValidation is null now that this
is called for successful validations as well.
(WebKit::NetworkCache::SpeculativeLoad::abort):
New method that aborts the network loads, calls the completion handler
and clean up. It is called in the case we hit a redirect while
revalidating.
* NetworkProcess/cache/NetworkCacheSpeculativeLoad.h:
Drop m_redirectChainCacheStatus member as we no longer deal with
redirects.
* NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp:
(WebKit::NetworkCache::SpeculativeLoadManager::retrieveEntryFromStorage):
If the resource needs revalidation AND is a cached redirect, then do not
use it. This matches what is done in NetworkCache's makeUseDecision().
Tools:
Re-enable speculative loading in the context of layout tests. This was
turned off by mistake when speculative loading was turned into a
setting recently.
* WebKitTestRunner/TestController.cpp:
(WTR::TestController::generatePageConfiguration):
LayoutTests:
Add layout test to make sure that speculative loading does not break
redirects. This replicates the issue seen with fonts.css on stripe.com.
* http/tests/cache/disk-cache/speculative-validation/cacheable-redirect-expected.txt: Added.
* http/tests/cache/disk-cache/speculative-validation/cacheable-redirect.html: Added.
* http/tests/cache/disk-cache/speculative-validation/resources/cacheable-redirect-frame.php: Added.
* http/tests/cache/disk-cache/speculative-validation/resources/css-to-revalidate.php: Added.
* http/tests/cache/disk-cache/speculative-validation/resources/redirect-to-css.php: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (199520 => 199521)
--- trunk/LayoutTests/ChangeLog 2016-04-14 00:13:52 UTC (rev 199520)
+++ trunk/LayoutTests/ChangeLog 2016-04-14 00:14:05 UTC (rev 199521)
@@ -1,3 +1,20 @@
+2016-04-13 Chris Dumez <cdu...@apple.com>
+
+ We should not speculatively revalidate cached redirects
+ https://bugs.webkit.org/show_bug.cgi?id=156548
+ <rdar://problem/25583886>
+
+ Reviewed by Darin Adler.
+
+ Add layout test to make sure that speculative loading does not break
+ redirects. This replicates the issue seen with fonts.css on stripe.com.
+
+ * http/tests/cache/disk-cache/speculative-validation/cacheable-redirect-expected.txt: Added.
+ * http/tests/cache/disk-cache/speculative-validation/cacheable-redirect.html: Added.
+ * http/tests/cache/disk-cache/speculative-validation/resources/cacheable-redirect-frame.php: Added.
+ * http/tests/cache/disk-cache/speculative-validation/resources/css-to-revalidate.php: Added.
+ * http/tests/cache/disk-cache/speculative-validation/resources/redirect-to-css.php: Added.
+
2016-04-13 Zalan Bujtas <za...@apple.com>
Text on compositing layer with negative letter-spacing is truncated.
Added: trunk/LayoutTests/http/tests/cache/disk-cache/speculative-validation/cacheable-redirect-expected.txt (0 => 199521)
--- trunk/LayoutTests/http/tests/cache/disk-cache/speculative-validation/cacheable-redirect-expected.txt (rev 0)
+++ trunk/LayoutTests/http/tests/cache/disk-cache/speculative-validation/cacheable-redirect-expected.txt 2016-04-14 00:14:05 UTC (rev 199521)
@@ -0,0 +1,10 @@
+Tests that speculative validation does not break redirects.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS frames[0].getComputedStyle(testDiv).color is "rgb(0, 128, 0)"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/http/tests/cache/disk-cache/speculative-validation/cacheable-redirect.html (0 => 199521)
--- trunk/LayoutTests/http/tests/cache/disk-cache/speculative-validation/cacheable-redirect.html (rev 0)
+++ trunk/LayoutTests/http/tests/cache/disk-cache/speculative-validation/cacheable-redirect.html 2016-04-14 00:14:05 UTC (rev 199521)
@@ -0,0 +1,32 @@
+<script src=""
+<script>
+description("Tests that speculative validation does not break redirects.");
+jsTestIsAsync = true;
+
+state = "warmup";
+
+function frameLoaded()
+{
+ if (state == "warmup") {
+ state = "flushingMetadata";
+ document.getElementById("testFrame").src = ""
+ return;
+ }
+ if (state == "flushingMetadata") {
+ // Navigate frame to its original location again. This time it should speculatively
+ // validate subresources as we have subresource loads metadata in the disk cache.
+ state = "speculativeRevalidation";
+ document.getElementById("testFrame").src = ""
+ return;
+ }
+ if (state == "speculativeRevalidation") {
+ testDiv = frames[0].document.getElementById("testDiv");
+ shouldBeEqualToString("frames[0].getComputedStyle(testDiv).color", "rgb(0, 128, 0)");
+ finishJSTest();
+ return;
+ }
+}
+
+</script>
+<iframe id="testFrame" src="" _onload_="frameLoaded()"></iframe>
+<script src=""
Added: trunk/LayoutTests/http/tests/cache/disk-cache/speculative-validation/resources/cacheable-redirect-frame.php (0 => 199521)
--- trunk/LayoutTests/http/tests/cache/disk-cache/speculative-validation/resources/cacheable-redirect-frame.php (rev 0)
+++ trunk/LayoutTests/http/tests/cache/disk-cache/speculative-validation/resources/cacheable-redirect-frame.php 2016-04-14 00:14:05 UTC (rev 199521)
@@ -0,0 +1,17 @@
+<?php
+header('Content-Type: text/html');
+header('Cache-Control: max-age=0');
+header('Etag: 123456789');
+
+?>
+<!DOCTYPE html>
+<html>
+<head>
+<link rel="stylesheet" type="text/css" href=""
+</head>
+<body>
+<div id="testDiv" class="testClass">
+TEST
+</div>
+</body>
+</html>
Added: trunk/LayoutTests/http/tests/cache/disk-cache/speculative-validation/resources/css-to-revalidate.php (0 => 199521)
--- trunk/LayoutTests/http/tests/cache/disk-cache/speculative-validation/resources/css-to-revalidate.php (rev 0)
+++ trunk/LayoutTests/http/tests/cache/disk-cache/speculative-validation/resources/css-to-revalidate.php 2016-04-14 00:14:05 UTC (rev 199521)
@@ -0,0 +1,17 @@
+<?php
+if ($_SERVER["HTTP_IF_NONE_MATCH"] == "foo") {
+ header("HTTP/1.1 304 Not Modified");
+ header("ETag: foo");
+ return;
+}
+
+header("Content-Type: text/css");
+header("ETag: foo");
+header("Cache-Control: max-age=0");
+
+?>
+
+.testClass
+{
+ color: green;
+}
Added: trunk/LayoutTests/http/tests/cache/disk-cache/speculative-validation/resources/redirect-to-css.php (0 => 199521)
--- trunk/LayoutTests/http/tests/cache/disk-cache/speculative-validation/resources/redirect-to-css.php (rev 0)
+++ trunk/LayoutTests/http/tests/cache/disk-cache/speculative-validation/resources/redirect-to-css.php 2016-04-14 00:14:05 UTC (rev 199521)
@@ -0,0 +1,6 @@
+<?php
+header('HTTP/1.1 302 Found');
+header('Location: /cache/disk-cache/speculative-validation/resources/css-to-revalidate.php');
+header('Expires: Thu, 01 Dec 2003 16:00:00 GMT');
+header('ETag: foo');
+?>
Modified: trunk/Source/WebKit2/ChangeLog (199520 => 199521)
--- trunk/Source/WebKit2/ChangeLog 2016-04-14 00:13:52 UTC (rev 199520)
+++ trunk/Source/WebKit2/ChangeLog 2016-04-14 00:14:05 UTC (rev 199521)
@@ -1,3 +1,53 @@
+2016-04-13 Chris Dumez <cdu...@apple.com>
+
+ We should not speculatively revalidate cached redirects
+ https://bugs.webkit.org/show_bug.cgi?id=156548
+ <rdar://problem/25583886>
+
+ Reviewed by Darin Adler.
+
+ Stop speculatively revalidating cached redirects. This matches matches
+ the behavior in NetworkCache's makeUseDecision() which reuses cached
+ redirects only if they do not need revalidation.
+
+ This was breaking fonts.css loading on stripe.com because the
+ SpeculativeLoadManager would wrongly speculatively revalidate the
+ redirect and then serve a 302 response the NetworkResourceLoader
+ when the actual request came in. This would cause us to not follow
+ the redirect.
+
+ * NetworkProcess/cache/NetworkCacheSpeculativeLoad.cpp:
+ (WebKit::NetworkCache::SpeculativeLoad::willSendRedirectedRequest):
+ Abort the speculative load if it hits a redirect. This is the safe thing
+ to do in this case, as we are supposed to do a hand-shake with WebCore
+ in such case.
+
+ (WebKit::NetworkCache::SpeculativeLoad::didReceiveResponse):
+ Let successful validations fall through instead of calling didComplete()
+ early. This matches what is not in NetworkResourceLoader. This way,
+ didFinishLoading() ends up getting called for both successful and
+ unsuccessful (i.e. did not return a 302 status code) network validation.
+
+ (WebKit::NetworkCache::SpeculativeLoad::didFinishLoading):
+ - Stop dealing with redirects as we abort the load as soon as we hit a
+ redirect now.
+ - Stop asserting that m_cacheEntryForValidation is null now that this
+ is called for successful validations as well.
+
+ (WebKit::NetworkCache::SpeculativeLoad::abort):
+ New method that aborts the network loads, calls the completion handler
+ and clean up. It is called in the case we hit a redirect while
+ revalidating.
+
+ * NetworkProcess/cache/NetworkCacheSpeculativeLoad.h:
+ Drop m_redirectChainCacheStatus member as we no longer deal with
+ redirects.
+
+ * NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp:
+ (WebKit::NetworkCache::SpeculativeLoadManager::retrieveEntryFromStorage):
+ If the resource needs revalidation AND is a cached redirect, then do not
+ use it. This matches what is done in NetworkCache's makeUseDecision().
+
2016-04-13 Brady Eidson <beid...@apple.com>
Modern IDB: NetworkProcessConnection::didClose needs to have a self ref.
Modified: trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoad.cpp (199520 => 199521)
--- trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoad.cpp 2016-04-14 00:13:52 UTC (rev 199520)
+++ trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoad.cpp 2016-04-14 00:14:05 UTC (rev 199521)
@@ -65,8 +65,9 @@
void SpeculativeLoad::willSendRedirectedRequest(const ResourceRequest&, const ResourceRequest& redirectRequest, const ResourceResponse& redirectResponse)
{
- updateRedirectChainStatus(m_redirectChainCacheStatus, redirectResponse);
- m_networkLoad->continueWillSendRequest(redirectRequest);
+ LOG(NetworkCacheSpeculativePreloading, "(NetworkProcess) Speculative revalidation for %s hit a redirect, aborting the load.", redirectResponse.url().string().utf8().data());
+ // We drop speculative revalidations if they redirect for now as we would need to notify WebCore of such redirects.
+ abort();
}
auto SpeculativeLoad::didReceiveResponse(const ResourceResponse& receivedResponse) -> ShouldContinueDidReceiveResponse
@@ -79,14 +80,11 @@
ASSERT(m_cacheEntryForValidation);
bool validationSucceeded = m_response.httpStatusCode() == 304; // 304 Not Modified
- if (validationSucceeded) {
+ if (validationSucceeded)
m_cacheEntryForValidation = NetworkCache::singleton().update(m_originalRequest, m_frameID, *m_cacheEntryForValidation, m_response);
- didComplete();
- return ShouldContinueDidReceiveResponse::No;
- }
+ else
+ m_cacheEntryForValidation = nullptr;
- m_cacheEntryForValidation = nullptr;
-
return ShouldContinueDidReceiveResponse::Yes;
}
@@ -106,23 +104,8 @@
void SpeculativeLoad::didFinishLoading(double finishTime)
{
- ASSERT(!m_cacheEntryForValidation);
-
- bool allowStale = m_originalRequest.cachePolicy() >= ReturnCacheDataElseLoad;
- bool hasCacheableRedirect = m_response.isHTTP() && redirectChainAllowsReuse(m_redirectChainCacheStatus, allowStale ? ReuseExpiredRedirection : DoNotReuseExpiredRedirection);
- if (hasCacheableRedirect && m_redirectChainCacheStatus.status == RedirectChainCacheStatus::CachedRedirection) {
- // Maybe we should cache the actual redirects instead of the end result?
- auto now = std::chrono::system_clock::now();
- auto responseEndOfValidity = now + computeFreshnessLifetimeForHTTPFamily(m_response, now) - computeCurrentAge(m_response, now);
- hasCacheableRedirect = responseEndOfValidity <= m_redirectChainCacheStatus.endOfValidity;
- }
-
- if (m_bufferedDataForCache && hasCacheableRedirect)
+ if (!m_cacheEntryForValidation && m_bufferedDataForCache)
m_cacheEntryForValidation = NetworkCache::singleton().store(m_originalRequest, m_response, WTFMove(m_bufferedDataForCache), [](NetworkCache::MappedBody& mappedBody) { });
- else if (!hasCacheableRedirect) {
- // Make sure we don't keep a stale entry in the cache.
- NetworkCache::singleton().remove(m_originalRequest);
- }
didComplete();
}
@@ -134,6 +117,15 @@
didComplete();
}
+void SpeculativeLoad::abort()
+{
+ if (m_networkLoad)
+ m_networkLoad->cancel();
+
+ m_cacheEntryForValidation = nullptr;
+ didComplete();
+}
+
void SpeculativeLoad::didComplete()
{
RELEASE_ASSERT(RunLoop::isMain());
Modified: trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoad.h (199520 => 199521)
--- trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoad.h 2016-04-14 00:13:52 UTC (rev 199520)
+++ trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoad.h 2016-04-14 00:14:05 UTC (rev 199521)
@@ -65,6 +65,7 @@
void didBecomeDownload() override { ASSERT_NOT_REACHED(); }
#endif
+ void abort();
void didComplete();
GlobalFrameID m_frameID;
@@ -77,8 +78,6 @@
RefPtr<WebCore::SharedBuffer> m_bufferedDataForCache;
std::unique_ptr<NetworkCache::Entry> m_cacheEntryForValidation;
-
- WebCore::RedirectChainCacheStatus m_redirectChainCacheStatus;
};
} // namespace NetworkCache
Modified: trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp (199520 => 199521)
--- trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp 2016-04-14 00:13:52 UTC (rev 199520)
+++ trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp 2016-04-14 00:14:05 UTC (rev 199521)
@@ -425,8 +425,14 @@
return true;
}
- if (responseNeedsRevalidation(response, entry->timeStamp()))
+ if (responseNeedsRevalidation(response, entry->timeStamp())) {
+ // Do not use cached redirects that have expired.
+ if (entry->redirectRequest()) {
+ completionHandler(nullptr);
+ return true;
+ }
entry->setNeedsValidation(true);
+ }
completionHandler(WTFMove(entry));
return true;
Modified: trunk/Tools/ChangeLog (199520 => 199521)
--- trunk/Tools/ChangeLog 2016-04-14 00:13:52 UTC (rev 199520)
+++ trunk/Tools/ChangeLog 2016-04-14 00:14:05 UTC (rev 199521)
@@ -1,3 +1,18 @@
+2016-04-13 Chris Dumez <cdu...@apple.com>
+
+ We should not speculatively revalidate cached redirects
+ https://bugs.webkit.org/show_bug.cgi?id=156548
+ <rdar://problem/25583886>
+
+ Reviewed by Darin Adler.
+
+ Re-enable speculative loading in the context of layout tests. This was
+ turned off by mistake when speculative loading was turned into a
+ setting recently.
+
+ * WebKitTestRunner/TestController.cpp:
+ (WTR::TestController::generatePageConfiguration):
+
2016-04-12 Alexey Proskuryakov <a...@apple.com>
Python test webkitpy.common.system.executive_unittest.ExecutiveTest.serial_test_kill_process is flaky
Modified: trunk/Tools/WebKitTestRunner/TestController.cpp (199520 => 199521)
--- trunk/Tools/WebKitTestRunner/TestController.cpp 2016-04-14 00:13:52 UTC (rev 199520)
+++ trunk/Tools/WebKitTestRunner/TestController.cpp 2016-04-14 00:14:05 UTC (rev 199521)
@@ -426,6 +426,7 @@
WKContextSetIconDatabasePath(m_context.get(), toWK(emptyString()).get());
}
+ WKContextSetDiskCacheSpeculativeValidationEnabled(m_context.get(), true);
WKContextUseTestingNetworkSession(m_context.get());
WKContextSetCacheModel(m_context.get(), kWKCacheModelDocumentBrowser);
