REGRESSION(PSON, r240660): Navigation over process boundary is flashy…

… when using Cmd-left/right arrow to navigate

https://bugs.webkit.org/show_bug.cgi?id=195684
<rdar://problem/48294714>

Reviewed by Antti Koivisto.

Source/WebCore:

The issue was caused by us failing to suspend the current page on navigation because the source and
target WebBackForwardListItem are identical. The source WebBackForwardListItem was wrong.

When a navigation is triggered by the WebContent process (and not the UIProcess), we create the Navigation
object in WebPageProxy::decidePolicyForNavigationAction(). For the navigation's targetItem, we use the
target item identifier provided by the WebContent process via the NavigationActionData. However,
for the source item, we would use the WebBackForwardList's currentItem in the UIProcess. The issue
is that the WebBackForwardList's currentItem usually has already been updated to be the target
item via a WebPageProxy::BackForwardGoToItem() synchronous IPC.

To avoid raciness and given that the current history management is fragile (as it is managed by
both the UIProcess and the WebProcess), I am now passing the source item identifier in
addition to the target item identifier in the NavigationActionData that is sent by the WebProcess.
This is a lot less error prone, the WebProcess knows more accurately which history items it is going
from and to.

* loader/FrameLoader.cpp:
(WebCore::FrameLoader::loadURLIntoChildFrame):
(WebCore::FrameLoader::loadDifferentDocumentItem):
(WebCore::FrameLoader::loadItem):
(WebCore::FrameLoader::retryAfterFailedCacheOnlyMainResourceLoad):
* loader/FrameLoader.h:
* loader/HistoryController.cpp:
(WebCore::HistoryController::recursiveGoToItem):
* loader/NavigationAction.cpp:
(WebCore::NavigationAction::setSourceBackForwardItem):
* loader/NavigationAction.h:
(WebCore::NavigationAction::sourceBackForwardItemIdentifier const):

Source/WebKit:

* Shared/NavigationActionData.cpp:
(WebKit::NavigationActionData::encode const):
(WebKit::NavigationActionData::decode):
* Shared/NavigationActionData.h:
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::decidePolicyForNavigationAction):
(WebKit::WebPageProxy::backForwardAddItem):
* WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
(WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):

Tools:

Add API test coverage.

* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:


Canonical link: https://commits.webkit.org/209994@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@242903 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog

@@ -1,3 +1,40 @@
+2019-03-13  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION(PSON, r240660): Navigation over process boundary is flashy when using Cmd-left/right arrow to navigate
+        https://bugs.webkit.org/show_bug.cgi?id=195684
+        <rdar://problem/48294714>
+
+        Reviewed by Antti Koivisto.
+
+        The issue was caused by us failing to suspend the current page on navigation because the source and
+        target WebBackForwardListItem are identical. The source WebBackForwardListItem was wrong.
+
+        When a navigation is triggered by the WebContent process (and not the UIProcess), we create the Navigation
+        object in WebPageProxy::decidePolicyForNavigationAction(). For the navigation's targetItem, we use the
+        target item identifier provided by the WebContent process via the NavigationActionData. However,
+        for the source item, we would use the WebBackForwardList's currentItem in the UIProcess. The issue
+        is that the WebBackForwardList's currentItem usually has already been updated to be the target
+        item via a WebPageProxy::BackForwardGoToItem() synchronous IPC.
+
+        To avoid raciness and given that the current history management is fragile (as it is managed by
+        both the UIProcess and the WebProcess), I am now passing the source item identifier in
+        addition to the target item identifier in the NavigationActionData that is sent by the WebProcess.
+        This is a lot less error prone, the WebProcess knows more accurately which history items it is going
+        from and to.
+
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::loadURLIntoChildFrame):
+        (WebCore::FrameLoader::loadDifferentDocumentItem):
+        (WebCore::FrameLoader::loadItem):
+        (WebCore::FrameLoader::retryAfterFailedCacheOnlyMainResourceLoad):
+        * loader/FrameLoader.h:
+        * loader/HistoryController.cpp:
+        (WebCore::HistoryController::recursiveGoToItem):
+        * loader/NavigationAction.cpp:
+        (WebCore::NavigationAction::setSourceBackForwardItem):
+        * loader/NavigationAction.h:
+        (WebCore::NavigationAction::sourceBackForwardItemIdentifier const):
+
 2019-03-13  Jer Noble  <jer.noble@apple.com>
 
         Add AggregateLogger, a Logger specialization for singleton classes.

