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 Chris Dumez. Source/WebKit: 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: LayoutTests: * http/tests/contentextensions/crash-xhr-expected.txt: Added. * http/tests/contentextensions/crash-xhr.html: Added. * http/tests/contentextensions/crash-xhr.html.json: Added. Canonical link: https://commits.webkit.org/201253@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@231988 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · May 18, 2018 · ce9eb0c5f2015290efddf9a9ed11e1aff781a0b9 · ce9eb0c
1 parent e11c6c5
commit ce9eb0c5f2015290efddf9a9ed11e1aff781a0b9
Showing 11 changed files with 114 additions and 19 deletions.
diff --git 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 Chris Dumez.
+
+        * 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-18  Jer Noble  <jer.noble@apple.com>
 
         Complete fix for enabling modern EME by default

diff --git a/LayoutTests/http/tests/contentextensions/crash-xhr-expected.txt 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
@@ -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
@@ -0,0 +1,18 @@
+[
+    {
+        "action": {
+            "type": "block"
+        },
+        "trigger": {
+            "url-filter": ".*localhost"
+        }
+    },
+    {
+        "action": {
+            "type": "ignore-previous-rules"
+        },
+        "trigger": {
+            "url-filter": ".*whitelist"
+        }
+    }
+]
diff --git 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 Chris Dumez.
+
+        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-18  Per Arne Vollan  <pvollan@apple.com>
 
         WebProcess fails to launch

diff --git 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
@@ -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 @@ class NetworkLoadChecker : public RefCounted<NetworkLoadChecker> {
     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 @@ class NetworkLoadChecker : public RefCounted<NetworkLoadChecker> {
     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 @@ class NetworkLoadChecker : public RefCounted<NetworkLoadChecker> {
     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
@@ -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
@@ -207,7 +207,7 @@ class NetworkResourceLoader final
     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
@@ -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
@@ -70,7 +70,7 @@ class PingLoad final : private NetworkDataTaskClient {
     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;
 };