Refactoring related to Safe Browsing

https://bugs.webkit.org/show_bug.cgi?id=189631

Patch by Alex Christensen <achristensen@webkit.org> on 2018-09-14
Reviewed by Tim Horton.

Make SafeBrowsingResult RefCounted.
Move logic from an unnamed lambda to WebPageProxy::receivedNavigationPolicyDecision.

* UIProcess/Cocoa/WebPageProxyCocoa.mm:
(WebKit::WebPageProxy::beginSafeBrowsingCheck):
(WebKit::WebPageProxy::contentFilterDidBlockLoadForFrame): Deleted.
(WebKit::WebPageProxy::addPlatformLoadParameters): Deleted.
(WebKit::WebPageProxy::createSandboxExtensionsIfNeeded): Deleted.
(WebKit::WebPageProxy::startDrag): Deleted.
(WebKit::WebPageProxy::setPromisedDataForImage): Deleted.
(WebKit::WebPageProxy::setDragCaretRect): Deleted.
(WebKit::WebPageProxy::platformRegisterAttachment): Deleted.
(WebKit::WebPageProxy::platformCloneAttachment): Deleted.
* UIProcess/SafeBrowsingResult.h:
(WebKit::SafeBrowsingResult::create):
* UIProcess/WebFramePolicyListenerProxy.cpp:
(WebKit::WebFramePolicyListenerProxy::didReceiveSafeBrowsingResults):
* UIProcess/WebFramePolicyListenerProxy.h:
* UIProcess/WebFrameProxy.cpp:
(WebKit::WebFrameProxy::setUpPolicyListenerProxy):
* UIProcess/WebFrameProxy.h:
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::receivedNavigationPolicyDecision):
(WebKit::WebPageProxy::decidePolicyForNavigationAction):
(WebKit::WebPageProxy::decidePolicyForNewWindowAction):
(WebKit::WebPageProxy::decidePolicyForResponse):
* UIProcess/WebPageProxy.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@236020 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
index bae1975..fceb8d8 100644 (file)
--- a/Source/WebKit/ChangeLog
+++ b/Source/WebKit/ChangeLog
@@ -1,3 +1,38 @@
+2018-09-14  Alex Christensen  <achristensen@webkit.org>
+
+        Refactoring related to Safe Browsing
+        https://bugs.webkit.org/show_bug.cgi?id=189631
+
+        Reviewed by Tim Horton.
+
+        Make SafeBrowsingResult RefCounted.
+        Move logic from an unnamed lambda to WebPageProxy::receivedNavigationPolicyDecision.
+
+        * UIProcess/Cocoa/WebPageProxyCocoa.mm:
+        (WebKit::WebPageProxy::beginSafeBrowsingCheck):
+        (WebKit::WebPageProxy::contentFilterDidBlockLoadForFrame): Deleted.
+        (WebKit::WebPageProxy::addPlatformLoadParameters): Deleted.
+        (WebKit::WebPageProxy::createSandboxExtensionsIfNeeded): Deleted.
+        (WebKit::WebPageProxy::startDrag): Deleted.
+        (WebKit::WebPageProxy::setPromisedDataForImage): Deleted.
+        (WebKit::WebPageProxy::setDragCaretRect): Deleted.
+        (WebKit::WebPageProxy::platformRegisterAttachment): Deleted.
+        (WebKit::WebPageProxy::platformCloneAttachment): Deleted.
+        * UIProcess/SafeBrowsingResult.h:
+        (WebKit::SafeBrowsingResult::create):
+        * UIProcess/WebFramePolicyListenerProxy.cpp:
+        (WebKit::WebFramePolicyListenerProxy::didReceiveSafeBrowsingResults):
+        * UIProcess/WebFramePolicyListenerProxy.h:
+        * UIProcess/WebFrameProxy.cpp:
+        (WebKit::WebFrameProxy::setUpPolicyListenerProxy):
+        * UIProcess/WebFrameProxy.h:
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::receivedNavigationPolicyDecision):
+        (WebKit::WebPageProxy::decidePolicyForNavigationAction):
+        (WebKit::WebPageProxy::decidePolicyForNewWindowAction):
+        (WebKit::WebPageProxy::decidePolicyForResponse):
+        * UIProcess/WebPageProxy.h:
+
 2018-09-14  Geoffrey Garen  <ggaren@apple.com>
 
         Clarify the configuration used by WKUIDelegate's createWebViewWithConfiguration

