Bug 185661 – NetworkLoadChecker should cancel its content extension retrieval task when being destroyed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[youenn fablet]

NetworkLoadChecker should cancel its content extension retrieval task when being destroyed

[youenn fablet]

<rdar://problem/39985509>

[youenn fablet]

Created attachment 340439 [details]
Patch

[youenn fablet]

Created attachment 340451 [details]
Patch

[EWS Watchlist]

Comment on attachment 340451 [details]
Patch

Attachment 340451 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7695861

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 340474 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[youenn fablet]

Created attachment 340492 [details]
Patch

[youenn fablet]

Comment on attachment 340492 [details]
Patch

>Subversion Revision: 231744
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 06d62a4046247736fe55e92cc5797c552fa67a39..1efb47a612948cf09db2514d9b09939cf5e54519 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,39 @@
>[phone]  Youenn Fablet  <[e-mail]>
>+
>+        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!).
>+
>+        When NetworkLoadChecker is freed, make its Content Extension retrieval callback
>+        if any, be called with a cancel error.
>+        Store the callbacks in a map keyed by the NetworkLoadChecker pointer instead of a vector for that purpose.
>+
>+        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/NetworkContentRuleListManager.cpp:
>+        (WebKit::NetworkContentRuleListManager::~NetworkContentRuleListManager):
>+        (WebKit::NetworkContentRuleListManager::contentExtensionsBackend):
>+        renamed to retrieveContentExtensionsBackend.
>+        (WebKit::NetworkContentRuleListManager::addContentRuleLists):
>+        (WebKit::NetworkContentRuleListManager::cancelContentExtensionsBackendRetrieval):
>+        * NetworkProcess/NetworkContentRuleListManager.h:
>+        * NetworkProcess/NetworkLoadChecker.cpp:
>+        (WebKit::NetworkLoadChecker::~NetworkLoadChecker):
>+        (WebKit::NetworkLoadChecker::checkRequest):
>+        (WebKit::NetworkLoadChecker::processContentExtensionRulesForLoad):
>+        * NetworkProcess/NetworkLoadChecker.h:
>+        * NetworkProcess/NetworkResourceLoader.cpp:
>+        (WebKit::NetworkResourceLoader::startNetworkLoad):
>+        (WebKit::NetworkResourceLoader::didReceiveResponse):
>+        * NetworkProcess/NetworkResourceLoader.h:
>+        * NetworkProcess/PingLoad.cpp:
>+        (WebKit::PingLoad::PingLoad):
>+        * NetworkProcess/PingLoad.h:
>+
> 2018-05-13  Dean Jackson  <[e-mail]>
> 
>         WebKit2_Sim-7606.1.17.4 introduced dep cycle
>diff --git a/Source/WebKit/NetworkProcess/NetworkContentRuleListManager.cpp b/Source/WebKit/NetworkProcess/NetworkContentRuleListManager.cpp
>index c56a5577d5df7efb74238bd24daf62a357dc4ad9..e9f353115684af5d12447eeb99875013d2af5588 100644
>--- a/Source/WebKit/NetworkProcess/NetworkContentRuleListManager.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkContentRuleListManager.cpp
>@@ -45,21 +45,22 @@ NetworkContentRuleListManager::~NetworkContentRuleListManager()
> 
>     WebCore::ContentExtensions::ContentExtensionsBackend backend;
>     for (auto& callbacks : pendingCallbacks.values()) {
>-        for (auto& callback : callbacks)
>-            callback(backend);
>+        for (auto& callback : callbacks.values())
>+            callback(makeUnexpected(ResourceError(ResourceError::Type::Cancellation)));
>     }
> }
> 
>-void NetworkContentRuleListManager::contentExtensionsBackend(UserContentControllerIdentifier identifier, BackendCallback&& callback)
>+void NetworkContentRuleListManager::retrieveContentExtensionsBackend(UserContentControllerIdentifier identifier, void* callbackOwner, BackendCallback&& callback)
> {
>     auto iterator = m_contentExtensionBackends.find(identifier);
>     if (iterator != m_contentExtensionBackends.end()) {
>-        callback(*iterator->value);
>+        callback(std::reference_wrapper<WebCore::ContentExtensions::ContentExtensionsBackend>(*iterator->value));
>         return;
>     }
>+    
>     m_pendingCallbacks.ensure(identifier, [] {
>-        return Vector<BackendCallback> { };
>-    }).iterator->value.append(WTFMove(callback));
>+        return HashMap<void*, BackendCallback> { };
>+    }).iterator->value.add(callbackOwner, WTFMove(callback));
>     NetworkProcess::singleton().parentProcessConnection()->send(Messages::NetworkProcessProxy::ContentExtensionRules { identifier }, 0);
> }
> 
>@@ -76,8 +77,8 @@ void NetworkContentRuleListManager::addContentRuleLists(UserContentControllerIde
>     }
> 
>     auto pendingCallbacks = m_pendingCallbacks.take(identifier);
>-    for (auto& callback : pendingCallbacks)
>-        callback(backend);
>+    for (auto& callback : pendingCallbacks.values())
>+        callback(std::reference_wrapper<WebCore::ContentExtensions::ContentExtensionsBackend>(backend));
> 
> }
> 
>@@ -104,6 +105,21 @@ void NetworkContentRuleListManager::remove(UserContentControllerIdentifier ident
>     m_contentExtensionBackends.remove(identifier);
> }
> 
>+void NetworkContentRuleListManager::cancelContentExtensionsBackendRetrieval(UserContentControllerIdentifier identifier, void* callbackOwner)
>+{
>+    ASSERT(m_pendingCallbacks.contains(identifier));
>+    auto iterator = m_pendingCallbacks.find(identifier);
>+    if (iterator == m_pendingCallbacks.end())
>+        return;
>+
>+    ASSERT(iterator->value.contains(callbackOwner));
>+    if (auto callback = iterator->value.take(callbackOwner)) {
>+        callback(makeUnexpected(ResourceError(ResourceError::Type::Cancellation)));
>+        if (iterator->value.isEmpty())
>+            m_pendingCallbacks.remove(iterator);
>+    }
>+}
>+
> } // namespace WebKit
> 
> #endif // ENABLE(CONTENT_EXTENSIONS)
>diff --git a/Source/WebKit/NetworkProcess/NetworkContentRuleListManager.h b/Source/WebKit/NetworkProcess/NetworkContentRuleListManager.h
>index 0f76ce259aa7364dc0e7612e0dd33969a66e0785..398262f5a9e4b540212c98f4f0a7f6aeff48afb4 100644
>--- a/Source/WebKit/NetworkProcess/NetworkContentRuleListManager.h
>+++ b/Source/WebKit/NetworkProcess/NetworkContentRuleListManager.h
>@@ -46,8 +46,10 @@ public:
> 
>     void didReceiveMessage(IPC::Connection&, IPC::Decoder&);
> 
>-    using BackendCallback = CompletionHandler<void(WebCore::ContentExtensions::ContentExtensionsBackend&)>;
>-    void contentExtensionsBackend(UserContentControllerIdentifier, BackendCallback&&);
>+    using BackendOrError = Expected<std::reference_wrapper<WebCore::ContentExtensions::ContentExtensionsBackend>, WebCore::ResourceError>;
>+    using BackendCallback = CompletionHandler<void(BackendOrError&&)>;
>+    void retrieveContentExtensionsBackend(UserContentControllerIdentifier, void* callbackOwner, BackendCallback&&);
>+    void cancelContentExtensionsBackendRetrieval(UserContentControllerIdentifier, void* callbackOwner);
> 
> private:
>     void addContentRuleLists(UserContentControllerIdentifier, const Vector<std::pair<String, WebCompiledContentRuleListData>>&);
>@@ -56,7 +58,7 @@ private:
>     void remove(UserContentControllerIdentifier);
> 
>     HashMap<UserContentControllerIdentifier, std::unique_ptr<WebCore::ContentExtensions::ContentExtensionsBackend>> m_contentExtensionBackends;
>-    HashMap<UserContentControllerIdentifier, Vector<BackendCallback>> m_pendingCallbacks;
>+    HashMap<UserContentControllerIdentifier, HashMap<void*, BackendCallback>> m_pendingCallbacks;
> };
> 
> } // namespace WebKit
>diff --git a/Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp b/Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp
>index 960731d13547ec48d8c296fb59dd791797f82e69..64a05cc8f4fd59dc4fafa3bec74354801537d2d6 100644
>--- a/Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp
>@@ -71,7 +71,13 @@ NetworkLoadChecker::NetworkLoadChecker(FetchOptions&& options, PAL::SessionID se
>     }
> }
> 
>-NetworkLoadChecker::~NetworkLoadChecker() = default;
>+NetworkLoadChecker::~NetworkLoadChecker()
>+{
>+#if ENABLE(CONTENT_EXTENSIONS)
>+    if (m_isRetrievingContentExtensions)
>+        NetworkProcess::singleton().networkContentRuleListManager().cancelContentExtensionsBackendRetrieval(*m_userContentControllerIdentifier, this);
>+#endif
>+}
> 
> void NetworkLoadChecker::check(ResourceRequest&& request, ValidationHandler&& handler)
> {
>@@ -161,12 +167,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 +333,27 @@ 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 {
>-        auto status = backend.processContentExtensionRulesForPingLoad(request.url(), m_mainDocumentURL);
>+
>+    m_isRetrievingContentExtensions = true;
>+    NetworkProcess::singleton().networkContentRuleListManager().retrieveContentExtensionsBackend(*m_userContentControllerIdentifier, this, [this, request = WTFMove(request), callback = WTFMove(callback)](auto result) mutable {
>+        if (!result.has_value()) {
>+            ASSERT(result.error().isCancellation());
>+            callback(makeUnexpected(WTFMove(result.error())));
>+            return;
>+        }
>+
>+        m_isRetrievingContentExtensions = false;
>+        auto status = result.value().get().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..9811876deadbf4009cf3107784d77db6f33b775e 100644
>--- a/Source/WebKit/NetworkProcess/NetworkLoadChecker.h
>+++ b/Source/WebKit/NetworkProcess/NetworkLoadChecker.h
>@@ -40,12 +40,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>;
>@@ -69,8 +66,6 @@ public:
>     WebCore::StoredCredentialsPolicy storedCredentialsPolicy() const { return m_storedCredentialsPolicy; }
> 
> 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; }
>     bool isRedirected() const { return m_redirectCount; }
>@@ -87,7 +82,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 +113,7 @@ private:
>     WebCore::PreflightPolicy m_preflightPolicy;
>     String m_dntHeaderValue;
>     String m_referrer;
>+    bool m_isRetrievingContentExtensions { false };
> };
> 
> }
>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 e351694c69521ea03ec435a73666bf5b3af06f84..b1ba2e8ba315546bbaf222421754ed830c799ff2 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,15 @@
>[phone]  Youenn Fablet  <[e-mail]>
>+
>+        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-14  Youenn Fablet  <[e-mail]>
> 
>         readableStreamDefaultControllerError should return early if stream is not readable
>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"
>+        }
>+    }
>+]

