Changeset 241606 in webkit

Timestamp:

Feb 15, 2019 1:03:53 PM (5 years ago)

Author:

Chris Dumez

Message:

Regression(PSON) Navigating quickly back and forth can lead to getting 'about:blank' in the backforward list
​https://bugs.webkit.org/show_bug.cgi?id=194717
<rdar://problem/47884404>

Reviewed by Brady Eidson.

Source/WebKit:

When the client does a history navigation, the UIProcess sends a WebPage::GoToBackForwardItem IPC to the
WebProcess and the WebProcess sends a WebPageProxy::BackForwardGoToItem IPC back to the UIProcess to
update the current item in the BackForwardList. This means that there is a slight delay between the
point a client requests a history navigation and the point where the BackForwardList's current item gets
update. This delay is pre-existing behavior and not new to PSON.

However, with PSON enabled, if we decide to process-swap for the history navigation, we'll tell the
previous (committed) process to ignore the load and we ask a new (provisional) process to do the history
navigation. When the previous process receives the request to ignore the history navigation, it restores
the History's current item to the one previous the navigation, which sends a WebPageProxy::GoToBackForwardItem
IPC to the UIProcess to update the BackForwardList as well. In parallel, the new process starts the
history navigation and also sends a WebPageProxy::GoToBackForwardItem to update the BackForwardList's
current item as well. We end up with a race between the 2 GoToBackForwardItem IPC messages coming from
the old and new process. If the old process's message loses the race, we end up with the wrong current
history item getting set in the UIProcess. Later, when we commit the provisional load and try to suspend
the previous page, we would save the SuspendedPage on the *wrong* BackForwardList item. If one tries to
load this BackForwardList item later, we'll use its SuspendedPage and try to unsuspend it. However,
because the PageCache entry is saved on another HistoryItem than the one getting loaded in the WebProcess
side, we attempt to do a regular load instead of a PageCache restore. We end up failing the load because
pages cannot trigger new loads while in page cache. Because the load fails, we end up loading the
initial empty document and this is how we end up with 'about:blank' in the back forward list.

To address the issue, update WebPageProxy::backForwardGoToItem() to ignore messages from the old/committed
WebProcess when there is a pending provisional load. If the committed processes starts a legit new
load, it would clear any existing pending provisional load before attempting the call backForwardGoToItem().
As a result, ignoring such messages from the old processes when there is a pending provisional load is
safe.

In the future, we should probably move more of the history / backForwardList management to the UIProcess
to avoid this sort of issues. This would be a much larger refactoring though so I am going with this
simpler fix that is easily cherry-pickable for now.

• UIProcess/WebPageProxy.cpp:

(WebKit::WebPageProxy::suspendCurrentPageIfPossible):
(WebKit::WebPageProxy::continueNavigationInNewProcess):
(WebKit::WebPageProxy::backForwardGoToItem):

Tools:

Add API test coverage.

• TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

Location:

trunk

Files:

• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/WebPageProxy.cpp (modified) (5 diffs)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-02-15  Chris Dumez  <cdumez@apple.com>
+
+        Regression(PSON) Navigating quickly back and forth can lead to getting 'about:blank' in the backforward list
+        https://bugs.webkit.org/show_bug.cgi?id=194717
+        <rdar://problem/47884404>
+
+        Reviewed by Brady Eidson.
+
+        When the client does a history navigation, the UIProcess sends a WebPage::GoToBackForwardItem IPC to the
+        WebProcess and the WebProcess sends a WebPageProxy::BackForwardGoToItem IPC back to the UIProcess to
+        update the current item in the BackForwardList. This means that there is a slight delay between the
+        point a client requests a history navigation and the point where the BackForwardList's current item gets
+        update. This delay is pre-existing behavior and not new to PSON.
+
+        However, with PSON enabled, if we decide to process-swap for the history navigation, we'll tell the
+        previous (committed) process to ignore the load and we ask a new (provisional) process to do the history
+        navigation. When the previous process receives the request to ignore the history navigation, it restores
+        the History's current item to the one previous the navigation, which sends a WebPageProxy::GoToBackForwardItem
+        IPC to the UIProcess to update the BackForwardList as well. In parallel, the new process starts the
+        history navigation and also sends a WebPageProxy::GoToBackForwardItem to update the BackForwardList's
+        current item as well. We end up with a race between the 2 GoToBackForwardItem IPC messages coming from
+        the old and new process. If the old process's message loses the race, we end up with the wrong current
+        history item getting set in the UIProcess. Later, when we commit the provisional load and try to suspend
+        the previous page, we would save the SuspendedPage on the *wrong* BackForwardList item. If one tries to
+        load this BackForwardList item later, we'll use its SuspendedPage and try to unsuspend it. However,
+        because the PageCache entry is saved on another HistoryItem than the one getting loaded in the WebProcess
+        side, we attempt to do a regular load instead of a PageCache restore. We end up failing the load because
+        pages cannot trigger new loads while in page cache. Because the load fails, we end up loading the
+        initial empty document and this is how we end up with 'about:blank' in the back forward list.
+
+        To address the issue, update WebPageProxy::backForwardGoToItem() to ignore messages from the old/committed
+        WebProcess when there is a pending provisional load. If the committed processes starts a legit new
+        load, it would clear any existing pending provisional load before attempting the call backForwardGoToItem().
+        As a result, ignoring such messages from the old processes when there is a pending provisional load is
+        safe.
+
+        In the future, we should probably move more of the history / backForwardList management to the UIProcess
+        to avoid this sort of issues. This would be a much larger refactoring though so I am going with this
+        simpler fix that is easily cherry-pickable for now.
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::suspendCurrentPageIfPossible):
+        (WebKit::WebPageProxy::continueNavigationInNewProcess):
+        (WebKit::WebPageProxy::backForwardGoToItem):
+
 2019-02-15  Alex Christensen  <achristensen@webkit.org>
 

trunk/Source/WebKit/UIProcess/WebPageProxy.cpp

@@ -246 +246 @@
 
 #define RELEASE_LOG_IF_ALLOWED(channel, fmt, ...) RELEASE_LOG_IF(isAlwaysOnLoggingAllowed(), channel, "%p - WebPageProxy::" fmt, this, ##__VA_ARGS__)
+#define RELEASE_LOG_ERROR_IF_ALLOWED(channel, fmt, ...) RELEASE_LOG_ERROR_IF(isAlwaysOnLoggingAllowed(), channel, "%p - WebPageProxy::" fmt, this, ##__VA_ARGS__)
 
 // Represents the number of wheel events we can hold in the queue before we start pushing them preemptively.
@@ -761 +762 @@
 
     // If the client forced a swap then it may not be Web-compatible to suspend the previous page because other windows may have an opener link to the page.
-    if (processSwapRequestedByClient == ProcessSwapRequestedByClient::Yes)
+    if (processSwapRequestedByClient == ProcessSwapRequestedByClient::Yes) {
+        RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "suspendCurrentPageIfPossible: Not suspending current page for process pid %i because the swap was requested by the client", m_process->processIdentifier());
         return false;
-
-    if (isPageOpenedByDOMShowingInitialEmptyDocument())
+    }
+
+    if (isPageOpenedByDOMShowingInitialEmptyDocument()) {
+        RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "suspendCurrentPageIfPossible: Not suspending current page for process pid %i because it is showing the initial empty document", m_process->processIdentifier());
         return false;
+    }
 
     auto* fromItem = navigation.fromItem();
     if (!fromItem) {
-        LOG(ProcessSwapping, "WebPageProxy %" PRIu64 " unable to create suspended page for process pid %i - No current back/forward item", pageID(), m_process->processIdentifier());
+        RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "suspendCurrentPageIfPossible: Not suspending current page for process pid %i because the navigation does not have a fromItem", m_process->processIdentifier());
         return false;
     }
@@ -775 +780 @@
     // If the source and the destination back / forward list items are the same, then this is a client-side redirect. In this case,
     // there is no need to suspend the previous page as there will be no way to get back to it.
-    if (fromItem == m_backForwardList->currentItem())
+    if (fromItem == m_backForwardList->currentItem()) {
+        RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "suspendCurrentPageIfPossible: Not suspending current page for process pid %i because this is a client-side redirect", m_process->processIdentifier());
         return false;
-
+    }
+
+    if (fromItem->url() != pageLoadState().url()) {
+        RELEASE_LOG_ERROR_IF_ALLOWED(ProcessSwapping, "suspendCurrentPageIfPossible: Not suspending current page for process pid %i because fromItem's URL does not match the page URL.", m_process->processIdentifier());
+        ASSERT_NOT_REACHED();
+        return false;
+    }
+
+    RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "suspendCurrentPageIfPossible: Suspending current page for process pid %i", m_process->processIdentifier());
     auto suspendedPage = std::make_unique<SuspendedPageProxy>(*this, m_process.copyRef(), *fromItem, *mainFrameID);
 
