[PSON] Navigating cross-site with locked history but unlocked back/forward list fails...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 26 Oct 2018 05:06:24 +0000 (05:06 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 26 Oct 2018 05:06:24 +0000 (05:06 +0000)
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:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@237446 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/history/PageCache.cpp
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/loader/PolicyChecker.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

index 0c0410d..ae5da07 100644 (file)
@@ -1,3 +1,32 @@
+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>
 
         Fix build after r237431 for platforms that don't support FULLSCREEN_API
index 7d0e09b..3cf836c 100644 (file)
@@ -85,6 +85,11 @@ static bool canCacheFrame(Frame& frame, DiagnosticLoggingClient& diagnosticLoggi
         return false;
     }
 
+    if (frame.isMainFrame() && frameLoader.stateMachine().isDisplayingInitialEmptyDocument()) {
+        PCLOG("   -MainFrame is displaying initial empty document");
+        return false;
+    }
+
     DocumentLoader* documentLoader = frameLoader.documentLoader();
     if (!documentLoader) {
         PCLOG("   -There is no DocumentLoader object");
index c5f9206..38fda8b 100644 (file)
@@ -1461,11 +1461,12 @@ void FrameLoader::load(FrameLoadRequest&& request)
     addSameSiteInfoToRequestIfNeeded(loader->request());
     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;
+        }
     }
 
     SetForScope<bool> currentLoadShouldBeTreatedAsContinuingLoadGuard(m_currentLoadShouldBeTreatedAsContinuingLoad, request.shouldTreatAsContinuingLoad());
index 63f4398..4f90f94 100644 (file)
@@ -179,7 +179,7 @@ void PolicyChecker::checkNavigationPolicy(ResourceRequest&& request, const Resou
         case PolicyAction::Ignore:
             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)) {
                 handleUnimplementablePolicy(m_frame.loader().client().cannotShowURLError(request));
index 2db009e..526ba44 100644 (file)
@@ -1,3 +1,15 @@
+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>
 
         Web Inspector: Add a watchlist message rule to warn about feature checking new inspector protocol changes
index 09451cd..099b0c2 100644 (file)
@@ -280,6 +280,38 @@ onload = () => {
 </body>
 )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)
 
 static const char* windowOpenCrossSiteNoOpenerTestBytes = R"PSONRESOURCE(
@@ -1267,6 +1299,87 @@ TEST(ProcessSwap, CrossSiteClientSideRedirectWithPSON)
     runClientSideRedirectTest(ShouldEnablePSON::Yes);
 }
 
+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>
 <script>
@@ -1448,15 +1561,6 @@ TEST(ProcessSwap, ThreePreviousProcessesRemains)
     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)
 {
     auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);