Attachment 358270 Details for Bug 193120 – Patch

[patch] Patch

bug-193120-20190103125240.patch (text/plain), 7.99 KB, created by Chris Dumez on 2019-01-03 12:52:41 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-01-03 12:52:41 PST

Size: 7.99 KB

patch

obsolete

>Subversion Revision: 239579
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 03c3dea385567be5fe5addb363d453d0128be63c..0e9f58455f98d7694505b2d0268fed3f293438b0 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,26 @@
>+2019-01-03  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 NOBODY (OOPS!).
>+
>+        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-03  Chris Dumez  <cdumez@apple.com>
> 
>         Crash under WebPageProxy::continueNavigationInNewProcess()
>diff --git a/Source/WebKit/UIProcess/WebProcessPool.cpp b/Source/WebKit/UIProcess/WebProcessPool.cpp
>index 71ec76ee8711953e7969a95b4d1deadcedd05550..c95c93b8155010432f697e698eb6a6a77b84bc4f 100644
>--- a/Source/WebKit/UIProcess/WebProcessPool.cpp
>+++ b/Source/WebKit/UIProcess/WebProcessPool.cpp
>@@ -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())
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 83074fef25b72b9b2d628fe99e4babf06e99e267..44d83f5789dc69407a055986009bb13dfcee1470 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,14 @@
>+2019-01-03  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 NOBODY (OOPS!).
>+
>+        Add API test coverage.
>+
>+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
>+
> 2019-01-03  Chris Dumez  <cdumez@apple.com>
> 
>         Crash under WebProcessPool::addSuspendedPage()
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>index 47a27e395401dfc217ea9dee61f26cf1ef47f17b..96133a9877799018a5b59410641a1fd268520fef 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>@@ -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(

Actions: View | Formatted Diff | Diff

Attachments on bug 193120: 358270