Load may get committed before receiving policy for the resource response
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 12 Mar 2018 23:06:51 +0000 (23:06 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 12 Mar 2018 23:06:51 +0000 (23:06 +0000)
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.

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

LayoutTests/ChangeLog
LayoutTests/http/tests/cache/cachedEntry-waits-for-response-policy-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/cache/cachedEntry-waits-for-response-policy.html [new file with mode: 0644]
Source/WebKit/ChangeLog
Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
Source/WebKit/NetworkProcess/NetworkResourceLoader.h
Source/WebKit/WebProcess/Network/WebResourceLoader.cpp
Source/WebKit/WebProcess/Network/WebResourceLoader.h

index 88712b4..1b93565 100644 (file)
@@ -1,3 +1,16 @@
+2018-03-12  Chris Dumez  <cdumez@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  <ajuma@chromium.org>
 
         http/tests/workers/service/service-worker-download.https.html times out with async policy delegates
diff --git a/LayoutTests/http/tests/cache/cachedEntry-waits-for-response-policy-expected.txt b/LayoutTests/http/tests/cache/cachedEntry-waits-for-response-policy-expected.txt
new file mode 100644 (file)
index 0000000..b512692
--- /dev/null
@@ -0,0 +1,3 @@
+This test passes if it does not crash.
+
diff --git a/LayoutTests/http/tests/cache/cachedEntry-waits-for-response-policy.html b/LayoutTests/http/tests/cache/cachedEntry-waits-for-response-policy.html
new file mode 100644 (file)
index 0000000..e52b8aa
--- /dev/null
@@ -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 = url;
+        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>
index 451b9b4..343e7af 100644 (file)
@@ -1,3 +1,38 @@
+2018-03-12  Chris Dumez  <cdumez@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  <ajuma@chromium.org>
 
         http/tests/workers/service/service-worker-download.https.html times out with async policy delegates
index 1f45d7b..e42c907 100644 (file)
@@ -488,6 +488,11 @@ void NetworkResourceLoader::continueWillSendRequest(ResourceRequest&& newRequest
 
 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 @@ void NetworkResourceLoader::didRetrieveCacheEntry(std::unique_ptr<NetworkCache::
     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();
index 9db11b0..de3efc6 100644 (file)
@@ -126,6 +126,7 @@ private:
     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 @@ private:
     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
index 1b42615..ca46482 100644 (file)
@@ -114,7 +114,13 @@ void WebResourceLoader::didReceiveResponse(const ResourceResponse& response, boo
 
     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::didReceiveResponse(const ResourceResponse& response, boo
 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 @@ void WebResourceLoader::didFinishResourceLoad(const NetworkLoadMetrics& networkL
     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 @@ void WebResourceLoader::didFailResourceLoad(const ResourceError& error)
     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);
index 3198ac7..d2ae6a2 100644 (file)
@@ -89,6 +89,10 @@ private:
     RefPtr<WebCore::ResourceLoader> m_coreLoader;
     TrackingParameters m_trackingParameters;
     size_t m_numBytesReceived { 0 };
+
+#if !ASSERT_DISABLED
+    bool m_isProcessingNetworkResponse { false };
+#endif
 };
 
 } // namespace WebKit