[PSON] WebPageProxy::receivedNavigationPolicyDecision() should not schedule the new...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 1 Nov 2018 20:53:50 +0000 (20:53 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 1 Nov 2018 20:53:50 +0000 (20:53 +0000)
https://bugs.webkit.org/show_bug.cgi?id=191076

Reviewed by Geoffrey Garen.

Source/WebKit:

WebPageProxy::receivedNavigationPolicyDecision() should not schedule the new load asynchronously when process-swapping.
The client can request a new load synchronously after answering the policy decision, in which case we'd end up loading
the wrong URL.

* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::receivedNavigationPolicyDecision):
* UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::processForNavigation):
(WebKit::WebProcessPool::processForNavigationInternal):
* UIProcess/WebProcessPool.h:

Tools:

Add API test coverage.

* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
(-[PSONNavigationDelegate init]):
(-[PSONNavigationDelegate webView:decidePolicyForNavigationAction:decisionHandler:]):

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

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/WebPageProxy.cpp
Source/WebKit/UIProcess/WebProcessPool.cpp
Source/WebKit/UIProcess/WebProcessPool.h
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

index d702b04..4eb77c5 100644 (file)
@@ -1,5 +1,23 @@
 2018-11-01  Chris Dumez  <cdumez@apple.com>
 
+        [PSON] WebPageProxy::receivedNavigationPolicyDecision() should not schedule the new load asynchronously when process-swapping
+        https://bugs.webkit.org/show_bug.cgi?id=191076
+
+        Reviewed by Geoffrey Garen.
+
+        WebPageProxy::receivedNavigationPolicyDecision() should not schedule the new load asynchronously when process-swapping.
+        The client can request a new load synchronously after answering the policy decision, in which case we'd end up loading
+        the wrong URL.
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::receivedNavigationPolicyDecision):
+        * UIProcess/WebProcessPool.cpp:
+        (WebKit::WebProcessPool::processForNavigation):
+        (WebKit::WebProcessPool::processForNavigationInternal):
+        * UIProcess/WebProcessPool.h:
+
+2018-11-01  Chris Dumez  <cdumez@apple.com>
+
         [PSON] Unable to submit a file in FormData cross-site
         https://bugs.webkit.org/show_bug.cgi?id=191138
 
index df41cfd..571dd75 100644 (file)
@@ -2496,25 +2496,24 @@ void WebPageProxy::receivedNavigationPolicyDecision(PolicyAction policyAction, A
         if (policies->websiteDataStore())
             changeWebsiteDataStore(policies->websiteDataStore()->websiteDataStore());
     }
-    
+
+    Ref<WebProcessProxy> processForNavigation = process();
     if (policyAction == PolicyAction::Use && frame.isMainFrame()) {
         String reason;
-        auto proposedProcess = process().processPool().processForNavigation(*this, *navigation, processSwapRequestedByClient, policyAction, reason);
+        processForNavigation = process().processPool().processForNavigation(*this, *navigation, processSwapRequestedByClient, 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));
-            });
+        if (processForNavigation.ptr() != &process()) {
+            policyAction = PolicyAction::Suspend;
+            RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "%p - WebPageProxy::decidePolicyForNavigationAction, swapping process %i with process %i for navigation, reason: %{public}s", this, processIdentifier(), processForNavigation->processIdentifier(), reason.utf8().data());
+            LOG(ProcessSwapping, "(ProcessSwapping) Switching from process %i to new process (%i) for navigation %" PRIu64 " '%s'", processIdentifier(), processForNavigation->processIdentifier(), navigation->navigationID(), navigation->loggingString());
         } 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));
 
+    if (processForNavigation.ptr() != &process())
+        continueNavigationInNewProcess(*navigation, WTFMove(processForNavigation));
 }
 
 void WebPageProxy::receivedPolicyDecision(PolicyAction action, API::Navigation* navigation, std::optional<WebsitePoliciesData>&& websitePolicies, Ref<PolicyDecisionSender>&& sender)
index 04f1ead..86335a2 100644 (file)
@@ -2071,9 +2071,9 @@ void WebProcessPool::removeProcessFromOriginCacheSet(WebProcessProxy& process)
         m_swappedProcessesPerRegistrableDomain.remove(registrableDomain);
 }
 
