⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 242903 in webkit


Ignore:
Timestamp:
Mar 13, 2019, 1:32:48 PM (7 years ago)
Author:
Chris Dumez
Message:

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:
Location:
trunk
Files:
13 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r242901 r242903  
     12019-03-13  Chris Dumez  <cdumez@apple.com>
     2
     3        REGRESSION(PSON, r240660): Navigation over process boundary is flashy when using Cmd-left/right arrow to navigate
     4        https://bugs.webkit.org/show_bug.cgi?id=195684
     5        <rdar://problem/48294714>
     6
     7        Reviewed by Antti Koivisto.
     8
     9        The issue was caused by us failing to suspend the current page on navigation because the source and
     10        target WebBackForwardListItem are identical. The source WebBackForwardListItem was wrong.
     11
     12        When a navigation is triggered by the WebContent process (and not the UIProcess), we create the Navigation
     13        object in WebPageProxy::decidePolicyForNavigationAction(). For the navigation's targetItem, we use the
     14        target item identifier provided by the WebContent process via the NavigationActionData. However,
     15        for the source item, we would use the WebBackForwardList's currentItem in the UIProcess. The issue
     16        is that the WebBackForwardList's currentItem usually has already been updated to be the target
     17        item via a WebPageProxy::BackForwardGoToItem() synchronous IPC.
     18
     19        To avoid raciness and given that the current history management is fragile (as it is managed by
     20        both the UIProcess and the WebProcess), I am now passing the source item identifier in
     21        addition to the target item identifier in the NavigationActionData that is sent by the WebProcess.
     22        This is a lot less error prone, the WebProcess knows more accurately which history items it is going
     23        from and to.
     24
     25        * loader/FrameLoader.cpp:
     26        (WebCore::FrameLoader::loadURLIntoChildFrame):
     27        (WebCore::FrameLoader::loadDifferentDocumentItem):
     28        (WebCore::FrameLoader::loadItem):
     29        (WebCore::FrameLoader::retryAfterFailedCacheOnlyMainResourceLoad):
     30        * loader/FrameLoader.h:
     31        * loader/HistoryController.cpp:
     32        (WebCore::HistoryController::recursiveGoToItem):
     33        * loader/NavigationAction.cpp:
     34        (WebCore::NavigationAction::setSourceBackForwardItem):
     35        * loader/NavigationAction.h:
     36        (WebCore::NavigationAction::sourceBackForwardItemIdentifier const):
     37
    1382019-03-13  Jer Noble  <jer.noble@apple.com>
    239

  • trunk/Source/WebCore/loader/FrameLoader.cpp

    r242899 r242903  
    975975        if (auto* childItem = parentItem->childItemWithTarget(childFrame->tree().uniqueName())) {
    976976            childFrame->loader().m_requestedHistoryItem = childItem;
    977             childFrame->loader().loadDifferentDocumentItem(*childItem, loadType(), MayAttemptCacheOnlyLoadForFormSubmissionItem, ShouldTreatAsContinuingLoad::No);
     977            childFrame->loader().loadDifferentDocumentItem(*childItem, nullptr, loadType(), MayAttemptCacheOnlyLoadForFormSubmissionItem, ShouldTreatAsContinuingLoad::No);
    978978            return;
    979979        }
    … …  
    36873687// which should be methods of HistoryController and some of which should be
    36883688// methods of FrameLoader.
    3689 void FrameLoader::loadDifferentDocumentItem(HistoryItem& item, FrameLoadType loadType, FormSubmissionCacheLoadPolicy cacheLoadPolicy, ShouldTreatAsContinuingLoad shouldTreatAsContinuingLoad)
     3689void FrameLoader::loadDifferentDocumentItem(HistoryItem& item, HistoryItem* fromItem, FrameLoadType loadType, FormSubmissionCacheLoadPolicy cacheLoadPolicy, ShouldTreatAsContinuingLoad shouldTreatAsContinuingLoad)
    36903690{
    36913691    RELEASE_LOG_IF_ALLOWED("loadDifferentDocumentItem: frame load started (frame = %p, main = %d)", &m_frame, m_frame.isMainFrame());
    … …  
    37073707        auto action = NavigationAction { *m_frame.document(), documentLoader->request(), initiatedByMainFrame, loadType, false };
    37083708        action.setTargetBackForwardItem(item);
     3709        action.setSourceBackForwardItem(fromItem);
    37093710        documentLoader->setTriggeringAction(WTFMove(action));
    37103711
    … …  
    37973798
    37983799    action.setTargetBackForwardItem(item);
     3800    action.setSourceBackForwardItem(fromItem);
    37993801
    38003802    loadWithNavigationAction(request, WTFMove(action), LockHistory::No, loadType, { }, AllowNavigationToInvalidURL::Yes);
    … …  
    38023804
    38033805// Loads content into this frame, as specified by history item
    3804 void FrameLoader::loadItem(HistoryItem& item, FrameLoadType loadType, ShouldTreatAsContinuingLoad shouldTreatAsContinuingLoad)
     3806void FrameLoader::loadItem(HistoryItem& item, HistoryItem* fromItem, FrameLoadType loadType, ShouldTreatAsContinuingLoad shouldTreatAsContinuingLoad)
    38053807{
    38063808    m_requestedHistoryItem = &item;
    … …  
    38113813        loadSameDocumentItem(item);
    38123814    else
    3813         loadDifferentDocumentItem(item, loadType, MayAttemptCacheOnlyLoadForFormSubmissionItem, shouldTreatAsContinuingLoad);
     3815        loadDifferentDocumentItem(item, fromItem, loadType, MayAttemptCacheOnlyLoadForFormSubmissionItem, shouldTreatAsContinuingLoad);
    38143816}
    38153817
    … …  
    38263828
    38273829    stopAllLoaders(ShouldNotClearProvisionalItem);
    3828     loadDifferentDocumentItem(item, loadType, MayNotAttemptCacheOnlyLoadForFormSubmissionItem, ShouldTreatAsContinuingLoad::No);
     3830    loadDifferentDocumentItem(item, history().currentItem(), loadType, MayNotAttemptCacheOnlyLoadForFormSubmissionItem, ShouldTreatAsContinuingLoad::No);
    38293831}
    38303832

  • trunk/Source/WebCore/loader/FrameLoader.h

    r241480 r242903  
    134134
    135135    void open(CachedFrameBase&);
    136     void loadItem(HistoryItem&, FrameLoadType, ShouldTreatAsContinuingLoad);
     136    void loadItem(HistoryItem&, HistoryItem* fromItem, FrameLoadType, ShouldTreatAsContinuingLoad);
    137137    HistoryItem* requestedHistoryItem() const { return m_requestedHistoryItem.get(); }
    138138
    … …  
    336336
    337337    void loadSameDocumentItem(HistoryItem&);
    338     void loadDifferentDocumentItem(HistoryItem&, FrameLoadType, FormSubmissionCacheLoadPolicy, ShouldTreatAsContinuingLoad);
     338    void loadDifferentDocumentItem(HistoryItem&, HistoryItem* fromItem, FrameLoadType, FormSubmissionCacheLoadPolicy, ShouldTreatAsContinuingLoad);
    339339
    340340    void loadProvisionalItemFromCachedPage();

  • trunk/Source/WebCore/loader/HistoryController.cpp

    r239720 r242903  
    752752{
    753753    if (!itemsAreClones(item, fromItem)) {
    754         m_frame.loader().loadItem(item, type, shouldTreatAsContinuingLoad);
     754        m_frame.loader().loadItem(item, fromItem, type, shouldTreatAsContinuingLoad);
    755755        return;
    756756    }

  • trunk/Source/WebCore/loader/NavigationAction.cpp

    r239461 r242903  
    145145}
    146146
     147void NavigationAction::setSourceBackForwardItem(HistoryItem* item)
     148{
     149    m_sourceBackForwardItemIdentifier = item ? makeOptional(item->identifier()) : WTF::nullopt;
    147150}
     151
     152}

  • trunk/Source/WebCore/loader/NavigationAction.h

    r241451 r242903  
    130130    const Optional<BackForwardItemIdentifier>& targetBackForwardItemIdentifier() const { return m_targetBackForwardItemIdentifier; }
    131131
     132    void setSourceBackForwardItem(HistoryItem*);
     133    const Optional<BackForwardItemIdentifier>& sourceBackForwardItemIdentifier() const { return m_sourceBackForwardItemIdentifier; }
     134
    132135    LockHistory lockHistory() const { return m_lockHistory; }
    133136    void setLockHistory(LockHistory lockHistory) { m_lockHistory = lockHistory; }
    … …  
    155158    bool m_openedByDOMWithOpener { false };
    156159    Optional<BackForwardItemIdentifier> m_targetBackForwardItemIdentifier;
     160    Optional<BackForwardItemIdentifier> m_sourceBackForwardItemIdentifier;
    157161    LockHistory m_lockHistory { LockHistory::No };
    158162    LockBackForwardList m_lockBackForwardList { LockBackForwardList::No };

  • trunk/Source/WebKit/ChangeLog

    r242899 r242903  
     12019-03-13  Chris Dumez  <cdumez@apple.com>
     2
     3        REGRESSION(PSON, r240660): Navigation over process boundary is flashy when using Cmd-left/right arrow to navigate
     4        https://bugs.webkit.org/show_bug.cgi?id=195684
     5        <rdar://problem/48294714>
     6
     7        Reviewed by Antti Koivisto.
     8
     9        * Shared/NavigationActionData.cpp:
     10        (WebKit::NavigationActionData::encode const):
     11        (WebKit::NavigationActionData::decode):
     12        * Shared/NavigationActionData.h:
     13        * UIProcess/WebPageProxy.cpp:
     14        (WebKit::WebPageProxy::decidePolicyForNavigationAction):
     15        (WebKit::WebPageProxy::backForwardAddItem):
     16        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
     17        (WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
     18
    1192019-03-13  Chris Dumez  <cdumez@apple.com>
    220

  • trunk/Source/WebKit/Shared/NavigationActionData.cpp

    r241049 r242903  
    5151    encoder << requesterOrigin;
    5252    encoder << targetBackForwardItemIdentifier;
     53    encoder << sourceBackForwardItemIdentifier;
    5354    encoder.encodeEnum(lockHistory);
    5455    encoder.encodeEnum(lockBackForwardList);
    … …  
    128129        return WTF::nullopt;
    129130
     131    Optional<Optional<WebCore::BackForwardItemIdentifier>> sourceBackForwardItemIdentifier;
     132    decoder >> sourceBackForwardItemIdentifier;
     133    if (!sourceBackForwardItemIdentifier)
     134        return WTF::nullopt;
     135
    130136    WebCore::LockHistory lockHistory;
    131137    if (!decoder.decodeEnum(lockHistory))
    … …  
    149155        WTFMove(*canHandleRequest), WTFMove(shouldOpenExternalURLsPolicy), WTFMove(*downloadAttribute), WTFMove(clickLocationInRootViewCoordinates),
    150156        WTFMove(*isRedirect), *treatAsSameOriginNavigation, *hasOpenedFrames, *openedByDOMWithOpener, WTFMove(*requesterOrigin),
    151         WTFMove(*targetBackForwardItemIdentifier), lockHistory, lockBackForwardList, WTFMove(*clientRedirectSourceForHistory), WTFMove(*adClickAttribution) }};
     157        WTFMove(*targetBackForwardItemIdentifier), WTFMove(*sourceBackForwardItemIdentifier), lockHistory, lockBackForwardList, WTFMove(*clientRedirectSourceForHistory), WTFMove(*adClickAttribution) }};
    152158}
    153159

  • trunk/Source/WebKit/Shared/NavigationActionData.h

    r241049 r242903  
    5959    WebCore::SecurityOriginData requesterOrigin;
    6060    Optional<WebCore::BackForwardItemIdentifier> targetBackForwardItemIdentifier;
     61    Optional<WebCore::BackForwardItemIdentifier> sourceBackForwardItemIdentifier;
    6162    WebCore::LockHistory lockHistory;
    6263    WebCore::LockBackForwardList lockBackForwardList;

  • trunk/Source/WebKit/UIProcess/WebPageProxy.cpp

    r242840 r242903  
    45164516    if (!navigation) {
    45174517        if (auto targetBackForwardItemIdentifier = navigationActionData.targetBackForwardItemIdentifier) {
    4518             if (auto* item = m_backForwardList->itemForID(*targetBackForwardItemIdentifier))
    4519                 navigation = m_navigationState->createBackForwardNavigation(*item, m_backForwardList->currentItem(), FrameLoadType::IndexedBackForward);
     4518            if (auto* item = m_backForwardList->itemForID(*targetBackForwardItemIdentifier)) {
     4519                auto* fromItem = navigationActionData.sourceBackForwardItemIdentifier ? m_backForwardList->itemForID(*navigationActionData.sourceBackForwardItemIdentifier) : nullptr;
     4520                if (!fromItem)
     4521                    fromItem = m_backForwardList->currentItem();
     4522                WTFLogAlways("WebPageProxy::decidePolicyForNavigationAction() creates back/forward navigation from item %s", fromItem ? fromItem->url().utf8().data() : "null");
     4523                navigation = m_navigationState->createBackForwardNavigation(*item, fromItem, FrameLoadType::IndexedBackForward);
     4524            }
    45204525        }
    45214526        if (!navigation)
    … …  
    55805585void WebPageProxy::backForwardAddItem(BackForwardListItemState&& itemState)
    55815586{
    5582     m_backForwardList->addItem(WebBackForwardListItem::create(WTFMove(itemState), pageID()));
     5587    auto item = WebBackForwardListItem::create(WTFMove(itemState), pageID());
     5588    m_backForwardList->addItem(WTFMove(item));
    55835589}
    55845590

  • trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp

    r242899 r242903  
    881881        navigationActionData.requesterOrigin = requester->securityOrigin().data();
    882882    navigationActionData.targetBackForwardItemIdentifier = navigationAction.targetBackForwardItemIdentifier();
     883    navigationActionData.sourceBackForwardItemIdentifier = navigationAction.sourceBackForwardItemIdentifier();
    883884    navigationActionData.lockHistory = navigationAction.lockHistory();
    884885    navigationActionData.lockBackForwardList = navigationAction.lockBackForwardList();

  • trunk/Tools/ChangeLog

    r242885 r242903  
     12019-03-13  Chris Dumez  <cdumez@apple.com>
     2
     3        REGRESSION(PSON, r240660): Navigation over process boundary is flashy when using Cmd-left/right arrow to navigate
     4        https://bugs.webkit.org/show_bug.cgi?id=195684
     5        <rdar://problem/48294714>
     6
     7        Reviewed by Antti Koivisto.
     8
     9        Add API test coverage.
     10
     11        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
     12
    1132019-03-13  Aakash Jain  <aakash_jain@apple.com>
    214

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

    r242755 r242903  
    31383138}
    31393139
     3140TEST(ProcessSwap, PageCacheWhenNavigatingFromJS)
     3141{
     3142    auto processPoolConfiguration = psonProcessPoolConfiguration();
     3143    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
     3144
     3145    auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
     3146    [webViewConfiguration setProcessPool:processPool.get()];
     3147    auto handler = adoptNS([[PSONScheme alloc] init]);
     3148    [handler addMappingFromURLString:@"pson://www.webkit.org/main.html" toData:pageCache1Bytes];
     3149    [handler addMappingFromURLString:@"pson://www.apple.com/main.html" toData:pageCache1Bytes];
     3150    [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
     3151
     3152    auto messageHandler = adoptNS([[PSONMessageHandler alloc] init]);
     3153    [[webViewConfiguration userContentController] addScriptMessageHandler:messageHandler.get() name:@"pson"];
     3154
     3155    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
     3156    auto delegate = adoptNS([[PSONNavigationDelegate alloc] init]);
     3157    [webView setNavigationDelegate:delegate.get()];
     3158
     3159    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
     3160
     3161    [webView loadRequest:request];
     3162    TestWebKitAPI::Util::run(&done);
     3163    done = false;
     3164
     3165    auto pidAfterLoad1 = [webView _webProcessIdentifier];
     3166
     3167    EXPECT_EQ(1u, [processPool _webProcessCountIgnoringPrewarmedAndCached]);
     3168
     3169    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main.html"]];
     3170
     3171    [webView loadRequest:request];
     3172    TestWebKitAPI::Util::run(&done);
     3173    done = false;
     3174
     3175    auto pidAfterLoad2 = [webView _webProcessIdentifier];
     3176
     3177    EXPECT_EQ(2u, [processPool _webProcessCountIgnoringPrewarmedAndCached]);
     3178    EXPECT_NE(pidAfterLoad1, pidAfterLoad2);
     3179
     3180    [webView evaluateJavaScript:@"history.back()" completionHandler: nil];
     3181    TestWebKitAPI::Util::run(&receivedMessage);
     3182    receivedMessage = false;
     3183    TestWebKitAPI::Util::run(&done);
     3184    done = false;
     3185
     3186    auto pidAfterLoad3 = [webView _webProcessIdentifier];
     3187
     3188    EXPECT_EQ(2u, [processPool _webProcessCountIgnoringPrewarmedAndCached]);
     3189    EXPECT_EQ(pidAfterLoad1, pidAfterLoad3);
     3190    EXPECT_EQ(1u, [receivedMessages count]);
     3191    EXPECT_TRUE([receivedMessages.get()[0] isEqualToString:@"Was persisted" ]);
     3192    EXPECT_EQ(2u, seenPIDs.size());
     3193
     3194    [webView evaluateJavaScript:@"history.forward()" completionHandler: nil];
     3195    TestWebKitAPI::Util::run(&receivedMessage);
     3196    receivedMessage = false;
     3197    TestWebKitAPI::Util::run(&done);
     3198    done = false;
     3199
     3200    auto pidAfterLoad4 = [webView _webProcessIdentifier];
     3201
     3202    EXPECT_EQ(2u, [processPool _webProcessCountIgnoringPrewarmedAndCached]);
     3203    EXPECT_EQ(pidAfterLoad2, pidAfterLoad4);
     3204    EXPECT_EQ(2u, [receivedMessages count]);
     3205    EXPECT_TRUE([receivedMessages.get()[1] isEqualToString:@"Was persisted" ]);
     3206    EXPECT_EQ(2u, seenPIDs.size());
     3207}
     3208
    31403209TEST(ProcessSwap, NumberOfPrewarmedProcesses)
    31413210{
Note: See TracChangeset for help on using the changeset viewer.