Changeset 231988 in webkit

Timestamp:

May 18, 2018 4:02:38 PM (6 years ago)

Author:

youenn@apple.com

Message:

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/http/tests/contentextensions/crash-xhr-expected.txt (added)
• LayoutTests/http/tests/contentextensions/crash-xhr.html (added)
• LayoutTests/http/tests/contentextensions/crash-xhr.html.json (added)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp (modified) (2 diffs)
• Source/WebKit/NetworkProcess/NetworkLoadChecker.h (modified) (5 diffs)
• Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp (modified) (1 diff)
• Source/WebKit/NetworkProcess/NetworkResourceLoader.h (modified) (1 diff)
• Source/WebKit/NetworkProcess/PingLoad.cpp (modified) (1 diff)
• Source/WebKit/NetworkProcess/PingLoad.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

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

trunk/Source/WebKit/ChangeLog

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

trunk/Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp

@@ -162 +162 @@
 {
 #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
@@ -323 +328 @@
 
 #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);
-        return;
-    }
-    NetworkProcess::singleton().networkContentRuleListManager().contentExtensionsBackend(*m_userContentControllerIdentifier, [protectedThis = makeRef(*this), this, request = WTFMove(request), callback = WTFMove(callback)](auto& backend) mutable {
+        callback(ContentExtensionResult { WTFMove(request), status });
+        return;
+    }
+
+    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 });
     });
 }

trunk/Source/WebKit/NetworkProcess/NetworkLoadChecker.h

@@ -32 +32 @@
 #include <wtf/CompletionHandler.h>
 #include <wtf/Expected.h>
+#include <wtf/WeakPtr.h>
 
 namespace WebCore {
@@ -41 +42 @@
 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();
 
@@ -69 +67 @@
     WebCore::StoredCredentialsPolicy storedCredentialsPolicy() const { return m_storedCredentialsPolicy; }
 
+    WeakPtrFactory<NetworkLoadChecker>& weakPtrFactory() { return m_weakFactory; }
+
 private:
-    NetworkLoadChecker(WebCore::FetchOptions&&, PAL::SessionID, WebCore::HTTPHeaderMap&&, WebCore::URL&&, RefPtr<WebCore::SecurityOrigin>&&, WebCore::PreflightPolicy, String&& referrer);
-
     WebCore::ContentSecurityPolicy* contentSecurityPolicy();
     bool isChecking() const { return !!m_corsPreflightChecker; }
@@ -88 +86 @@
 
 #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
 
@@ -113 +117 @@
     String m_dntHeaderValue;
     String m_referrer;
+    WeakPtrFactory<NetworkLoadChecker> m_weakFactory;
 };
 

trunk/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp

@@ -120 +120 @@
 
     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() });

trunk/Source/WebKit/NetworkProcess/NetworkResourceLoader.h

@@ -208 +208 @@
     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;

trunk/Source/WebKit/NetworkProcess/PingLoad.cpp

@@ -43 +43 @@
     , 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()))
 {
 

trunk/Source/WebKit/NetworkProcess/PingLoad.h

@@ -71 +71 @@
     RefPtr<NetworkDataTask> m_task;
     WebCore::Timer m_timeoutTimer;
-    Ref<NetworkLoadChecker> m_networkLoadChecker;
+    UniqueRef<NetworkLoadChecker> m_networkLoadChecker;
     std::optional<WebCore::ResourceRequest> m_lastRedirectionRequest;
 };