Attachment 360395 Details for Bug 193932 – Patch

[patch] Patch

bug-193932-20190128160421.patch (text/plain), 8.28 KB, created by Chris Dumez on 2019-01-28 16:04:22 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-01-28 16:04:22 PST

Size: 8.28 KB

patch

obsolete

>Subversion Revision: 240599
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 5d0f5d9cf49f1af6c9b025eddf930777f49a1bfd..d641ab3f60c66ebb90da44ba98a25994f8f85eda 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,27 @@
>+2019-01-28  Chris Dumez  <cdumez@apple.com>
>+
>+        REGRESSION (PSON): Twitter link gets stuck at t.co after navigating back in tab
>+        https://bugs.webkit.org/show_bug.cgi?id=193932
>+        <rdar://problem/47598947>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        When doing a client side redirect from origin A to origin B, we would swap process and
>+        create a SuspendedPageProxy and save it on the source BackForwardListItem. The issue is
>+        that the BackForwardList is locked for such redirect so we end up updating the current
>+        BackForwardListItem with origin B's URL while origin A's suspended page remained on
>+        the item. When going to another URL in the same origin A, we would not create a suspended
>+        page since no process-swap would occur. When pressing the back button, we would go back
>+        to the previous BackForwardListItem and use its SuspendedPageProxy, which is for the
>+        wrong URL (The pre-client redirect one).
>+
>+        To address the issue, we no longer create a SuspendedPageProxy for cross-site client side
>+        redirects. There will be no way no go back to this suspended page anyway since the
>+        back/forward list item will be updated with the redirection URL.
>+
>+        * UIProcess/WebPageProxy.cpp:
>+        (WebKit::WebPageProxy::suspendCurrentPageIfPossible):
>+
> 2019-01-28  Chris Dumez  <cdumez@apple.com>
> 
>         Regression(PSON) Crash under WebPageProxy::didStartProgress()
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index 756f68678a0e81f9a7cebac86a180edd372ba4ad..ca17ee4f7a73b666a03fd1847cd9cb614fc642eb 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -762,15 +762,20 @@ bool WebPageProxy::suspendCurrentPageIfPossible(API::Navigation& navigation, Opt
>     if (isPageOpenedByDOMShowingInitialEmptyDocument())
>         return false;
> 
>-    auto* currentItem = navigation.fromItem();
>-    if (!currentItem) {
>+    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());
>         return false;
>     }
> 
>-    auto suspendedPage = std::make_unique<SuspendedPageProxy>(*this, m_process.copyRef(), *currentItem, *mainFrameID);
>+    // 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())
>+        return false;
>+
>+    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(), currentItem->itemID().logString());
>+    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());
> 
>     m_process->processPool().addSuspendedPage(WTFMove(suspendedPage));
>     return true;
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index f1d60cdee7414cb9faeaf3d433116213c1352463..c945d5a395bec68f902fca176e648b9f97ca9499 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,15 @@
>+2019-01-28  Chris Dumez  <cdumez@apple.com>
>+
>+        REGRESSION (PSON): Twitter link gets stuck at t.co after navigating back in tab
>+        https://bugs.webkit.org/show_bug.cgi?id=193932
>+        <rdar://problem/47598947>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add API test coverage.
>+
>+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
>+
> 2019-01-28  Chris Dumez  <cdumez@apple.com>
> 
>         Regression(PSON) Crash under WebPageProxy::didStartProgress()
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>index 62caa3d0f75586bac5e6608bbaded250bd96ac53..d5dc35bb7c1f8cdc6de50141ee15ea0b17f58a8d 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>@@ -1846,11 +1846,86 @@ TEST(ProcessSwap, CrossSiteClientSideRedirectFromFileURL)
>     auto pid2 = [webView _webProcessIdentifier];
>     EXPECT_NE(pid1, pid2);
> 
>-    EXPECT_EQ(2U, [processPool _webProcessCountIgnoringPrewarmed]);
>+    EXPECT_EQ(1U, [processPool _webProcessCountIgnoringPrewarmed]);
>     EXPECT_TRUE(willPerformClientRedirect);
>     EXPECT_TRUE(didPerformClientRedirect);
> }
> 
>+TEST(ProcessSwap, NavigateBackAfterClientSideRedirect)
>+{
>+    auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
>+    processPoolConfiguration.get().processSwapsOnNavigation = 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]);
>+    [handler addMappingFromURLString:@"pson://www.webkit.org/main.html" toData:linkToCrossSiteClientSideRedirectBytes];
>+    [handler addMappingFromURLString:@"pson://www.google.com/clientSideRedirect.html" toData:crossSiteClientSideRedirectBytes];
>+    [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()];
>+
>+    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
>+    [webView loadRequest:request];
>+
>+    TestWebKitAPI::Util::run(&done);
>+    done = false;
>+
>+    auto webkitPID = [webView _webProcessIdentifier];
>+
>+    willPerformClientRedirect = false;
>+    didPerformClientRedirect = false;
>+
>+    // Navigate to the page doing a client-side redirect to apple.com.
>+    [webView evaluateJavaScript:@"testLink.click()" completionHandler:nil];
>+
>+    TestWebKitAPI::Util::run(&done);
>+    done = false;
>+
>+    EXPECT_WK_STREQ(@"pson://www.google.com/clientSideRedirect.html", [[webView URL] absoluteString]);
>+    auto googlePID = [webView _webProcessIdentifier];
>+    EXPECT_NE(webkitPID, googlePID);
>+
>+    TestWebKitAPI::Util::run(&done);
>+    done = false;
>+
>+    EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [[webView URL] absoluteString]);
>+
>+    auto applePID = [webView _webProcessIdentifier];
>+    EXPECT_NE(webkitPID, applePID);
>+    EXPECT_NE(webkitPID, googlePID);
>+
>+    EXPECT_TRUE(willPerformClientRedirect);
>+    EXPECT_TRUE(didPerformClientRedirect);
>+    EXPECT_WK_STREQ(@"pson://www.google.com/clientSideRedirect.html", [clientRedirectSourceURL absoluteString]);
>+    EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [clientRedirectDestinationURL absoluteString]);
>+
>+    willPerformClientRedirect = false;
>+    didPerformClientRedirect = false;
>+
>+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main2.html"]];
>+    [webView loadRequest:request];
>+
>+    TestWebKitAPI::Util::run(&done);
>+    done = false;
>+
>+    EXPECT_EQ(applePID, [webView _webProcessIdentifier]);
>+
>+    [webView goBack];
>+
>+    TestWebKitAPI::Util::run(&done);
>+    done = false;
>+
>+    EXPECT_EQ(applePID, [webView _webProcessIdentifier]);
>+    EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [[webView URL] absoluteString]);
>+
>+    EXPECT_FALSE(willPerformClientRedirect);
>+    EXPECT_FALSE(didPerformClientRedirect);
>+}
>+
> static void runNavigationWithLockedHistoryTest(ShouldEnablePSON shouldEnablePSON)
> {
>     auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);

Actions: View | Formatted Diff | Diff

Attachments on bug 193932: 360395