Changeset 237446 in webkit

Timestamp:

Oct 25, 2018 10:06:24 PM (5 years ago)

Author:

Chris Dumez

Message:

[PSON] Navigating cross-site with locked history but unlocked back/forward list fails to create a new BackForwardListItem
​https://bugs.webkit.org/show_bug.cgi?id=190915
<rdar://problem/45059069>

Reviewed by Geoffrey Garen.

Source/WebCore:

• history/PageCache.cpp:

(WebCore::canCacheFrame):
Make sure we do not put into PageCache a page whose main frame is showing the initial empty document.
We usually do not try to put those into PageCache because we do not have a HistoryItem to save the
PageCache entry on. However, when we process-swap on a navigation with the history locked, the new
process has a HistoryItem and is initially showing the initial empty document before continuing
the load from the previous process. Note that saving the initial empty document in PageCache would
lead to crashes later on previous the initial empty document's Window is taken and reused for the
next load.

• loader/FrameLoader.cpp:

(WebCore::FrameLoader::load):
Stop assuming that we're continuing a client-side redirect when lockHistory is Yes. It is
lockBackForwardList that is actually Yes when we're doing a client-side redirect.

• loader/PolicyChecker.cpp:

(WebCore::PolicyChecker::checkNavigationPolicy):
Stop using calling the completion handler with an invalid URL when the policy decision is 'Suspend' and
use 'about:blank' instead. Without this change, FrameLoader::continueLoadAfterNavigationPolicy() would
not load 'about:blank' when its AllowNavigationToInvalidURL parameter is No.

Tools:

Add API test coverage.

• TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

Location:

trunk

Files:

• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/history/PageCache.cpp (modified) (1 diff)
• Source/WebCore/loader/FrameLoader.cpp (modified) (1 diff)
• Source/WebCore/loader/PolicyChecker.cpp (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (modified) (3 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2018-10-25  Chris Dumez  <cdumez@apple.com>
+
+        [PSON] Navigating cross-site with locked history but unlocked back/forward list fails to create a new BackForwardListItem
+        https://bugs.webkit.org/show_bug.cgi?id=190915
+        <rdar://problem/45059069>
+
+        Reviewed by Geoffrey Garen.
+
+        * history/PageCache.cpp:
+        (WebCore::canCacheFrame):
+        Make sure we do not put into PageCache a page whose main frame is showing the initial empty document.
+        We usually do not try to put those into PageCache because we do not have a HistoryItem to save the
+        PageCache entry on. However, when we process-swap on a navigation with the history locked, the new
+        process has a HistoryItem and is initially showing the initial empty document before continuing
+        the load from the previous process. Note that saving the initial empty document in PageCache would
+        lead to crashes later on previous the initial empty document's Window is taken and reused for the
+        next load.
+
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::load):
+        Stop assuming that we're continuing a client-side redirect when lockHistory is Yes. It is
+        lockBackForwardList that is actually Yes when we're doing a client-side redirect.
+
+        * loader/PolicyChecker.cpp:
+        (WebCore::PolicyChecker::checkNavigationPolicy):
+        Stop using calling the completion handler with an invalid URL when the policy decision is 'Suspend' and
+        use 'about:blank' instead. Without this change, FrameLoader::continueLoadAfterNavigationPolicy() would
+        not load 'about:blank' when its AllowNavigationToInvalidURL parameter is No.
+
 2018-10-25  Devin Rousso  <drousso@apple.com>
 

trunk/Source/WebCore/history/PageCache.cpp

@@ -86 +86 @@
     }
 