Source/WebCore/loader/FrameLoader.cpp

@@ -974,7 +974,7 @@ void FrameLoader::loadURLIntoChildFrame(const URL& url, const String& referer, F
     if (parentItem && parentItem->children().size() && isBackForwardLoadType(loadType()) && !m_frame.document()->loadEventFinished()) {
         if (auto* childItem = parentItem->childItemWithTarget(childFrame->tree().uniqueName())) {
             childFrame->loader().m_requestedHistoryItem = childItem;
-            childFrame->loader().loadDifferentDocumentItem(*childItem, loadType(), MayAttemptCacheOnlyLoadForFormSubmissionItem, ShouldTreatAsContinuingLoad::No);
+            childFrame->loader().loadDifferentDocumentItem(*childItem, nullptr, loadType(), MayAttemptCacheOnlyLoadForFormSubmissionItem, ShouldTreatAsContinuingLoad::No);
             return;
         }
     }
@@ -3686,7 +3686,7 @@ void FrameLoader::loadSameDocumentItem(HistoryItem& item)
 // FIXME: This function should really be split into a couple pieces, some of
 // which should be methods of HistoryController and some of which should be
 // methods of FrameLoader.
-void FrameLoader::loadDifferentDocumentItem(HistoryItem& item, FrameLoadType loadType, FormSubmissionCacheLoadPolicy cacheLoadPolicy, ShouldTreatAsContinuingLoad shouldTreatAsContinuingLoad)
+void FrameLoader::loadDifferentDocumentItem(HistoryItem& item, HistoryItem* fromItem, FrameLoadType loadType, FormSubmissionCacheLoadPolicy cacheLoadPolicy, ShouldTreatAsContinuingLoad shouldTreatAsContinuingLoad)
 {
     RELEASE_LOG_IF_ALLOWED("loadDifferentDocumentItem: frame load started (frame = %p, main = %d)", &m_frame, m_frame.isMainFrame());
 
@@ -3706,6 +3706,7 @@ void FrameLoader::loadDifferentDocumentItem(HistoryItem& item, FrameLoadType loa
 
         auto action = NavigationAction { *m_frame.document(), documentLoader->request(), initiatedByMainFrame, loadType, false };
         action.setTargetBackForwardItem(item);
+        action.setSourceBackForwardItem(fromItem);
         documentLoader->setTriggeringAction(WTFMove(action));
 
         documentLoader->setLastCheckedRequest(ResourceRequest());
@@ -3796,12 +3797,13 @@ void FrameLoader::loadDifferentDocumentItem(HistoryItem& item, FrameLoadType loa
     }
 
     action.setTargetBackForwardItem(item);
+    action.setSourceBackForwardItem(fromItem);
 
     loadWithNavigationAction(request, WTFMove(action), LockHistory::No, loadType, { }, AllowNavigationToInvalidURL::Yes);
 }
 
 // Loads content into this frame, as specified by history item
-void FrameLoader::loadItem(HistoryItem& item, FrameLoadType loadType, ShouldTreatAsContinuingLoad shouldTreatAsContinuingLoad)
+void FrameLoader::loadItem(HistoryItem& item, HistoryItem* fromItem, FrameLoadType loadType, ShouldTreatAsContinuingLoad shouldTreatAsContinuingLoad)
 {
     m_requestedHistoryItem = &item;
     HistoryItem* currentItem = history().currentItem();
@@ -3810,7 +3812,7 @@ void FrameLoader::loadItem(HistoryItem& item, FrameLoadType loadType, ShouldTrea
     if (sameDocumentNavigation)
         loadSameDocumentItem(item);
     else
-        loadDifferentDocumentItem(item, loadType, MayAttemptCacheOnlyLoadForFormSubmissionItem, shouldTreatAsContinuingLoad);
+        loadDifferentDocumentItem(item, fromItem, loadType, MayAttemptCacheOnlyLoadForFormSubmissionItem, shouldTreatAsContinuingLoad);
 }
 
 void FrameLoader::retryAfterFailedCacheOnlyMainResourceLoad()
@@ -3825,7 +3827,7 @@ void FrameLoader::retryAfterFailedCacheOnlyMainResourceLoad()
     HistoryItem& item = *history().provisionalItem();
 
     stopAllLoaders(ShouldNotClearProvisionalItem);
-    loadDifferentDocumentItem(item, loadType, MayNotAttemptCacheOnlyLoadForFormSubmissionItem, ShouldTreatAsContinuingLoad::No);
+    loadDifferentDocumentItem(item, history().currentItem(), loadType, MayNotAttemptCacheOnlyLoadForFormSubmissionItem, ShouldTreatAsContinuingLoad::No);
 }
 
 ResourceError FrameLoader::cancelledError(const ResourceRequest& request) const

Source/WebCore/loader/FrameLoader.h

@@ -133,7 +133,7 @@ class FrameLoader {
     WEBCORE_EXPORT void reloadWithOverrideEncoding(const String& overrideEncoding);
 
     void open(CachedFrameBase&);
-    void loadItem(HistoryItem&, FrameLoadType, ShouldTreatAsContinuingLoad);
+    void loadItem(HistoryItem&, HistoryItem* fromItem, FrameLoadType, ShouldTreatAsContinuingLoad);
     HistoryItem* requestedHistoryItem() const { return m_requestedHistoryItem.get(); }
 
     void retryAfterFailedCacheOnlyMainResourceLoad();
@@ -335,7 +335,7 @@ class FrameLoader {
     void checkCompletenessNow();
 
     void loadSameDocumentItem(HistoryItem&);
-    void loadDifferentDocumentItem(HistoryItem&, FrameLoadType, FormSubmissionCacheLoadPolicy, ShouldTreatAsContinuingLoad);
+    void loadDifferentDocumentItem(HistoryItem&, HistoryItem* fromItem, FrameLoadType, FormSubmissionCacheLoadPolicy, ShouldTreatAsContinuingLoad);
 
     void loadProvisionalItemFromCachedPage();
 

Source/WebCore/loader/HistoryController.cpp

@@ -751,7 +751,7 @@ void HistoryController::recursiveSetProvisionalItem(HistoryItem& item, HistoryIt
 void HistoryController::recursiveGoToItem(HistoryItem& item, HistoryItem* fromItem, FrameLoadType type, ShouldTreatAsContinuingLoad shouldTreatAsContinuingLoad)
 {
     if (!itemsAreClones(item, fromItem)) {
-        m_frame.loader().loadItem(item, type, shouldTreatAsContinuingLoad);
+        m_frame.loader().loadItem(item, fromItem, type, shouldTreatAsContinuingLoad);
         return;
     }
 

Source/WebCore/loader/NavigationAction.cpp

@@ -144,4 +144,9 @@ void NavigationAction::setTargetBackForwardItem(HistoryItem& item)
     m_targetBackForwardItemIdentifier = item.identifier();
 }
 
+void NavigationAction::setSourceBackForwardItem(HistoryItem* item)
+{
+    m_sourceBackForwardItemIdentifier = item ? makeOptional(item->identifier()) : WTF::nullopt;
+}
+
 }

Source/WebCore/loader/NavigationAction.h

@@ -129,6 +129,9 @@ class NavigationAction {
     void setTargetBackForwardItem(HistoryItem&);
     const Optional<BackForwardItemIdentifier>& targetBackForwardItemIdentifier() const { return m_targetBackForwardItemIdentifier; }
 
+    void setSourceBackForwardItem(HistoryItem*);
+    const Optional<BackForwardItemIdentifier>& sourceBackForwardItemIdentifier() const { return m_sourceBackForwardItemIdentifier; }
+
     LockHistory lockHistory() const { return m_lockHistory; }
     void setLockHistory(LockHistory lockHistory) { m_lockHistory = lockHistory; }
 
@@ -154,6 +157,7 @@ class NavigationAction {
     bool m_hasOpenedFrames { false };
     bool m_openedByDOMWithOpener { false };
     Optional<BackForwardItemIdentifier> m_targetBackForwardItemIdentifier;
+    Optional<BackForwardItemIdentifier> m_sourceBackForwardItemIdentifier;
     LockHistory m_lockHistory { LockHistory::No };
     LockBackForwardList m_lockBackForwardList { LockBackForwardList::No };
     Optional<AdClickAttribution> m_adClickAttribution;

Source/WebKit/ChangeLog

@@ -1,3 +1,21 @@
+2019-03-13  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION(PSON, r240660): Navigation over process boundary is flashy when using Cmd-left/right arrow to navigate
+        https://bugs.webkit.org/show_bug.cgi?id=195684
+        <rdar://problem/48294714>
+
+        Reviewed by Antti Koivisto.
+
+        * Shared/NavigationActionData.cpp:
+        (WebKit::NavigationActionData::encode const):
+        (WebKit::NavigationActionData::decode):
+        * Shared/NavigationActionData.h:
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::decidePolicyForNavigationAction):
+        (WebKit::WebPageProxy::backForwardAddItem):
+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
+        (WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
+
 2019-03-13  Chris Dumez  <cdumez@apple.com>
 
         Drop legacy WebCore::toRegistrableDomain() utility function

Source/WebKit/Shared/NavigationActionData.cpp

@@ -50,6 +50,7 @@ void NavigationActionData::encode(IPC::Encoder& encoder) const
     encoder << openedByDOMWithOpener;
     encoder << requesterOrigin;
     encoder << targetBackForwardItemIdentifier;
+    encoder << sourceBackForwardItemIdentifier;
     encoder.encodeEnum(lockHistory);
     encoder.encodeEnum(lockBackForwardList);
     encoder << clientRedirectSourceForHistory;
@@ -127,6 +128,11 @@ Optional<NavigationActionData> NavigationActionData::decode(IPC::Decoder& decode
     if (!targetBackForwardItemIdentifier)
         return WTF::nullopt;
 
+    Optional<Optional<WebCore::BackForwardItemIdentifier>> sourceBackForwardItemIdentifier;
+    decoder >> sourceBackForwardItemIdentifier;
+    if (!sourceBackForwardItemIdentifier)
+        return WTF::nullopt;
+
     WebCore::LockHistory lockHistory;
     if (!decoder.decodeEnum(lockHistory))
         return WTF::nullopt;
@@ -148,7 +154,7 @@ Optional<NavigationActionData> NavigationActionData::decode(IPC::Decoder& decode
     return {{ WTFMove(navigationType), modifiers, WTFMove(mouseButton), WTFMove(syntheticClickType), WTFMove(*userGestureTokenIdentifier),
         WTFMove(*canHandleRequest), WTFMove(shouldOpenExternalURLsPolicy), WTFMove(*downloadAttribute), WTFMove(clickLocationInRootViewCoordinates),
         WTFMove(*isRedirect), *treatAsSameOriginNavigation, *hasOpenedFrames, *openedByDOMWithOpener, WTFMove(*requesterOrigin),
-        WTFMove(*targetBackForwardItemIdentifier), lockHistory, lockBackForwardList, WTFMove(*clientRedirectSourceForHistory), WTFMove(*adClickAttribution) }};
+        WTFMove(*targetBackForwardItemIdentifier), WTFMove(*sourceBackForwardItemIdentifier), lockHistory, lockBackForwardList, WTFMove(*clientRedirectSourceForHistory), WTFMove(*adClickAttribution) }};
 }
 
 } // namespace WebKit

Source/WebKit/Shared/NavigationActionData.h

@@ -58,6 +58,7 @@ struct NavigationActionData {
     bool openedByDOMWithOpener { false };
     WebCore::SecurityOriginData requesterOrigin;
     Optional<WebCore::BackForwardItemIdentifier> targetBackForwardItemIdentifier;
+    Optional<WebCore::BackForwardItemIdentifier> sourceBackForwardItemIdentifier;
     WebCore::LockHistory lockHistory;
     WebCore::LockBackForwardList lockBackForwardList;
     WTF::String clientRedirectSourceForHistory;

Source/WebKit/UIProcess/WebPageProxy.cpp

@@ -4515,8 +4515,13 @@ void WebPageProxy::decidePolicyForNavigationAction(Ref<WebProcessProxy>&& proces
 
     if (!navigation) {
         if (auto targetBackForwardItemIdentifier = navigationActionData.targetBackForwardItemIdentifier) {
-            if (auto* item = m_backForwardList->itemForID(*targetBackForwardItemIdentifier))
-                navigation = m_navigationState->createBackForwardNavigation(*item, m_backForwardList->currentItem(), FrameLoadType::IndexedBackForward);
+            if (auto* item = m_backForwardList->itemForID(*targetBackForwardItemIdentifier)) {
+                auto* fromItem = navigationActionData.sourceBackForwardItemIdentifier ? m_backForwardList->itemForID(*navigationActionData.sourceBackForwardItemIdentifier) : nullptr;
+                if (!fromItem)
+                    fromItem = m_backForwardList->currentItem();
+                WTFLogAlways("WebPageProxy::decidePolicyForNavigationAction() creates back/forward navigation from item %s", fromItem ? fromItem->url().utf8().data() : "null");
+                navigation = m_navigationState->createBackForwardNavigation(*item, fromItem, FrameLoadType::IndexedBackForward);
+            }
         }
         if (!navigation)
             navigation = m_navigationState->createLoadRequestNavigation(ResourceRequest(request), m_backForwardList->currentItem());
@@ -5579,7 +5584,8 @@ void WebPageProxy::requestDOMPasteAccess(const WebCore::IntRect& elementRect, co
 
 void WebPageProxy::backForwardAddItem(BackForwardListItemState&& itemState)
 {
-    m_backForwardList->addItem(WebBackForwardListItem::create(WTFMove(itemState), pageID()));
+    auto item = WebBackForwardListItem::create(WTFMove(itemState), pageID());
+    m_backForwardList->addItem(WTFMove(item));
 }
 
 void WebPageProxy::backForwardGoToItem(const BackForwardItemIdentifier& itemID, SandboxExtension::Handle& sandboxExtensionHandle)

Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp

@@ -880,6 +880,7 @@ void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const Navigat
     if (auto& requester = navigationAction.requester())
         navigationActionData.requesterOrigin = requester->securityOrigin().data();
     navigationActionData.targetBackForwardItemIdentifier = navigationAction.targetBackForwardItemIdentifier();
+    navigationActionData.sourceBackForwardItemIdentifier = navigationAction.sourceBackForwardItemIdentifier();
     navigationActionData.lockHistory = navigationAction.lockHistory();
     navigationActionData.lockBackForwardList = navigationAction.lockBackForwardList();
     navigationActionData.adClickAttribution = navigationAction.adClickAttribution();

Tools/ChangeLog

@@ -1,3 +1,15 @@
+2019-03-13  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION(PSON, r240660): Navigation over process boundary is flashy when using Cmd-left/right arrow to navigate
+        https://bugs.webkit.org/show_bug.cgi?id=195684
+        <rdar://problem/48294714>
+
+        Reviewed by Antti Koivisto.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
 2019-03-13  Aakash Jain  <aakash_jain@apple.com>
 
         [ews-app] Remove unused patch view

Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

@@ -3137,6 +3137,75 @@ static unsigned waitUntilClientWidthIs(WKWebView *webView, unsigned expectedClie
     EXPECT_EQ(2u, seenPIDs.size());
 }
 
+TEST(ProcessSwap, PageCacheWhenNavigatingFromJS)
+{
+    auto processPoolConfiguration = psonProcessPoolConfiguration();
+    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:pageCache1Bytes];
+    [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 pidAfterLoad1 = [webView _webProcessIdentifier];
+
+    EXPECT_EQ(1u, [processPool _webProcessCountIgnoringPrewarmedAndCached]);
+
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main.html"]];
+
+    [webView loadRequest:request];
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto pidAfterLoad2 = [webView _webProcessIdentifier];
+
+    EXPECT_EQ(2u, [processPool _webProcessCountIgnoringPrewarmedAndCached]);
+    EXPECT_NE(pidAfterLoad1, pidAfterLoad2);
+
+    [webView evaluateJavaScript:@"history.back()" completionHandler: nil];
+    TestWebKitAPI::Util::run(&receivedMessage);
+    receivedMessage = false;
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto pidAfterLoad3 = [webView _webProcessIdentifier];
+
+    EXPECT_EQ(2u, [processPool _webProcessCountIgnoringPrewarmedAndCached]);
+    EXPECT_EQ(pidAfterLoad1, pidAfterLoad3);
+    EXPECT_EQ(1u, [receivedMessages count]);
+    EXPECT_TRUE([receivedMessages.get()[0] isEqualToString:@"Was persisted" ]);
+    EXPECT_EQ(2u, seenPIDs.size());
+
+    [webView evaluateJavaScript:@"history.forward()" completionHandler: nil];
+    TestWebKitAPI::Util::run(&receivedMessage);
+    receivedMessage = false;
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto pidAfterLoad4 = [webView _webProcessIdentifier];
+
+    EXPECT_EQ(2u, [processPool _webProcessCountIgnoringPrewarmedAndCached]);
+    EXPECT_EQ(pidAfterLoad2, pidAfterLoad4);
+    EXPECT_EQ(2u, [receivedMessages count]);
+    EXPECT_TRUE([receivedMessages.get()[1] isEqualToString:@"Was persisted" ]);
+    EXPECT_EQ(2u, seenPIDs.size());
+}
+
 TEST(ProcessSwap, NumberOfPrewarmedProcesses)
 {
     auto processPoolConfiguration = psonProcessPoolConfiguration();