[PSON] Calling history.back() from inside the load event handler prevents process...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 4 Jan 2019 19:02:19 +0000 (19:02 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 4 Jan 2019 19:02:19 +0000 (19:02 +0000)
https://bugs.webkit.org/show_bug.cgi?id=193120

Reviewed by Alex Christensen.

Source/WebKit:

A HistoryItem is created only *after* we've fired the load event. As a result, if you call
history.back() in JS from inside the load event handler, the current HistoryItem and and
the target HistoryItem will be the same. This is normally not an issue. However, there was
logic inside of WebProcessPool::processForNavigationInternal() which would compare the
processID of the source and destination BackForwardListItems and which would force a process
reuse if both BackForwardListItems came from the same WebContent process. So even though
we swapped when doing a standard load from site A to site B, we would fail to swap if site
B called history.back() from inside its load event handler.

To address the issue, stop relying on the source backforward item's processID. Instead, just
use the WebContent process matching the destination backforward item's processID if it still
exists.

* UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::processForNavigationInternal):

Tools:

Add API test coverage.

* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

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

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

index 3ccbfee..e189581 100644 (file)
@@ -1,3 +1,26 @@
+2019-01-04  Chris Dumez  <cdumez@apple.com>
+
+        [PSON] Calling history.back() from inside the load event handler prevents process-swapping
+        https://bugs.webkit.org/show_bug.cgi?id=193120
+
+        Reviewed by Alex Christensen.
+
+        A HistoryItem is created only *after* we've fired the load event. As a result, if you call
+        history.back() in JS from inside the load event handler, the current HistoryItem and and
+        the target HistoryItem will be the same. This is normally not an issue. However, there was
+        logic inside of WebProcessPool::processForNavigationInternal() which would compare the
+        processID of the source and destination BackForwardListItems and which would force a process
+        reuse if both BackForwardListItems came from the same WebContent process. So even though
+        we swapped when doing a standard load from site A to site B, we would fail to swap if site
+        B called history.back() from inside its load event handler.
+
+        To address the issue, stop relying on the source backforward item's processID. Instead, just
+        use the WebContent process matching the destination backforward item's processID if it still
+        exists.
+
+        * UIProcess/WebProcessPool.cpp:
+        (WebKit::WebProcessPool::processForNavigationInternal):
+
 2019-01-04  Keith Rollin  <krollin@apple.com>
 
         Bring back parent processID for logging
index 6ff5352..c890c89 100644 (file)
@@ -2172,25 +2172,18 @@ void WebProcessPool::processForNavigationInternal(WebPageProxy& page, const API:
     if (navigation.hasOpenedFrames())
         return completionHandler(page.process(), nullptr, "Browsing context has opened other windows"_s);
 
-    if (auto* backForwardListItem = navigation.targetItem()) {
-        if (auto* suspendedPage = backForwardListItem->suspendedPage()) {
+    if (auto* targetItem = navigation.targetItem()) {
+        if (auto* suspendedPage = targetItem->suspendedPage()) {
             return suspendedPage->waitUntilReadyToUnsuspend([createNewProcess = WTFMove(createNewProcess), completionHandler = WTFMove(completionHandler)](SuspendedPageProxy* suspendedPage) mutable {
                 if (!suspendedPage)
                     return completionHandler(createNewProcess(), nullptr, "Using new process because target back/forward item's suspended page is not reusable"_s);
                 Ref<WebProcessProxy> process = suspendedPage->process();
-                completionHandler(WTFMove(process), suspendedPage, "Using target back/forward item's process"_s);
+                completionHandler(WTFMove(process), suspendedPage, "Using target back/forward item's process and suspended page"_s);
             });
         }
 
-        // If the target back/forward item and the current back/forward item originated
-        // in the same WebProcess then we should reuse the current WebProcess.
-        if (auto* fromItem = navigation.fromItem()) {
-            auto uiProcessIdentifier = Process::identifier();
-            // In case of session restore, the item's process identifier is the UIProcess' identifier, in which case we do not want to do this check
-            // or we'd never swap after a session restore.
-            if (fromItem->itemID().processIdentifier == backForwardListItem->itemID().processIdentifier && backForwardListItem->itemID().processIdentifier != uiProcessIdentifier)
-                return completionHandler(page.process(), nullptr, "Source and target back/forward item originated in the same process"_s);
-        }
+        if (auto* process = WebProcessProxy::processForIdentifier(targetItem->itemID().processIdentifier))
+            return completionHandler(*process, nullptr, "Using target back/forward item's process"_s);
     }
 
     if (navigation.treatAsSameOriginNavigation())
index 15a1722..bfdc5f0 100644 (file)
@@ -1,5 +1,16 @@
 2019-01-04  Chris Dumez  <cdumez@apple.com>
 
+        [PSON] Calling history.back() from inside the load event handler prevents process-swapping
+        https://bugs.webkit.org/show_bug.cgi?id=193120
+
+        Reviewed by Alex Christensen.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
+2019-01-04  Chris Dumez  <cdumez@apple.com>
+
         Crash under WebProcessPool::addSuspendedPage()
         https://bugs.webkit.org/show_bug.cgi?id=193110
 
index 47a27e3..96133a9 100644 (file)
@@ -2810,6 +2810,62 @@ TEST(ProcessSwap, APIControlledProcessSwapping)
     EXPECT_NE(pid1, pid3);
 }
 
+static const char* navigateToCrossSiteThenBackFromJSBytes = R"PSONRESOURCE(
+<script>
+onpageshow = function(event) {
+    // Location changes need to happen outside the onload handler to generate history entries.
+    setTimeout(function() {
+      window.location.href = "pson://www.apple.com/main.html";
+    }, 0);
+}
+
+</script>
+)PSONRESOURCE";
+
+static const char* navigateBackFromJSBytes = R"PSONRESOURCE(
+<body onload='history.back()'></body>
+)PSONRESOURCE";
+
+TEST(ProcessSwap, NavigateToCrossSiteThenBackFromJS)
+{
+    auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
+    processPoolConfiguration.get().processSwapsOnNavigation = YES;
+    processPoolConfiguration.get().pageCacheEnabled = NO;
+    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
+
+    auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    [webViewConfiguration setProcessPool:processPool.get()];
+    auto handler = adoptNS([[PSONScheme alloc] init]);
+    [handler addMappingFromURLString:@"pson://www.webkit.org/main.html" toData:navigateToCrossSiteThenBackFromJSBytes]; // Navigates to "pson://www.apple.com/main.html".
+    [handler addMappingFromURLString:@"pson://www.apple.com/main.html" toData:navigateBackFromJSBytes];
+    [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()];
+
+    numberOfDecidePolicyCalls = 0;
+    [webView loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]]];
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+    auto webkitPID = [webView _webProcessIdentifier];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+    auto applePID = [webView _webProcessIdentifier];
+    EXPECT_NE(webkitPID, applePID); // Should have process-swapped when going from webkit.org to apple.com.
+
+    // Page now calls history.back() to navigate back to webkit.org.
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    EXPECT_EQ(3, numberOfDecidePolicyCalls);
+
+    // Should have process-swapped when going from apple.com to webkit.org.
+    // PID is not necessarily webkitPID because PageCache is disabled.
+    EXPECT_NE(applePID, [webView _webProcessIdentifier]);
+}
+
 #if PLATFORM(MAC)
 
 static const char* saveOpenerTestBytes = R"PSONRESOURCE(