Changeset 229560 in webkit

Timestamp:

Mar 12, 2018 4:06:51 PM (6 years ago)

Author:

Chris Dumez

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/http/tests/cache/cachedEntry-waits-for-response-policy-expected.txt (added)
• LayoutTests/http/tests/cache/cachedEntry-waits-for-response-policy.html (added)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp (modified) (2 diffs)
• Source/WebKit/NetworkProcess/NetworkResourceLoader.h (modified) (2 diffs)
• Source/WebKit/WebProcess/Network/WebResourceLoader.cpp (modified) (4 diffs)
• Source/WebKit/WebProcess/Network/WebResourceLoader.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp

@@ -489 +489 @@
 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.
@@ -564 +569 @@
     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;

trunk/Source/WebKit/NetworkProcess/NetworkResourceLoader.h

@@ -127 +127 @@
     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&);
@@ -175 +176 @@
     std::unique_ptr<NetworkCache::Entry> m_cacheEntryForValidation;
     bool m_isWaitingContinueWillSendRequestForCachedRedirect { false };
+    std::unique_ptr<NetworkCache::Entry> m_cacheEntryWaitingForContinueDidReceiveResponse;
 };
 

trunk/Source/WebKit/WebProcess/Network/WebResourceLoader.cpp

@@ -115 +115 @@
     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)
@@ -128 +134 @@
 {
     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) {
@@ -150 +157 @@
     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 +165 @@
     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))

trunk/Source/WebKit/WebProcess/Network/WebResourceLoader.h

@@ -90 +90 @@
     TrackingParameters m_trackingParameters;
     size_t m_numBytesReceived { 0 };
+
+#if !ASSERT_DISABLED
+    bool m_isProcessingNetworkResponse { false };
+#endif
 };