@@ -2837 +2851 @@
 
     if (m_provisionalPage) {
+        RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "continueNavigationInNewProcess: There is already a pending provisional load, cancelling it (provisonalNavigationID: %llu, navigationID: %llu)", m_provisionalPage->navigationID(), navigation.navigationID());
         if (m_provisionalPage->navigationID() != navigation.navigationID())
             m_provisionalPage->cancel();
@@ -5476 +5491 @@
 void WebPageProxy::backForwardGoToItem(const BackForwardItemIdentifier& itemID, SandboxExtension::Handle& sandboxExtensionHandle)
 {
+    // On process swap, we tell the previous process to ignore the load, which causes it so restore its current back forward item to its previous
+    // value. Since the load is really going on in a new provisional process, we want to ignore such requests from the committed process.
+    // Any real new load in the committed process would have cleared m_provisionalPage.
+    if (m_provisionalPage)
+        return;
+
     backForwardGoToItemShared(m_process.copyRef(), itemID, sandboxExtensionHandle);
 }

trunk/Tools/ChangeLog

@@ +1 @@
+2019-02-15  Chris Dumez  <cdumez@apple.com>
+
+        Regression(PSON) Navigating quickly back and forth can lead to getting 'about:blank' in the backforward list
+        https://bugs.webkit.org/show_bug.cgi?id=194717
+        <rdar://problem/47884404>
+
+        Reviewed by Brady Eidson.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
 2019-02-15  Youenn Fablet  <youenn@apple.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

@@ -2024 +2024 @@
 }
 
+static void runQuickBackForwardNavigationTest(ShouldEnablePSON shouldEnablePSON)
+{
+    auto processPoolConfiguration = psonProcessPoolConfiguration();
+    processPoolConfiguration.get().processSwapsOnNavigation = shouldEnablePSON == ShouldEnablePSON::Yes ? YES : 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]);
+    [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"pson"];
+
+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
+    auto delegate = adoptNS([[PSONNavigationDelegate alloc] init]);
+    [webView setNavigationDelegate:delegate.get()];
+
+    [webView configuration].preferences.safeBrowsingEnabled = NO;
+
+    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main1.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto webkitPID = [webView _webProcessIdentifier];
+
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main2.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
+
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto applePID = [webView _webProcessIdentifier];
+    if (shouldEnablePSON == ShouldEnablePSON::Yes)
+        EXPECT_NE(webkitPID, applePID);
+    else
+        EXPECT_EQ(webkitPID, applePID);
+
+    for (unsigned i = 0; i < 10; ++i) {
+        [webView goBack];
+        TestWebKitAPI::Util::sleep(0.1);
+        [webView goForward];
+        TestWebKitAPI::Util::spinRunLoop(0.1);
+    }
+
+    Vector<String> backForwardListURLs;
+    auto* backForwardList = [webView backForwardList];
+    for (unsigned i = 0; i < backForwardList.backList.count; ++i)
+        backForwardListURLs.append([backForwardList.backList[i].URL absoluteString]);
+    backForwardListURLs.append([backForwardList.currentItem.URL absoluteString]);
+    for (unsigned i = 0; i < backForwardList.forwardList.count; ++i)
+        backForwardListURLs.append([backForwardList.forwardList[i].URL absoluteString]);
+    EXPECT_EQ(3u, backForwardListURLs.size());
+    EXPECT_WK_STREQ("pson://www.webkit.org/main1.html", backForwardListURLs[0]);
+    EXPECT_WK_STREQ("pson://www.webkit.org/main2.html", backForwardListURLs[1]);
+    EXPECT_WK_STREQ("pson://www.apple.com/main.html", backForwardListURLs[2]);
+}
+
+TEST(ProcessSwap, QuickBackForwardNavigationWithoutPSON)
+{
+    runQuickBackForwardNavigationTest(ShouldEnablePSON::No);
+}
+
+TEST(ProcessSwap, QuickBackForwardNavigationWithPSON)
+{
+    runQuickBackForwardNavigationTest(ShouldEnablePSON::Yes);
+}
+
 TEST(ProcessSwap, NavigationWithLockedHistoryWithoutPSON)
 {