+    if (frame.isMainFrame() && frameLoader.stateMachine().isDisplayingInitialEmptyDocument()) {
+        PCLOG("   -MainFrame is displaying initial empty document");
+        return false;
+    }
+
     DocumentLoader* documentLoader = frameLoader.documentLoader();
     if (!documentLoader) {

trunk/Source/WebCore/loader/FrameLoader.cpp

@@ -1462 +1462 @@
     applyShouldOpenExternalURLsPolicyToNewDocumentLoader(m_frame, loader, request);
 
-    if (request.shouldTreatAsContinuingLoad() && request.lockHistory() == LockHistory::Yes) {
-        // The load we're continuing is a client-side redirect so set things up accordingly.
+    if (request.shouldTreatAsContinuingLoad()) {
         loader->setClientRedirectSourceForHistory(request.clientRedirectSourceForHistory());
-        loader->setIsClientRedirect(true);
-        m_loadType = FrameLoadType::RedirectWithLockedBackForwardList;
+        if (request.lockBackForwardList() == LockBackForwardList::Yes) {
+            loader->setIsClientRedirect(true);
+            m_loadType = FrameLoadType::RedirectWithLockedBackForwardList;
+        }
     }
 

trunk/Source/WebCore/loader/PolicyChecker.cpp

@@ -180 +180 @@
             return function({ }, nullptr, ShouldContinue::No);
         case PolicyAction::Suspend:
-            return function({ }, nullptr, ShouldContinue::ForSuspension);
+            return function({ blankURL() }, nullptr, ShouldContinue::ForSuspension);
         case PolicyAction::Use:
             if (!m_frame.loader().client().canHandleRequest(request)) {

trunk/Tools/ChangeLog

@@ +1 @@
+2018-10-25  Chris Dumez  <cdumez@apple.com>
+
+        [PSON] Navigating cross-site with locked history but unlocked back/forward list fails to create a new BackForwardListItem
+        https://bugs.webkit.org/show_bug.cgi?id=190915
+        <rdar://problem/45059069>
+
+        Reviewed by Geoffrey Garen.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
 2018-10-25  Joseph Pecoraro  <pecoraro@apple.com>
 

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

@@ -281 +281 @@
 )PSONRESOURCE";
 
+static const char* navigationWithLockedHistoryBytes = R"PSONRESOURCE(
+<script>
+let shouldNavigate = true;
+window.addEventListener('pageshow', function(event) {
+    if (event.persisted) {
+        window.webkit.messageHandlers.pson.postMessage("Was persisted");
+        shouldNavigate = false;
+    }
+});
+
+onload = function()
+{
+    if (!shouldNavigate)
+        return;
+
+    // JS navigation via window.location
+    setTimeout(() => {
+        location = "pson://www.apple.com/main.html";
+    }, 10);
+}
+</script>
+)PSONRESOURCE";
+
+static const char* pageCache1Bytes = R"PSONRESOURCE(
+<script>
+window.addEventListener('pageshow', function(event) {
+    if (event.persisted)
+        window.webkit.messageHandlers.pson.postMessage("Was persisted");
+});
+</script>
+)PSONRESOURCE";
+
 #if PLATFORM(MAC)
 
@@ -1268 +1300 @@
 }
 
+static void runNavigationWithLockedHistoryTest(ShouldEnablePSON shouldEnablePSON)
+{
+    auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
+    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]);
+    [handler addMappingFromURLString:@"pson://www.webkit.org/main.html" toData:navigationWithLockedHistoryBytes];
+    [handler addMappingFromURLString:@"pson://www.apple.com/main.html" toData:pageCache1Bytes];
+    [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"pson"];
+
+    auto messageHandler = adoptNS([[PSONMessageHandler alloc] init]);
+    [[webViewConfiguration userContentController] addScriptMessageHandler:messageHandler.get() name:@"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];
+
+    // Page redirects to apple.com.
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto applePID = [webView _webProcessIdentifier];
+    if (shouldEnablePSON == ShouldEnablePSON::Yes)
+        EXPECT_NE(webkitPID, applePID);
+    else
+        EXPECT_EQ(webkitPID, applePID);
+
+    auto* backForwardList = [webView backForwardList];
+    EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [backForwardList.currentItem.URL absoluteString]);
+    EXPECT_TRUE(!backForwardList.forwardItem);
+    EXPECT_EQ(1U, backForwardList.backList.count);
+    EXPECT_WK_STREQ(@"pson://www.webkit.org/main.html", [backForwardList.backItem.URL absoluteString]);
+
+    receivedMessage = false;
+    [webView goBack];
+    TestWebKitAPI::Util::run(&receivedMessage); // Should be restored from PageCache.
+    receivedMessage = false;
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
+    EXPECT_WK_STREQ(@"pson://www.webkit.org/main.html", [backForwardList.currentItem.URL absoluteString]);
+    EXPECT_TRUE(!backForwardList.backItem);
+    EXPECT_EQ(1U, backForwardList.forwardList.count);
+    EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [backForwardList.forwardItem.URL absoluteString]);
+
+    [webView goForward];
+    TestWebKitAPI::Util::run(&done);
+    TestWebKitAPI::Util::run(&receivedMessage); // Should be restored from PageCache.
+    receivedMessage = false;
+    done = false;
+
+    EXPECT_EQ(applePID, [webView _webProcessIdentifier]);
+
+    EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [backForwardList.currentItem.URL absoluteString]);
+    EXPECT_TRUE(!backForwardList.forwardItem);
+    EXPECT_EQ(1U, backForwardList.backList.count);
+    EXPECT_WK_STREQ(@"pson://www.webkit.org/main.html", [backForwardList.backItem.URL absoluteString]);
+}
+
+TEST(ProcessSwap, NavigationWithLockedHistoryWithPSON)
+{
+    runNavigationWithLockedHistoryTest(ShouldEnablePSON::Yes);
+}
+
+TEST(ProcessSwap, NavigationWithLockedHistoryWithoutPSON)
+{
+    runNavigationWithLockedHistoryTest(ShouldEnablePSON::No);
+}
+
 static const char* sessionStorageTestBytes = R"PSONRESOURCE(
 <head>
@@ -1448 +1561 @@
     EXPECT_EQ(4u, [processPool _webProcessCountIgnoringPrewarmed]);
 }
-
-static const char* pageCache1Bytes = R"PSONRESOURCE(
-<script>
-window.addEventListener('pageshow', function(event) {
-    if (event.persisted)
-        window.webkit.messageHandlers.pson.postMessage("Was persisted");
-});
-</script>
-)PSONRESOURCE";
 
 TEST(ProcessSwap, PageCache1)