Attachment 340734 Details for Bug 185661 – Patch

[patch] Patch

bug-185661-20180518135509.patch (text/plain), 14.66 KB, created by youenn fablet on 2018-05-18 13:55:11 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: youenn fablet

Created: 2018-05-18 13:55:11 PDT

Size: 14.66 KB

patch

obsolete

>Subversion Revision: 231917
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 46e9e6eeb67cd6e74df24c851cbdf71531c3f36b..cfda4e556e3cff26cfb3a61c5467b7057b348300 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,27 @@
>+2018-05-18  Youenn Fablet  <youenn@apple.com>
>+
>+        NetworkLoadChecker should cancel its content extension retrieval task when being destroyed
>+        https://bugs.webkit.org/show_bug.cgi?id=185661
>+        <rdar://problem/39985509>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Make sure that the Content Extension retrieval callback checks that NetworkLoadChecker is alive.
>+        This allows stopping NetworkLoadChecker be ref counted.
>+        This in turns allows NetworkResourceLoader to delete its NetworkLoadChecker when being deleted as well.
>+        By doing so, we simplify the memory management of NetworkResourceLoader and NetworkLoadChecker.
>+
>+        * NetworkProcess/NetworkLoadChecker.cpp:
>+        (WebKit::NetworkLoadChecker::checkRequest):
>+        (WebKit::NetworkLoadChecker::processContentExtensionRulesForLoad):
>+        * NetworkProcess/NetworkLoadChecker.h:
>+        (WebKit::NetworkLoadChecker::weakPtrFactory):
>+        * NetworkProcess/NetworkResourceLoader.cpp:
>+        * NetworkProcess/NetworkResourceLoader.h:
>+        * NetworkProcess/PingLoad.cpp:
>+        (WebKit::PingLoad::PingLoad):
>+        * NetworkProcess/PingLoad.h:
>+
> 2018-05-17  Youenn Fablet  <youenn@apple.com>
> 
>         -Wmemset-elt-size warning in LibWebRTCSocket constructor
>diff --git a/Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp b/Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp
>index 960731d13547ec48d8c296fb59dd791797f82e69..0a4d5c11308dfb2863f2ee56fb2bb8c491c7214a 100644
>--- a/Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp
>@@ -161,12 +161,17 @@ auto NetworkLoadChecker::accessControlErrorForValidationHandler(String&& message
> void NetworkLoadChecker::checkRequest(ResourceRequest&& request, ValidationHandler&& handler)
> {
> #if ENABLE(CONTENT_EXTENSIONS)
>-    processContentExtensionRulesForLoad(WTFMove(request), [this, handler = WTFMove(handler)](auto&& request, auto status) mutable {
>-        if (status.blockedLoad) {
>+    processContentExtensionRulesForLoad(WTFMove(request), [this, handler = WTFMove(handler)](auto result) mutable {
>+        if (!result.has_value()) {
>+            ASSERT(result.error().isCancellation());
>+            handler(makeUnexpected(WTFMove(result.error())));
>+            return;
>+        }
>+        if (result.value().status.blockedLoad) {
>             handler(this->accessControlErrorForValidationHandler(ASCIILiteral("Blocked by content extension")));
>             return;
>         }
>-        this->continueCheckingRequest(WTFMove(request), WTFMove(handler));
>+        this->continueCheckingRequest(WTFMove(result.value().request), WTFMove(handler));
>     });
> #else
>     continueCheckingRequest(WTFMove(request), WTFMove(handler));
>@@ -322,18 +327,24 @@ ContentSecurityPolicy* NetworkLoadChecker::contentSecurityPolicy()
> }
> 
> #if ENABLE(CONTENT_EXTENSIONS)
>-void NetworkLoadChecker::processContentExtensionRulesForLoad(ResourceRequest&& request, CompletionHandler<void(ResourceRequest&&, const ContentExtensions::BlockedStatus&)>&& callback)
>+void NetworkLoadChecker::processContentExtensionRulesForLoad(ResourceRequest&& request, ContentExtensionCallback&& callback)
> {
>     // FIXME: Enable content blockers for navigation loads.
>     if (!m_userContentControllerIdentifier || m_options.mode == FetchOptions::Mode::Navigate) {
>         ContentExtensions::BlockedStatus status;
>-        callback(WTFMove(request), status);
>+        callback(ContentExtensionResult { WTFMove(request), status });
>         return;
>     }
>-    NetworkProcess::singleton().networkContentRuleListManager().contentExtensionsBackend(*m_userContentControllerIdentifier, [protectedThis = makeRef(*this), this, request = WTFMove(request), callback = WTFMove(callback)](auto& backend) mutable {
>+
>+    NetworkProcess::singleton().networkContentRuleListManager().contentExtensionsBackend(*m_userContentControllerIdentifier, [this, weakThis = makeWeakPtr(this), request = WTFMove(request), callback = WTFMove(callback)](auto& backend) mutable {
>+        if (!weakThis) {
>+            callback(makeUnexpected(ResourceError { ResourceError::Type::Cancellation }));
>+            return;
>+        }
>+
>         auto status = backend.processContentExtensionRulesForPingLoad(request.url(), m_mainDocumentURL);
>         applyBlockedStatusToRequest(status, nullptr, request);
>-        callback(WTFMove(request), status);
>+        callback(ContentExtensionResult { WTFMove(request), status });
>     });
> }
> #endif // ENABLE(CONTENT_EXTENSIONS)
>diff --git a/Source/WebKit/NetworkProcess/NetworkLoadChecker.h b/Source/WebKit/NetworkProcess/NetworkLoadChecker.h
>index 9e4754a5d690fe9565f429dbc0092dc8e1d9f001..c6597a04d8d24697d8d7dab86e56c07995b774a5 100644
>--- a/Source/WebKit/NetworkProcess/NetworkLoadChecker.h
>+++ b/Source/WebKit/NetworkProcess/NetworkLoadChecker.h
>@@ -31,6 +31,7 @@
> #include <WebCore/ResourceResponse.h>
> #include <wtf/CompletionHandler.h>
> #include <wtf/Expected.h>
>+#include <wtf/WeakPtr.h>
> 
> namespace WebCore {
> class ContentSecurityPolicy;
>@@ -40,12 +41,9 @@ namespace WebKit {
> 
> class NetworkCORSPreflightChecker;
> 
>-class NetworkLoadChecker : public RefCounted<NetworkLoadChecker> {
>+class NetworkLoadChecker {
> public:
>-    static Ref<NetworkLoadChecker> create(WebCore::FetchOptions&& options, PAL::SessionID sessionID, WebCore::HTTPHeaderMap&& originalHeaders, WebCore::URL&& url, RefPtr<WebCore::SecurityOrigin>&& sourceOrigin, WebCore::PreflightPolicy preflightPolicy, String&& referrer)
>-    {
>-        return adoptRef(*new NetworkLoadChecker { WTFMove(options), sessionID, WTFMove(originalHeaders), WTFMove(url), WTFMove(sourceOrigin), preflightPolicy, WTFMove(referrer) });
>-    }
>+    NetworkLoadChecker(WebCore::FetchOptions&&, PAL::SessionID, WebCore::HTTPHeaderMap&&, WebCore::URL&&, RefPtr<WebCore::SecurityOrigin>&&, WebCore::PreflightPolicy, String&& referrer);
>     ~NetworkLoadChecker();
> 
>     using RequestOrError = Expected<WebCore::ResourceRequest, WebCore::ResourceError>;
>@@ -68,9 +66,9 @@ public:
>     const WebCore::URL& url() const { return m_url; }
>     WebCore::StoredCredentialsPolicy storedCredentialsPolicy() const { return m_storedCredentialsPolicy; }
> 
>-private:
>-    NetworkLoadChecker(WebCore::FetchOptions&&, PAL::SessionID, WebCore::HTTPHeaderMap&&, WebCore::URL&&, RefPtr<WebCore::SecurityOrigin>&&, WebCore::PreflightPolicy, String&& referrer);
>+    WeakPtrFactory<NetworkLoadChecker>& weakPtrFactory() { return m_weakFactory; }
> 
>+private:
>     WebCore::ContentSecurityPolicy* contentSecurityPolicy();
>     bool isChecking() const { return !!m_corsPreflightChecker; }
>     bool isRedirected() const { return m_redirectCount; }
>@@ -87,7 +85,13 @@ private:
>     RequestOrError accessControlErrorForValidationHandler(String&&);
> 
> #if ENABLE(CONTENT_EXTENSIONS)
>-    void processContentExtensionRulesForLoad(WebCore::ResourceRequest&&, CompletionHandler<void(WebCore::ResourceRequest&&, const WebCore::ContentExtensions::BlockedStatus&)>&&);
>+    struct ContentExtensionResult {
>+        WebCore::ResourceRequest request;
>+        const WebCore::ContentExtensions::BlockedStatus& status;
>+    };
>+    using ContentExtensionResultOrError = Expected<ContentExtensionResult, WebCore::ResourceError>;
>+    using ContentExtensionCallback = CompletionHandler<void(ContentExtensionResultOrError)>;
>+    void processContentExtensionRulesForLoad(WebCore::ResourceRequest&&, ContentExtensionCallback&&);
> #endif
> 
>     WebCore::FetchOptions m_options;
>@@ -112,6 +116,7 @@ private:
>     WebCore::PreflightPolicy m_preflightPolicy;
>     String m_dntHeaderValue;
>     String m_referrer;
>+    WeakPtrFactory<NetworkLoadChecker> m_weakFactory;
> };
> 
> }
>diff --git a/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp b/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
>index 8915fbb188ba0e0dc4d375a45eecdab73bade04f..f63eee95ac0ae080f57e97aee8538c15067acc23 100644
>--- a/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
>@@ -119,7 +119,7 @@ NetworkResourceLoader::NetworkResourceLoader(NetworkResourceLoadParameters&& par
>     }
> 
>     if (synchronousReply || parameters.shouldRestrictHTTPResponseAccess) {
>-        m_networkLoadChecker = NetworkLoadChecker::create(FetchOptions { m_parameters.options }, m_parameters.sessionID, HTTPHeaderMap { m_parameters.originalRequestHeaders }, URL { m_parameters.request.url() }, m_parameters.sourceOrigin.copyRef(), m_parameters.preflightPolicy, originalRequest().httpReferrer());
>+        m_networkLoadChecker = std::make_unique<NetworkLoadChecker>(FetchOptions { m_parameters.options }, m_parameters.sessionID, HTTPHeaderMap { m_parameters.originalRequestHeaders }, URL { m_parameters.request.url() }, m_parameters.sourceOrigin.copyRef(), m_parameters.preflightPolicy, originalRequest().httpReferrer());
>         if (m_parameters.cspResponseHeaders)
>             m_networkLoadChecker->setCSPResponseHeaders(ContentSecurityPolicyResponseHeaders { m_parameters.cspResponseHeaders.value() });
> #if ENABLE(CONTENT_EXTENSIONS)
>diff --git a/Source/WebKit/NetworkProcess/NetworkResourceLoader.h b/Source/WebKit/NetworkProcess/NetworkResourceLoader.h
>index b6a585eaf1d8a0c56af9fb802bc33bb2d66cb2f9..bdcf6da4d9972928076c2b27e4a61069dd6da6e3 100644
>--- a/Source/WebKit/NetworkProcess/NetworkResourceLoader.h
>+++ b/Source/WebKit/NetworkProcess/NetworkResourceLoader.h
>@@ -207,7 +207,7 @@ private:
>     std::unique_ptr<NetworkCache::Entry> m_cacheEntryForValidation;
>     bool m_isWaitingContinueWillSendRequestForCachedRedirect { false };
>     std::unique_ptr<NetworkCache::Entry> m_cacheEntryWaitingForContinueDidReceiveResponse;
>-    RefPtr<NetworkLoadChecker> m_networkLoadChecker;
>+    std::unique_ptr<NetworkLoadChecker> m_networkLoadChecker;
> 
>     std::optional<NetworkActivityTracker> m_networkActivityTracker;
> };
>diff --git a/Source/WebKit/NetworkProcess/PingLoad.cpp b/Source/WebKit/NetworkProcess/PingLoad.cpp
>index 35c7c0358e8caffc6e9b624ceb2fc9d43009c552..e3c8aa767ccfc21e1d6458d940d6b61c8f4c323a 100644
>--- a/Source/WebKit/NetworkProcess/PingLoad.cpp
>+++ b/Source/WebKit/NetworkProcess/PingLoad.cpp
>@@ -42,7 +42,7 @@ PingLoad::PingLoad(NetworkResourceLoadParameters&& parameters, WTF::CompletionHa
>     : m_parameters(WTFMove(parameters))
>     , m_completionHandler(WTFMove(completionHandler))
>     , m_timeoutTimer(*this, &PingLoad::timeoutTimerFired)
>-    , m_networkLoadChecker(NetworkLoadChecker::create(FetchOptions { m_parameters.options}, m_parameters.sessionID, WTFMove(m_parameters.originalRequestHeaders), URL { m_parameters.request.url() }, m_parameters.sourceOrigin.copyRef(), m_parameters.preflightPolicy, m_parameters.request.httpReferrer()))
>+    , m_networkLoadChecker(makeUniqueRef<NetworkLoadChecker>(FetchOptions { m_parameters.options}, m_parameters.sessionID, WTFMove(m_parameters.originalRequestHeaders), URL { m_parameters.request.url() }, m_parameters.sourceOrigin.copyRef(), m_parameters.preflightPolicy, m_parameters.request.httpReferrer()))
> {
> 
>     if (m_parameters.cspResponseHeaders)
>diff --git a/Source/WebKit/NetworkProcess/PingLoad.h b/Source/WebKit/NetworkProcess/PingLoad.h
>index d194592cea6ee24ec31699133dc6b56e670fa4a2..8b3c7d3c23770ea4a03393efc121b186ef25eda1 100644
>--- a/Source/WebKit/NetworkProcess/PingLoad.h
>+++ b/Source/WebKit/NetworkProcess/PingLoad.h
>@@ -70,7 +70,7 @@ private:
>     WTF::CompletionHandler<void(const WebCore::ResourceError&, const WebCore::ResourceResponse&)> m_completionHandler;
>     RefPtr<NetworkDataTask> m_task;
>     WebCore::Timer m_timeoutTimer;
>-    Ref<NetworkLoadChecker> m_networkLoadChecker;
>+    UniqueRef<NetworkLoadChecker> m_networkLoadChecker;
>     std::optional<WebCore::ResourceRequest> m_lastRedirectionRequest;
> };
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 376da3caf5c3e3c9297a1ecc612cafc968d012c0..9789efa0041bdca6286730b067cc10ce7443872b 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,15 @@
>+2018-05-18  Youenn Fablet  <youenn@apple.com>
>+
>+        NetworkLoadChecker should cancel its content extension retrieval task when being destroyed
>+        https://bugs.webkit.org/show_bug.cgi?id=185661
>+        <rdar://problem/39985509>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * http/tests/contentextensions/crash-xhr-expected.txt: Added.
>+        * http/tests/contentextensions/crash-xhr.html: Added.
>+        * http/tests/contentextensions/crash-xhr.html.json: Added.
>+
> 2018-05-17  Youenn Fablet  <youenn@apple.com>
> 
>         imported/w3c/web-platform-tests/fetch/nosniff/importscripts.html is crashing on debug builds
>diff --git a/LayoutTests/http/tests/contentextensions/crash-xhr-expected.txt b/LayoutTests/http/tests/contentextensions/crash-xhr-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..2064bd247727f5caeaf045eb3cd9637f4fbdfae5
>--- /dev/null
>+++ b/LayoutTests/http/tests/contentextensions/crash-xhr-expected.txt
>@@ -0,0 +1,3 @@
>+
>+PASS Test that aborting the XHR while gathering content blockers will not crash the Network Process 
>+
>diff --git a/LayoutTests/http/tests/contentextensions/crash-xhr.html b/LayoutTests/http/tests/contentextensions/crash-xhr.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..91237df09d14745790d3b6df490450328d8bf310
>--- /dev/null
>+++ b/LayoutTests/http/tests/contentextensions/crash-xhr.html
>@@ -0,0 +1,22 @@
>+<!DOCTYPE html>
>+<html>
>+<body>
>+    <script>
>+// We do the xhr load first so that this load will trigger the gathering by NetworkProcess of content blockers.
>+const promise = new Promise(resolve => {
>+var xhr = new XMLHttpRequest;
>+xhr.onloadend = resolve;
>+xhr.open("GET", ".", true);
>+xhr.send();
>+xhr.abort();
>+});
>+    </script>
>+    <script src="/resources/testharness.js"></script>
>+    <script src="/resources/testharnessreport.js"></script>
>+    <script>
>+promise_test((test) => {
>+    return promise;
>+}, "Test that aborting the XHR while gathering content blockers will not crash the Network Process");
>+    </script>
>+</body>
>+</html>
>diff --git a/LayoutTests/http/tests/contentextensions/crash-xhr.html.json b/LayoutTests/http/tests/contentextensions/crash-xhr.html.json
>new file mode 100644
>index 0000000000000000000000000000000000000000..cc4239b911179419599aec367604aec20de5d48d
>--- /dev/null
>+++ b/LayoutTests/http/tests/contentextensions/crash-xhr.html.json
>@@ -0,0 +1,18 @@
>+[
>+    {
>+        "action": {
>+            "type": "block"
>+        },
>+        "trigger": {
>+            "url-filter": ".*localhost"
>+        }
>+    },
>+    {
>+        "action": {
>+            "type": "ignore-previous-rules"
>+        },
>+        "trigger": {
>+            "url-filter": ".*whitelist"
>+        }
>+    }
>+]

Actions: View | Formatted Diff | Diff

Attachments on bug 185661: 340439 | 340451 | 340474 | 340492 | 340734