diff --git a/Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm b/Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm
index d2a2c46..db24bbc 100644 (file)
--- a/Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm
+++ b/Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm
@@ -85,10 +85,10 @@ void WebPageProxy::beginSafeBrowsingCheck(const URL& url, WebFramePolicyListener
             }
 
             NSArray<SSBServiceLookupResult *> *results = [result serviceLookupResults];
-            Vector<SafeBrowsingResult> resultsVector;
+            Vector<Ref<SafeBrowsingResult>> resultsVector;
             resultsVector.reserveInitialCapacity([results count]);
             for (SSBServiceLookupResult *result in results)
-                resultsVector.uncheckedAppend({ URL(url), result });
+                resultsVector.uncheckedAppend(SafeBrowsingResult::create(URL(url), result));
             listener->didReceiveSafeBrowsingResults(WTFMove(resultsVector));
         });
     }).get()];

diff --git a/Source/WebKit/UIProcess/SafeBrowsingResult.h b/Source/WebKit/UIProcess/SafeBrowsingResult.h
index 84f4938..793bf4b 100644 (file)
--- a/Source/WebKit/UIProcess/SafeBrowsingResult.h
+++ b/Source/WebKit/UIProcess/SafeBrowsingResult.h
@@ -26,19 +26,21 @@
 #pragma once
 
 #include <WebCore/URL.h>