-Ref<WebProcessProxy> WebProcessPool::processForNavigation(WebPageProxy& page, const API::Navigation& navigation, ProcessSwapRequestedByClient processSwapRequestedByClient, PolicyAction& action, String& reason)
+Ref<WebProcessProxy> WebProcessPool::processForNavigation(WebPageProxy& page, const API::Navigation& navigation, ProcessSwapRequestedByClient processSwapRequestedByClient, String& reason)
 {
-    auto process = processForNavigationInternal(page, navigation, processSwapRequestedByClient, action, reason);
+    auto process = processForNavigationInternal(page, navigation, processSwapRequestedByClient, reason);
 
     if (m_configuration->alwaysKeepAndReuseSwappedProcesses() && process.ptr() != &page.process()) {
         static std::once_flag onceFlag;
@@ -2089,7 +2089,7 @@ Ref<WebProcessProxy> WebProcessPool::processForNavigation(WebPageProxy& page, co
     return process;
 }
 
-Ref<WebProcessProxy> WebProcessPool::processForNavigationInternal(WebPageProxy& page, const API::Navigation& navigation, ProcessSwapRequestedByClient processSwapRequestedByClient, PolicyAction& action, String& reason)
+Ref<WebProcessProxy> WebProcessPool::processForNavigationInternal(WebPageProxy& page, const API::Navigation& navigation, ProcessSwapRequestedByClient processSwapRequestedByClient, String& reason)
 {
     auto& targetURL = navigation.currentRequest().url();
 
@@ -2135,7 +2135,6 @@ Ref<WebProcessProxy> WebProcessPool::processForNavigationInternal(WebPageProxy&
 
     if (auto* backForwardListItem = navigation.targetItem()) {
         if (auto* suspendedPage = backForwardListItem->suspendedPage()) {
-            action = PolicyAction::Suspend;
             reason = "Using target back/forward item's process"_s;
             return suspendedPage->process();
         }
@@ -2198,8 +2197,6 @@ Ref<WebProcessProxy> WebProcessPool::processForNavigationInternal(WebPageProxy&
         }
     }
 
-    action = PolicyAction::Suspend;
-
     if (RefPtr<WebProcessProxy> process = tryTakePrewarmedProcess(page.websiteDataStore())) {
         tryPrewarmWithDomainInformation(*process, targetURL);
         return process.releaseNonNull();
index 9d0f5d9..14071f0 100644 (file)
@@ -440,7 +440,7 @@ public:
     BackgroundWebProcessToken backgroundWebProcessToken() const { return BackgroundWebProcessToken(m_backgroundWebProcessCounter.count()); }
 #endif
 
-    Ref<WebProcessProxy> processForNavigation(WebPageProxy&, const API::Navigation&, ProcessSwapRequestedByClient, WebCore::PolicyAction&, String& reason);
+    Ref<WebProcessProxy> processForNavigation(WebPageProxy&, const API::Navigation&, ProcessSwapRequestedByClient, String& reason);
 
     // SuspendedPageProxy management.
     void addSuspendedPageProxy(std::unique_ptr<SuspendedPageProxy>&&);
@@ -472,7 +472,7 @@ private:
     void platformInitializeWebProcess(WebProcessCreationParameters&);
     void platformInvalidateContext();
 
-    Ref<WebProcessProxy> processForNavigationInternal(WebPageProxy&, const API::Navigation&, ProcessSwapRequestedByClient, WebCore::PolicyAction&, String& reason);
+    Ref<WebProcessProxy> processForNavigationInternal(WebPageProxy&, const API::Navigation&, ProcessSwapRequestedByClient, String& reason);
 
     RefPtr<WebProcessProxy> tryTakePrewarmedProcess(WebsiteDataStore&);
 
index 789df06..d93bb67 100644 (file)
@@ -1,3 +1,16 @@
+2018-11-01  Chris Dumez  <cdumez@apple.com>
+
+        [PSON] WebPageProxy::receivedNavigationPolicyDecision() should not schedule the new load asynchronously when process-swapping
+        https://bugs.webkit.org/show_bug.cgi?id=191076
+
+        Reviewed by Geoffrey Garen.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+        (-[PSONNavigationDelegate init]):
+        (-[PSONNavigationDelegate webView:decidePolicyForNavigationAction:decisionHandler:]):
+
 2018-11-01  Adrian Perez de Castro  <aperez@igalia.com>
 
         Fix build with VIDEO and WEB_AUDIO disabled
index 1f62458..dedca56 100644 (file)
@@ -91,7 +91,7 @@ static RetainPtr<NSURL> clientRedirectDestinationURL;
 @end
 
 @interface PSONNavigationDelegate : NSObject <WKNavigationDelegate> {
-    @public WKNavigationActionPolicy navigationActionPolicyToUse;
+    @public void (^decidePolicyForNavigationAction)(WKNavigationAction *, void (^)(WKNavigationActionPolicy));
 }
 @end
 
@@ -100,7 +100,6 @@ static RetainPtr<NSURL> clientRedirectDestinationURL;
 - (instancetype) init
 {
     self = [super init];
-    navigationActionPolicyToUse = WKNavigationActionPolicyAllow;
     return self;
 }
 
@@ -120,7 +119,10 @@ static RetainPtr<NSURL> clientRedirectDestinationURL;
 {
     ++numberOfDecidePolicyCalls;
     seenPIDs.add([webView _webProcessIdentifier]);
-    decisionHandler(navigationActionPolicyToUse);
+    if (decidePolicyForNavigationAction)
+        decidePolicyForNavigationAction(navigationAction, decisionHandler);
+    else
+        decisionHandler(WKNavigationActionPolicyAllow);
 }
 
 - (void)webView:(WKWebView *)webView didReceiveServerRedirectForProvisionalNavigation:(WKNavigation *)navigation
@@ -442,6 +444,47 @@ TEST(ProcessSwap, Basic)
     EXPECT_EQ(numberOfDecidePolicyCalls, 3);
 }
 
+TEST(ProcessSwap, LoadAfterPolicyDecision)
+{
+    auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
+    processPoolConfiguration.get().processSwapsOnNavigation = YES;
+    processPoolConfiguration.get().prewarmsProcessesAutomatically = YES;
+    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
+
+    auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    [webViewConfiguration setProcessPool:processPool.get()];
+    auto handler = adoptNS([[PSONScheme alloc] init]);
+    [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
+
+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
+    auto navigationDelegate = adoptNS([[PSONNavigationDelegate alloc] init]);
+    [webView setNavigationDelegate:navigationDelegate.get()];
+
+    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main1.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    navigationDelegate->decidePolicyForNavigationAction = ^(WKNavigationAction *, void (^decisionHandler)(WKNavigationActionPolicy)) {
+        decisionHandler(WKNavigationActionPolicyAllow);
+
+        // Synchronously navigate again right after answering the policy delegate for the previous navigation.
+        navigationDelegate->decidePolicyForNavigationAction = nil;
+        NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main2.html"]];
+        [webView loadRequest:request];
+    };
+
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    // Should navigate to pson://www.webkit.org/main2.html and not pson://www.apple.com/main.html.
+    EXPECT_WK_STREQ(@"pson://www.webkit.org/main2.html", [[webView URL] absoluteString]);
+}
+
 TEST(ProcessSwap, NoSwappingForeTLDPlus2)
 {
     auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
@@ -2304,7 +2347,9 @@ TEST(ProcessSwap, APIControlledProcessSwapping)
 
     // Navigating from the above URL to this URL normally should not process swap,
     // but we'll explicitly ask for a swap.
-    navigationDelegate->navigationActionPolicyToUse = _WKNavigationActionPolicyAllowInNewProcess;
+    navigationDelegate->decidePolicyForNavigationAction = ^(WKNavigationAction *, void (^decisionHandler)(WKNavigationActionPolicy)) {
+        decisionHandler(_WKNavigationActionPolicyAllowInNewProcess);
+    };
     [webView loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webit.org/3"]]];
     TestWebKitAPI::Util::run(&done);
     done = false;