NetworkLoadChecker should cancel its content extension retrieval task when being destroyed
<rdar://problem/39985509>
Created attachment 340439 [details] Patch
Created attachment 340451 [details] Patch
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
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
Created attachment 340492 [details] Patch
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 @@ >+2018-05-15 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!). >+ >+ 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 <dino@apple.com> > > 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 @@ >+2018-05-15 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-14 Youenn Fablet <youenn@apple.com> > > 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" >+ } >+ } >+]
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.
Created attachment 340734 [details] Patch
Thanks for the feedback Chris, I updated the patch accordingly
Comment on attachment 340734 [details] Patch r=me
Comment on attachment 340734 [details] Patch Clearing flags on attachment: 340734 Committed r231988: <https://trac.webkit.org/changeset/231988>
All reviewed patches have been landed. Closing bug.