Attachment 362144 Details for Bug 194717 – Patch

[patch] Patch

bug-194717-20190215121835.patch (text/plain), 13.38 KB, created by Chris Dumez on 2019-02-15 12:18:36 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-02-15 12:18:36 PST

Size: 13.38 KB

patch

obsolete

>Subversion Revision: 241557
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 008f9d6544055b05bc01b4837bccc89989ac375c..c1488ad2f3940ced323fed537e8149d3d5063d0e 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,48 @@
>+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 NOBODY (OOPS!).
>+
>+        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-14  Chris Dumez  <cdumez@apple.com>
> 
>         [PSON] Introduce a WebContent Process cache
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index 174531a4345fb85bbdc8089d5fd93f7e35781ff9..f0a3d4c8fafaa111337f87e2dd058c81260baade 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -245,6 +245,7 @@
> #define MESSAGE_CHECK_URL(process, url) MESSAGE_CHECK_BASE(checkURLReceivedFromCurrentOrPreviousWebProcess(process, url), process->connection())
> 
> #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.
> static const unsigned wheelEventQueueSizeThreshold = 10;
>@@ -760,23 +761,36 @@ bool WebPageProxy::suspendCurrentPageIfPossible(API::Navigation& navigation, Opt
>         return false;
> 
>     // 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;
>     }
> 
>     // 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);
> 
>     LOG(ProcessSwapping, "WebPageProxy %" PRIu64 " created suspended page %s for process pid %i, back/forward item %s" PRIu64, pageID(), suspendedPage->loggingString(), m_process->processIdentifier(), fromItem->itemID().logString());
>@@ -2836,6 +2850,7 @@ void WebPageProxy::continueNavigationInNewProcess(API::Navigation& navigation, s
>     LOG(Loading, "Continuing navigation %" PRIu64 " '%s' in a new web process", navigation.navigationID(), navigation.loggingString());
> 
>     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();
>         m_provisionalPage = nullptr;
>@@ -5475,6 +5490,12 @@ void WebPageProxy::backForwardAddItem(BackForwardListItemState&& itemState)
> 
> 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);
> }
> 
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index d9750ee42f028baa8311312f45a02594f64532d2..a37b248312b73f43bd2478415dcdc2ccb941c2e2 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,15 @@
>+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 NOBODY (OOPS!).
>+
>+        Add API test coverage.
>+
>+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
>+
> 2019-02-14  Chris Dumez  <cdumez@apple.com>
> 
>         [PSON] Introduce a WebContent Process cache
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>index c2778501858dbe8b7c5bc59c1bd440c5783b614e..510ffba5b3e54c8e9f8fd52f9011c64fb944d3fe 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>@@ -2023,6 +2023,81 @@ TEST(ProcessSwap, NavigationWithLockedHistoryWithPSON)
>     runNavigationWithLockedHistoryTest(ShouldEnablePSON::Yes);
> }
> 
>+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)
> {
>     runNavigationWithLockedHistoryTest(ShouldEnablePSON::No);

Actions: View | Formatted Diff | Diff

Attachments on bug 194717: 362144