Attachment 339321 Details for Bug 185209 – Patch

[patch] Patch

bug-185209-20180502113129.patch (text/plain), 6.75 KB, created by Daniel Bates on 2018-05-02 11:32:21 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Daniel Bates

Created: 2018-05-02 11:32:21 PDT

Size: 6.75 KB

patch

obsolete

>Subversion Revision: 231201
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index a17076d0c4fe154419e30b8e186e755e1ca6717e..6894d7a4cd368068b0af657261683505cf3c396f 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,26 @@
>+2018-05-02  Daniel Bates  <dabates@apple.com>
>+
>+        Cleanup NetworkResourceLoader::didReceiveResponse()
>+        https://bugs.webkit.org/show_bug.cgi?id=185209
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Use early returns to make the control flow easier to read and reason about. Disregarding a
>+        From-Origin violation, NetworkResourceLoader::didReceiveResponse() only returns NetworkLoadClient::ShouldContinueDidReceiveResponse::No
>+        when the load is for a main resource and hence it must wait for the embedding client to allow
>+        the load before continuing with it. With regards to a From-Origin violation, the network
>+        process schedules to fail the load in a subsequent turn of the event loop before returning
>+        NetworkLoadClient::ShouldContinueDidReceiveResponse::No. It return NetworkLoadClient::ShouldContinueDidReceiveResponse::No
>+        solely to tell the NetworkLoadClient to defer assuming the load is allowed (because we will
>+        fail it on the next turn of the event loop).
>+
>+        Additionally, keep a single log message and remove all others. The only meaningful message
>+        to log is when the network process is waiting on the embedding client (via the WebContent process)
>+        to allow the load before continuing.
>+
>+        * NetworkProcess/NetworkResourceLoader.cpp:
>+        (WebKit::NetworkResourceLoader::didReceiveResponse):
>+
> 2018-05-01  Daniel Bates  <dabates@apple.com>
> 
>         Substitute CrossOriginPreflightResultCache::clear() for CrossOriginPreflightResultCache::empty()
>diff --git a/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp b/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
>index 297055aaf3e447c3f772b1be3eea20b5cea5034c..adfa7516e96641873ecbcd13c215dd5a1a40e3cc 100644
>--- a/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
>@@ -413,43 +413,40 @@ auto NetworkResourceLoader::didReceiveResponse(ResourceResponse&& receivedRespon
>         } else
>             m_cacheEntryForValidation = nullptr;
>     }
>-    bool shouldSendDidReceiveResponse = !m_cacheEntryForValidation;
>-
>-    bool shouldWaitContinueDidReceiveResponse = isMainResource();
>-    if (shouldSendDidReceiveResponse) {
>-
>-        ResourceError error;
>-        if (m_parameters.shouldEnableFromOriginResponseHeader && shouldCancelCrossOriginLoad(m_response, m_parameters.frameAncestorOrigins))
>-            error = fromOriginResourceError(m_response.url());
>-
>-        if (error.isNull() && m_networkLoadChecker)
>-            error = m_networkLoadChecker->validateResponse(m_response);
>-
>-        if (!error.isNull()) {
>-            RunLoop::main().dispatch([protectedThis = makeRef(*this), error = WTFMove(error)] {
>-                if (protectedThis->m_networkLoad)
>-                    protectedThis->didFailLoading(error);
>-            });
>-            return ShouldContinueDidReceiveResponse::No;
>-        }
>+    if (m_cacheEntryForValidation)
>+        return ShouldContinueDidReceiveResponse::Yes;
> 
>-        auto response = sanitizeResponseIfPossible(ResourceResponse { m_response }, ResourceResponse::SanitizationType::CrossOriginSafe);
>-        if (isSynchronous())
>-            m_synchronousLoadData->response = WTFMove(response);
>-        else
>-            send(Messages::WebResourceLoader::DidReceiveResponse { response, shouldWaitContinueDidReceiveResponse });
>+    ResourceError error;
>+    if (m_parameters.shouldEnableFromOriginResponseHeader && shouldCancelCrossOriginLoad(m_response, m_parameters.frameAncestorOrigins))
>+        error = fromOriginResourceError(m_response.url());
>+    if (error.isNull() && m_networkLoadChecker)
>+        error = m_networkLoadChecker->validateResponse(m_response);
>+    if (!error.isNull()) {
>+        // FIXME: We need to make a main resource load look successful to prevent leaking its existence. See <https://bugs.webkit.org/show_bug.cgi?id=185120>.
>+        RunLoop::main().dispatch([protectedThis = makeRef(*this), error = WTFMove(error)] {
>+            if (protectedThis->m_networkLoad)
>+                protectedThis->didFailLoading(error);
>+        });
>+        // FIXME: We know that we are not going to continue this load. ShouldContinueDidReceiveResponse::No should only be returned when
>+        // the network process is waiting to receive message NetworkResourceLoader::ContinueDidReceiveResponse to continue a load.
>+        return ShouldContinueDidReceiveResponse::No;
>     }
> 
>-    // For main resources, the web process is responsible for sending back a NetworkResourceLoader::ContinueDidReceiveResponse message.
>-    bool shouldContinueDidReceiveResponse = !shouldWaitContinueDidReceiveResponse || m_cacheEntryForValidation;
>-
>-    if (shouldContinueDidReceiveResponse) {
>-        RELEASE_LOG_IF_ALLOWED("didReceiveResponse: Should not wait for message from WebContent process before continuing resource load (pageID = %" PRIu64 ", frameID = %" PRIu64 ", resourceID = %" PRIu64 ")", m_parameters.webPageID, m_parameters.webFrameID, m_parameters.identifier);
>+    auto response = sanitizeResponseIfPossible(ResourceResponse { m_response }, ResourceResponse::SanitizationType::CrossOriginSafe);
>+    if (isSynchronous()) {
>+        m_synchronousLoadData->response = WTFMove(response);
>         return ShouldContinueDidReceiveResponse::Yes;
>     }
> 
>-    RELEASE_LOG_IF_ALLOWED("didReceiveResponse: Should wait for message from WebContent process before continuing resource load (pageID = %" PRIu64 ", frameID = %" PRIu64 ", resourceID = %" PRIu64 ")", m_parameters.webPageID, m_parameters.webFrameID, m_parameters.identifier);
>-    return ShouldContinueDidReceiveResponse::No;
>+    // We wait to receive message NetworkResourceLoader::ContinueDidReceiveResponse before continuing a load for
>+    // a main resource because the embedding client must decide whether to allow the load.
>+    bool willWaitForContinueDidReceiveResponse = isMainResource();
>+    send(Messages::WebResourceLoader::DidReceiveResponse { response, willWaitForContinueDidReceiveResponse });
>+    if (willWaitForContinueDidReceiveResponse) {
>+        RELEASE_LOG_IF_ALLOWED("didReceiveResponse: will not continue load until receive NetworkResourceLoader::ContinueDidReceiveResponse message (pageID = %" PRIu64 ", frameID = %" PRIu64 ", resourceID = %" PRIu64 ")", m_parameters.webPageID, m_parameters.webFrameID, m_parameters.identifier);
>+        return ShouldContinueDidReceiveResponse::No;
>+    }
>+    return ShouldContinueDidReceiveResponse::Yes;
> }
> 
> void NetworkResourceLoader::didReceiveBuffer(Ref<SharedBuffer>&& buffer, int reportedEncodedDataLength)

Flags:

cdumez: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 185209: 339319 | 339321