+#include <wtf/RefCounted.h>
 #include <wtf/text/WTFString.h>
 
 OBJC_CLASS SSBServiceLookupResult;
 
 namespace WebKit {
 
-class SafeBrowsingResult {
+class SafeBrowsingResult : public RefCounted<SafeBrowsingResult> {
 public:
 #if HAVE(SAFE_BROWSING)
-    SafeBrowsingResult(WebCore::URL&&, SSBServiceLookupResult *);
+    static Ref<SafeBrowsingResult> create(WebCore::URL&& url, SSBServiceLookupResult *result)
+    {
+        return adoptRef(*new SafeBrowsingResult(WTFMove(url), result));
+    }
 #endif
-    SafeBrowsingResult() = default;
-
     const WebCore::URL& url() const { return m_url; }
     const String& provider() const { return m_provider; }
     bool isPhishing() const { return m_isPhishing; }
@@ -47,6 +49,9 @@ public:
     bool isKnownToBeUnsafe() const { return m_isKnownToBeUnsafe; }
 
 private:
+#if HAVE(SAFE_BROWSING)
+    SafeBrowsingResult(WebCore::URL&&, SSBServiceLookupResult *);
+#endif
     WebCore::URL m_url;
     String m_provider;
     bool m_isPhishing { false };

diff --git a/Source/WebKit/UIProcess/WebFramePolicyListenerProxy.cpp b/Source/WebKit/UIProcess/WebFramePolicyListenerProxy.cpp
index 40aa184..568e745 100644 (file)
--- a/Source/WebKit/UIProcess/WebFramePolicyListenerProxy.cpp
+++ b/Source/WebKit/UIProcess/WebFramePolicyListenerProxy.cpp
@@ -45,7 +45,7 @@ WebFramePolicyListenerProxy::WebFramePolicyListenerProxy(Reply&& reply, ShouldEx
 
 WebFramePolicyListenerProxy::~WebFramePolicyListenerProxy() = default;
 
-void WebFramePolicyListenerProxy::didReceiveSafeBrowsingResults(Vector<SafeBrowsingResult>&& safeBrowsingResults)
+void WebFramePolicyListenerProxy::didReceiveSafeBrowsingResults(Vector<Ref<SafeBrowsingResult>>&& safeBrowsingResults)
 {
     ASSERT(!m_safeBrowsingResults);
     if (m_policyResult) {

diff --git a/Source/WebKit/UIProcess/WebFramePolicyListenerProxy.h b/Source/WebKit/UIProcess/WebFramePolicyListenerProxy.h
index f726c67..53075d0 100644 (file)
--- a/Source/WebKit/UIProcess/WebFramePolicyListenerProxy.h
+++ b/Source/WebKit/UIProcess/WebFramePolicyListenerProxy.h
@@ -47,7 +47,7 @@ enum class ShouldExpectSafeBrowsingResult { No, Yes };
 class WebFramePolicyListenerProxy : public API::ObjectImpl<API::Object::Type::FramePolicyListener> {
 public:
 
-    using Reply = CompletionHandler<void(WebCore::PolicyAction, API::WebsitePolicies*, ProcessSwapRequestedByClient, Vector<SafeBrowsingResult>&&)>;
+    using Reply = CompletionHandler<void(WebCore::PolicyAction, API::WebsitePolicies*, ProcessSwapRequestedByClient, Vector<Ref<SafeBrowsingResult>>&&)>;
     static Ref<WebFramePolicyListenerProxy> create(Reply&& reply, ShouldExpectSafeBrowsingResult expect)
     {
         return adoptRef(*new WebFramePolicyListenerProxy(WTFMove(reply), expect));
@@ -58,13 +58,13 @@ public:
     void download();
     void ignore();
     
-    void didReceiveSafeBrowsingResults(Vector<SafeBrowsingResult>&&);
+    void didReceiveSafeBrowsingResults(Vector<Ref<SafeBrowsingResult>>&&);
 
 private:
     WebFramePolicyListenerProxy(Reply&&, ShouldExpectSafeBrowsingResult);
 
     std::optional<std::pair<RefPtr<API::WebsitePolicies>, ProcessSwapRequestedByClient>> m_policyResult;
-    std::optional<Vector<SafeBrowsingResult>> m_safeBrowsingResults;
+    std::optional<Vector<Ref<SafeBrowsingResult>>> m_safeBrowsingResults;
     Reply m_reply;
 };
 

diff --git a/Source/WebKit/UIProcess/WebFrameProxy.cpp b/Source/WebKit/UIProcess/WebFrameProxy.cpp
index 026fc7b..b160ce4 100644 (file)
--- a/Source/WebKit/UIProcess/WebFrameProxy.cpp
+++ b/Source/WebKit/UIProcess/WebFrameProxy.cpp
@@ -177,11 +177,11 @@ void WebFrameProxy::didChangeTitle(const String& title)
     m_title = title;
 }
 
-WebFramePolicyListenerProxy& WebFrameProxy::setUpPolicyListenerProxy(CompletionHandler<void(WebCore::PolicyAction, API::WebsitePolicies*, ProcessSwapRequestedByClient, Vector<SafeBrowsingResult>&&)>&& completionHandler, ShouldExpectSafeBrowsingResult expect)
+WebFramePolicyListenerProxy& WebFrameProxy::setUpPolicyListenerProxy(CompletionHandler<void(WebCore::PolicyAction, API::WebsitePolicies*, ProcessSwapRequestedByClient, Vector<Ref<SafeBrowsingResult>>&&)>&& completionHandler, ShouldExpectSafeBrowsingResult expect)
 {
     if (m_activeListener)
         m_activeListener->ignore();
-    m_activeListener = WebFramePolicyListenerProxy::create([this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)] (WebCore::PolicyAction action, API::WebsitePolicies* policies, ProcessSwapRequestedByClient processSwapRequestedByClient, Vector<SafeBrowsingResult>&& safeBrowsingResults) mutable {
+    m_activeListener = WebFramePolicyListenerProxy::create([this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)] (WebCore::PolicyAction action, API::WebsitePolicies* policies, ProcessSwapRequestedByClient processSwapRequestedByClient, Vector<Ref<SafeBrowsingResult>>&& safeBrowsingResults) mutable {
         completionHandler(action, policies, processSwapRequestedByClient, WTFMove(safeBrowsingResults));
         m_activeListener = nullptr;
     }, expect);

diff --git a/Source/WebKit/UIProcess/WebFrameProxy.h b/Source/WebKit/UIProcess/WebFrameProxy.h
index 476a5a4..297aa8a 100644 (file)
--- a/Source/WebKit/UIProcess/WebFrameProxy.h
+++ b/Source/WebKit/UIProcess/WebFrameProxy.h
@@ -117,7 +117,7 @@ public:
     void didSameDocumentNavigation(const WebCore::URL&); // eg. anchor navigation, session state change.
     void didChangeTitle(const String&);
 
-    WebFramePolicyListenerProxy& setUpPolicyListenerProxy(CompletionHandler<void(WebCore::PolicyAction, API::WebsitePolicies*, ProcessSwapRequestedByClient, Vector<SafeBrowsingResult>&&)>&&, ShouldExpectSafeBrowsingResult);
+    WebFramePolicyListenerProxy& setUpPolicyListenerProxy(CompletionHandler<void(WebCore::PolicyAction, API::WebsitePolicies*, ProcessSwapRequestedByClient, Vector<Ref<SafeBrowsingResult>>&&)>&&, ShouldExpectSafeBrowsingResult);
 
 #if ENABLE(CONTENT_FILTERING)
     void contentFilterDidBlockLoad(WebCore::ContentFilterUnblockHandler contentFilterUnblockHandler) { m_contentFilterUnblockHandler = WTFMove(contentFilterUnblockHandler); }

diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
index 7ab5591..3883751 100644 (file)
--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
@@ -2431,7 +2431,36 @@ private:
 
     SendFunction m_sendFunction;
 };
+
+void WebPageProxy::receivedNavigationPolicyDecision(PolicyAction policyAction, API::Navigation& navigation, ProcessSwapRequestedByClient processSwapRequestedByClient, WebFrameProxy& frame, API::WebsitePolicies* policies, Ref<PolicyDecisionSender>&& sender)
+{
+    std::optional<WebsitePoliciesData> data;
+    if (policies) {
+        data = policies->data();
+        if (policies->websiteDataStore())
+            changeWebsiteDataStore(policies->websiteDataStore()->websiteDataStore());
+    }
+    
+    if (policyAction == PolicyAction::Use && frame.isMainFrame()) {
+        String reason;
+        auto proposedProcess = process().processPool().processForNavigation(*this, navigation, processSwapRequestedByClient, policyAction, reason);
+        ASSERT(!reason.isNull());
+        
+        if (proposedProcess.ptr() != &process()) {
+            RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "%p - WebPageProxy::decidePolicyForNavigationAction, swapping process %i with process %i for navigation, reason: %{public}s", this, processIdentifier(), proposedProcess->processIdentifier(), reason.utf8().data());
+            LOG(ProcessSwapping, "(ProcessSwapping) Switching from process %i to new process (%i) for navigation %" PRIu64 " '%s'", processIdentifier(), proposedProcess->processIdentifier(), navigation.navigationID(), navigation.loggingString());
+            
+            RunLoop::main().dispatch([this, protectedThis = makeRef(*this), navigation = makeRef(navigation), proposedProcess = WTFMove(proposedProcess)]() mutable {
+                continueNavigationInNewProcess(navigation, WTFMove(proposedProcess));
+            });
+        } else
+            RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "%p - WebPageProxy::decidePolicyForNavigationAction, keep using process %i for navigation, reason: %{public}s", this, processIdentifier(), reason.utf8().data());
+    }
     
+    receivedPolicyDecision(policyAction, &navigation, WTFMove(data), WTFMove(sender));
+
+}
+
 void WebPageProxy::receivedPolicyDecision(PolicyAction action, API::Navigation* navigation, std::optional<WebsitePoliciesData>&& websitePolicies, Ref<PolicyDecisionSender>&& sender)
 {
     if (!isValid()) {
@@ -4033,33 +4062,9 @@ void WebPageProxy::decidePolicyForNavigationAction(uint64_t frameID, const WebCo
     UNUSED_PARAM(newNavigationID);
 #endif
 
-    auto listener = makeRef(frame->setUpPolicyListenerProxy([this, protectedThis = makeRef(*this), frame = makeRef(*frame), sender = sender.copyRef(), navigation] (WebCore::PolicyAction policyAction, API::WebsitePolicies* policies, ProcessSwapRequestedByClient processSwapRequestedByClient, Vector<SafeBrowsingResult>&&) mutable {
+    auto listener = makeRef(frame->setUpPolicyListenerProxy([this, protectedThis = makeRef(*this), frame = makeRef(*frame), sender = WTFMove(sender), navigation = navigation.releaseNonNull()] (WebCore::PolicyAction policyAction, API::WebsitePolicies* policies, ProcessSwapRequestedByClient processSwapRequestedByClient, Vector<Ref<SafeBrowsingResult>>&&) mutable {
         // FIXME: do something with the SafeBrowsingResults.
-
-        std::optional<WebsitePoliciesData> data;
-        if (policies) {
-            data = policies->data();
-            if (policies->websiteDataStore())
-                changeWebsiteDataStore(policies->websiteDataStore()->websiteDataStore());
-        }
-
-        if (policyAction == PolicyAction::Use && frame->isMainFrame()) {
-            String reason;
-            auto proposedProcess = process().processPool().processForNavigation(*this, *navigation, processSwapRequestedByClient, policyAction, reason);
-            ASSERT(!reason.isNull());
-            
-            if (proposedProcess.ptr() != &process()) {
-                RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "%p - WebPageProxy::decidePolicyForNavigationAction, swapping process %i with process %i for navigation, reason: %{public}s", this, processIdentifier(), proposedProcess->processIdentifier(), reason.utf8().data());
-                LOG(ProcessSwapping, "(ProcessSwapping) Switching from process %i to new process (%i) for navigation %" PRIu64 " '%s'", processIdentifier(), proposedProcess->processIdentifier(), navigation->navigationID(), navigation->loggingString());
-                
-                RunLoop::main().dispatch([this, protectedThis = WTFMove(protectedThis), navigation = makeRef(*navigation), proposedProcess = WTFMove(proposedProcess)]() mutable {
-                    continueNavigationInNewProcess(navigation.get(), WTFMove(proposedProcess));
-                });
-            } else
-                RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "%p - WebPageProxy::decidePolicyForNavigationAction, keep using process %i for navigation, reason: %{public}s", this, processIdentifier(), reason.utf8().data());
-        }
-
-        receivedPolicyDecision(policyAction, navigation.get(), WTFMove(data), WTFMove(sender));
+        receivedNavigationPolicyDecision(policyAction, navigation, processSwapRequestedByClient, frame, policies, WTFMove(sender));
     }, shouldSkipSafeBrowsingCheck == ShouldSkipSafeBrowsingCheck::Yes ? ShouldExpectSafeBrowsingResult::No : ShouldExpectSafeBrowsingResult::Yes));
     if (shouldSkipSafeBrowsingCheck == ShouldSkipSafeBrowsingCheck::No)
         beginSafeBrowsingCheck(request.url(), listener);
@@ -4108,7 +4113,7 @@ void WebPageProxy::decidePolicyForNewWindowAction(uint64_t frameID, const Securi
     MESSAGE_CHECK(frame);
     MESSAGE_CHECK_URL(request.url());
 
-    auto listener = makeRef(frame->setUpPolicyListenerProxy([this, protectedThis = makeRef(*this), listenerID, frameID] (WebCore::PolicyAction policyAction, API::WebsitePolicies*, ProcessSwapRequestedByClient processSwapRequestedByClient, Vector<SafeBrowsingResult>&& safeBrowsingResults) mutable {
+    auto listener = makeRef(frame->setUpPolicyListenerProxy([this, protectedThis = makeRef(*this), listenerID, frameID] (WebCore::PolicyAction policyAction, API::WebsitePolicies*, ProcessSwapRequestedByClient processSwapRequestedByClient, Vector<Ref<SafeBrowsingResult>>&& safeBrowsingResults) mutable {
         // FIXME: Assert the API::WebsitePolicies* is nullptr here once clients of WKFramePolicyListenerUseWithPolicies go away.
         RELEASE_ASSERT(processSwapRequestedByClient == ProcessSwapRequestedByClient::No);
         ASSERT_UNUSED(safeBrowsingResults, safeBrowsingResults.isEmpty());
@@ -4144,7 +4149,7 @@ void WebPageProxy::decidePolicyForResponse(uint64_t frameID, const SecurityOrigi
     MESSAGE_CHECK_URL(response.url());
 
     RefPtr<API::Navigation> navigation = navigationID ? &m_navigationState->navigation(navigationID) : nullptr;
-    auto listener = makeRef(frame->setUpPolicyListenerProxy([this, protectedThis = makeRef(*this), frameID, listenerID, navigation = WTFMove(navigation)] (WebCore::PolicyAction policyAction, API::WebsitePolicies*, ProcessSwapRequestedByClient processSwapRequestedByClient, Vector<SafeBrowsingResult>&& safeBrowsingResults) mutable {
+    auto listener = makeRef(frame->setUpPolicyListenerProxy([this, protectedThis = makeRef(*this), frameID, listenerID, navigation = WTFMove(navigation)] (WebCore::PolicyAction policyAction, API::WebsitePolicies*, ProcessSwapRequestedByClient processSwapRequestedByClient, Vector<Ref<SafeBrowsingResult>>&& safeBrowsingResults) mutable {
         // FIXME: Assert the API::WebsitePolicies* is nullptr here once clients of WKFramePolicyListenerUseWithPolicies go away.
         RELEASE_ASSERT(processSwapRequestedByClient == ProcessSwapRequestedByClient::No);
         ASSERT_UNUSED(safeBrowsingResults, safeBrowsingResults.isEmpty());

diff --git a/Source/WebKit/UIProcess/WebPageProxy.h b/Source/WebKit/UIProcess/WebPageProxy.h
index a9d3072..898da18 100644 (file)
--- a/Source/WebKit/UIProcess/WebPageProxy.h
+++ b/Source/WebKit/UIProcess/WebPageProxy.h
@@ -913,6 +913,7 @@ public:
 
     class PolicyDecisionSender;
     void receivedPolicyDecision(WebCore::PolicyAction, API::Navigation*, std::optional<WebsitePoliciesData>&&, Ref<PolicyDecisionSender>&&);
+    void receivedNavigationPolicyDecision(WebCore::PolicyAction, API::Navigation&, ProcessSwapRequestedByClient, WebFrameProxy&, API::WebsitePolicies*, Ref<PolicyDecisionSender>&&);
 
     void backForwardRemovedItem(const WebCore::BackForwardItemIdentifier&);