[Chris Dumez]

Comment on attachment 340492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=340492&action=review

> Source/WebKit/ChangeLog:13
> +        This allows stopping NetworkLoadChecker be ref counted.

Why cannot we used WeakPtr? Wouldn't it lead to simpler code? It looks a bit odd to have a HashMap of void*.

> Source/WebKit/NetworkProcess/NetworkContentRuleListManager.cpp:57
> +        callback(std::reference_wrapper<WebCore::ContentExtensions::ContentExtensionsBackend>(*iterator->value));

Why do we need this ?

> Source/WebKit/NetworkProcess/NetworkContentRuleListManager.cpp:81
> +        callback(std::reference_wrapper<WebCore::ContentExtensions::ContentExtensionsBackend>(backend));

Why do we need this? This looks really ugly :/

> Source/WebKit/NetworkProcess/NetworkContentRuleListManager.cpp:119
> +            m_pendingCallbacks.remove(iterator);

I always worry about relying on iterator still being valid after calling the callback. I think we should make this more robust before landing.

[youenn fablet]

Created attachment 340734 [details]
Patch

[youenn fablet]

Thanks for the feedback Chris, I updated the patch accordingly

[Chris Dumez]

Comment on attachment 340734 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 340734 [details]
Patch

Clearing flags on attachment: 340734

Committed r231988: <https://trac.webkit.org/changeset/231988>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.