- Revision
- 229560
- Author
- cdu...@apple.com
- Date
- 2018-03-12 16:06:51 -0700 (Mon, 12 Mar 2018)
Log Message
Load may get committed before receiving policy for the resource response
https://bugs.webkit.org/show_bug.cgi?id=183579
<rdar://problem/38268780>
Reviewed by Youenn Fablet.
Source/WebKit:
r228852 updated WebResourceLoader::didReceiveResponse to only send the
ContinueDidReceiveResponse IPC back to the Networkprocess *after* the
policy decision for the resource response has been made. This is necessary
now that policy decisions can be made asynchronously.
However, one of the 2 code paths in NetworkProcess side (code path when
the resource is already in the HTTP disk cache) failed to wait for the
ContinueDidReceiveResponse IPC before sending over the data to the WebProcess.
As a result, the WebProcess could commit the load before even receiving the
policy response from the client.
* NetworkProcess/NetworkResourceLoader.cpp:
(WebKit::NetworkResourceLoader::continueDidReceiveResponse):
(WebKit::NetworkResourceLoader::didRetrieveCacheEntry):
(WebKit::NetworkResourceLoader::continueProcessingCachedEntryAfterDidReceiveResponse):
* NetworkProcess/NetworkResourceLoader.h:
Make sure NetworkResourceLoader::didRetrieveCacheEntry() does not start sending the data
until the network process gets the ContinueDidReceiveResponse IPC back from the WebProcess.
* WebProcess/Network/WebResourceLoader.cpp:
(WebKit::WebResourceLoader::didReceiveResponse):
(WebKit::WebResourceLoader::didReceiveData):
* WebProcess/Network/WebResourceLoader.h:
Add assertion to make sure didReceiveData() never gets called before didReceiveResponse's
completion handler has been called. If this hits, then the load may get committed even
though the client did not reply to the policy for the resource response yet.
LayoutTests:
Add layout test coverage.
* http/tests/cache/cachedEntry-waits-for-response-policy-expected.txt: Added.
* http/tests/cache/cachedEntry-waits-for-response-policy.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (229559 => 229560)
--- trunk/LayoutTests/ChangeLog 2018-03-12 23:01:26 UTC (rev 229559)
+++ trunk/LayoutTests/ChangeLog 2018-03-12 23:06:51 UTC (rev 229560)
@@ -1,3 +1,16 @@
+2018-03-12 Chris Dumez <cdu...@apple.com>
+
+ Load may get committed before receiving policy for the resource response
+ https://bugs.webkit.org/show_bug.cgi?id=183579
+ <rdar://problem/38268780>
+
+ Reviewed by Youenn Fablet.
+
+ Add layout test coverage.
+
+ * http/tests/cache/cachedEntry-waits-for-response-policy-expected.txt: Added.
+ * http/tests/cache/cachedEntry-waits-for-response-policy.html: Added.
+
2018-03-12 Ali Juma <aj...@chromium.org>
http/tests/workers/service/service-worker-download.https.html times out with async policy delegates
Added: trunk/LayoutTests/http/tests/cache/cachedEntry-waits-for-response-policy-expected.txt (0 => 229560)
--- trunk/LayoutTests/http/tests/cache/cachedEntry-waits-for-response-policy-expected.txt (rev 0)
+++ trunk/LayoutTests/http/tests/cache/cachedEntry-waits-for-response-policy-expected.txt 2018-03-12 23:06:51 UTC (rev 229560)
@@ -0,0 +1,3 @@
+This test passes if it does not crash.
+
+
Added: trunk/LayoutTests/http/tests/cache/cachedEntry-waits-for-response-policy.html (0 => 229560)
--- trunk/LayoutTests/http/tests/cache/cachedEntry-waits-for-response-policy.html (rev 0)
+++ trunk/LayoutTests/http/tests/cache/cachedEntry-waits-for-response-policy.html 2018-03-12 23:06:51 UTC (rev 229560)
@@ -0,0 +1,34 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This test passes if it does not crash.</p>
+<script>
+if (window.testRunner) {
+ testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+ if (testRunner.setShouldDecideNavigationPolicyAfterDelay)
+ testRunner.setShouldDecideNavigationPolicyAfterDelay(true);
+ if (testRunner.setShouldDecideResponsePolicyAfterDelay)
+ testRunner.setShouldDecideResponsePolicyAfterDelay(true);
+}
+function createFrame(url) {
+ return new Promise((resolve) => {
+ let frame = document.createElement('iframe');
+ frame.src = ""
+ frame._onload_ = function() { resolve(frame); };
+ document.body.appendChild(frame);
+ });
+}
+
+createFrame("/cache/resources/cacheable-random-text.php").then(function(frame) {
+ frame.remove();
+ internals.clearMemoryCache();
+ createFrame("/cache/resources/cacheable-random-text.php").then(function(frame) {
+ if (window.testRunner)
+ testRunner.notifyDone();
+ });
+});
+
+</script>
+</body>
+</html>
Modified: trunk/Source/WebKit/ChangeLog (229559 => 229560)
--- trunk/Source/WebKit/ChangeLog 2018-03-12 23:01:26 UTC (rev 229559)
+++ trunk/Source/WebKit/ChangeLog 2018-03-12 23:06:51 UTC (rev 229560)
@@ -1,3 +1,38 @@
+2018-03-12 Chris Dumez <cdu...@apple.com>
+
+ Load may get committed before receiving policy for the resource response
+ https://bugs.webkit.org/show_bug.cgi?id=183579
+ <rdar://problem/38268780>
+
+ Reviewed by Youenn Fablet.
+
+ r228852 updated WebResourceLoader::didReceiveResponse to only send the
+ ContinueDidReceiveResponse IPC back to the Networkprocess *after* the
+ policy decision for the resource response has been made. This is necessary
+ now that policy decisions can be made asynchronously.
+
+ However, one of the 2 code paths in NetworkProcess side (code path when
+ the resource is already in the HTTP disk cache) failed to wait for the
+ ContinueDidReceiveResponse IPC before sending over the data to the WebProcess.
+ As a result, the WebProcess could commit the load before even receiving the
+ policy response from the client.
+
+ * NetworkProcess/NetworkResourceLoader.cpp:
+ (WebKit::NetworkResourceLoader::continueDidReceiveResponse):
+ (WebKit::NetworkResourceLoader::didRetrieveCacheEntry):
+ (WebKit::NetworkResourceLoader::continueProcessingCachedEntryAfterDidReceiveResponse):
+ * NetworkProcess/NetworkResourceLoader.h:
+ Make sure NetworkResourceLoader::didRetrieveCacheEntry() does not start sending the data
+ until the network process gets the ContinueDidReceiveResponse IPC back from the WebProcess.
+
+ * WebProcess/Network/WebResourceLoader.cpp:
+ (WebKit::WebResourceLoader::didReceiveResponse):
+ (WebKit::WebResourceLoader::didReceiveData):
+ * WebProcess/Network/WebResourceLoader.h:
+ Add assertion to make sure didReceiveData() never gets called before didReceiveResponse's
+ completion handler has been called. If this hits, then the load may get committed even
+ though the client did not reply to the policy for the resource response yet.
+
2018-03-12 Ali Juma <aj...@chromium.org>
http/tests/workers/service/service-worker-download.https.html times out with async policy delegates
Modified: trunk/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp (229559 => 229560)
--- trunk/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp 2018-03-12 23:01:26 UTC (rev 229559)
+++ trunk/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp 2018-03-12 23:06:51 UTC (rev 229560)
@@ -488,6 +488,11 @@
void NetworkResourceLoader::continueDidReceiveResponse()
{
+ if (m_cacheEntryWaitingForContinueDidReceiveResponse) {
+ continueProcessingCachedEntryAfterDidReceiveResponse(WTFMove(m_cacheEntryWaitingForContinueDidReceiveResponse));
+ return;
+ }
+
// FIXME: Remove this check once BlobResourceHandle implements didReceiveResponseAsync correctly.
// Currently, it does not wait for response, so the load is likely to finish before continueDidReceiveResponse.
if (m_networkLoad)
@@ -563,6 +568,14 @@
bool needsContinueDidReceiveResponseMessage = isMainResource();
send(Messages::WebResourceLoader::DidReceiveResponse(entry->response(), needsContinueDidReceiveResponseMessage));
+ if (needsContinueDidReceiveResponseMessage)
+ m_cacheEntryWaitingForContinueDidReceiveResponse = WTFMove(entry);
+ else
+ continueProcessingCachedEntryAfterDidReceiveResponse(WTFMove(entry));
+}
+
+void NetworkResourceLoader::continueProcessingCachedEntryAfterDidReceiveResponse(std::unique_ptr<NetworkCache::Entry> entry)
+{
if (entry->sourceStorageRecord().bodyHash && !m_parameters.derivedCachedDataTypesToRetrieve.isEmpty()) {
auto bodyHash = *entry->sourceStorageRecord().bodyHash;
auto* entryPtr = entry.release();
Modified: trunk/Source/WebKit/NetworkProcess/NetworkResourceLoader.h (229559 => 229560)
--- trunk/Source/WebKit/NetworkProcess/NetworkResourceLoader.h 2018-03-12 23:01:26 UTC (rev 229559)
+++ trunk/Source/WebKit/NetworkProcess/NetworkResourceLoader.h 2018-03-12 23:06:51 UTC (rev 229560)
@@ -126,6 +126,7 @@
void sendResultForCacheEntry(std::unique_ptr<NetworkCache::Entry>);
void validateCacheEntry(std::unique_ptr<NetworkCache::Entry>);
void dispatchWillSendRequestForCacheEntry(std::unique_ptr<NetworkCache::Entry>);
+ void continueProcessingCachedEntryAfterDidReceiveResponse(std::unique_ptr<NetworkCache::Entry>);
void startNetworkLoad(const WebCore::ResourceRequest&);
void continueDidReceiveResponse();
@@ -174,6 +175,7 @@
RefPtr<WebCore::SharedBuffer> m_bufferedDataForCache;
std::unique_ptr<NetworkCache::Entry> m_cacheEntryForValidation;
bool m_isWaitingContinueWillSendRequestForCachedRedirect { false };
+ std::unique_ptr<NetworkCache::Entry> m_cacheEntryWaitingForContinueDidReceiveResponse;
};
} // namespace WebKit
Modified: trunk/Source/WebKit/WebProcess/Network/WebResourceLoader.cpp (229559 => 229560)
--- trunk/Source/WebKit/WebProcess/Network/WebResourceLoader.cpp 2018-03-12 23:01:26 UTC (rev 229559)
+++ trunk/Source/WebKit/WebProcess/Network/WebResourceLoader.cpp 2018-03-12 23:06:51 UTC (rev 229560)
@@ -114,7 +114,13 @@
CompletionHandler<void()> policyDecisionCompletionHandler;
if (needsContinueDidReceiveResponseMessage) {
+#if !ASSERT_DISABLED
+ m_isProcessingNetworkResponse = true;
+#endif
policyDecisionCompletionHandler = [this, protectedThis = WTFMove(protectedThis)] {
+#if !ASSERT_DISABLED
+ m_isProcessingNetworkResponse = false;
+#endif
// If m_coreLoader becomes null as a result of the didReceiveResponse callback, we can't use the send function().
if (m_coreLoader)
send(Messages::NetworkResourceLoader::ContinueDidReceiveResponse());
@@ -127,6 +133,7 @@
void WebResourceLoader::didReceiveData(const IPC::DataReference& data, int64_t encodedDataLength)
{
LOG(Network, "(WebProcess) WebResourceLoader::didReceiveData of size %lu for '%s'", data.size(), m_coreLoader->url().string().latin1().data());
+ ASSERT_WITH_MESSAGE(!m_isProcessingNetworkResponse, "Network process should not send data until we've validated the response");
if (!m_numBytesReceived) {
RELEASE_LOG_IF_ALLOWED("didReceiveData: Started receiving data (pageID = %" PRIu64 ", frameID = %" PRIu64 ", resourceID = %" PRIu64 ")", m_trackingParameters.pageID, m_trackingParameters.frameID, m_trackingParameters.resourceID);
@@ -149,6 +156,7 @@
LOG(Network, "(WebProcess) WebResourceLoader::didFinishResourceLoad for '%s'", m_coreLoader->url().string().latin1().data());
RELEASE_LOG_IF_ALLOWED("didFinishResourceLoad: (pageID = %" PRIu64 ", frameID = %" PRIu64 ", resourceID = %" PRIu64 ", length = %zd)", m_trackingParameters.pageID, m_trackingParameters.frameID, m_trackingParameters.resourceID, m_numBytesReceived);
+ ASSERT_WITH_MESSAGE(!m_isProcessingNetworkResponse, "Load should not be able to finish before we've validated the response");
m_coreLoader->didFinishLoading(networkLoadMetrics);
}
@@ -157,6 +165,8 @@
LOG(Network, "(WebProcess) WebResourceLoader::didFailResourceLoad for '%s'", m_coreLoader->url().string().latin1().data());
RELEASE_LOG_IF_ALLOWED("didFailResourceLoad: (pageID = %" PRIu64 ", frameID = %" PRIu64 ", resourceID = %" PRIu64 ")", m_trackingParameters.pageID, m_trackingParameters.frameID, m_trackingParameters.resourceID);
+ ASSERT_WITH_MESSAGE(!m_isProcessingNetworkResponse, "Load should not be able to finish before we've validated the response");
+
if (m_coreLoader->documentLoader()->applicationCacheHost().maybeLoadFallbackForError(m_coreLoader.get(), error))
return;
m_coreLoader->didFail(error);
Modified: trunk/Source/WebKit/WebProcess/Network/WebResourceLoader.h (229559 => 229560)
--- trunk/Source/WebKit/WebProcess/Network/WebResourceLoader.h 2018-03-12 23:01:26 UTC (rev 229559)
+++ trunk/Source/WebKit/WebProcess/Network/WebResourceLoader.h 2018-03-12 23:06:51 UTC (rev 229560)
@@ -89,6 +89,10 @@
RefPtr<WebCore::ResourceLoader> m_coreLoader;
TrackingParameters m_trackingParameters;
size_t m_numBytesReceived { 0 };
+
+#if !ASSERT_DISABLED
+ bool m_isProcessingNetworkResponse { false };
+#endif
};
} // namespace WebKit
