Changeset 240161 in webkit


Ignore:
Timestamp:
Jan 18, 2019 11:47:33 AM (5 years ago)
Author:
Chris Dumez
Message:

Regression(PSON) Scroll position is not always restored properly when navigating back
https://bugs.webkit.org/show_bug.cgi?id=193578
<rdar://problem/47386331>

Reviewed by Tim Horton.

Source/WebKit:

Fix issues causing the scroll position to not be restored at all (or incorrectly) when
navigating back cross-site with PSON enabled. Also make sure that the swipe gesture
snapshot really stays up until we've restored the scroll position.

Note that even after those changes, I can still sometimes reproduce a white flash when
swiping back to Google search results (scroll position being correct now). This is
tracked by <rdar://problem/47071684> and happens even if I disable PSON entirely.

  • Shared/SessionState.cpp:

(WebKit::FrameState::encode const):
(WebKit::FrameState::decode):

  • Shared/SessionState.h:
  • WebProcess/WebCoreSupport/SessionStateConversion.cpp:

(WebKit::toFrameState):
(WebKit::applyFrameState):
obscuredInsets is present on the HistoryItem in the WebProcess but was never passed to
or stored by the UIProcess on the WebBackForwardListItem. obscuredInsets is needed to
properly restore the scrollPosition (position was 70px off on my iPad without this).
With PSON enabled, if you swipe back cross-process and the previous page was not put
into PageCache, then the HistoryItem is gone on the WebProcess side. What happens is
that the UIProcess sends its WebBackForwardListItem to the WebProcess, which restores
the HistoryItem there, and then asks it to load it. The obscuredInsets was getting lost
in the process since the UIProcess never knew about it.

  • UIProcess/Cocoa/ViewGestureController.cpp:

(WebKit::ViewGestureController::didReachMainFrameLoadTerminalState):
Drop logic that was causing the ViewGestureController to not wait for the scroll position
to be restored before taking down the snapshot, when UI-side compositing is enabled.
If you look at the comment above the code, you'll see that the code in question was meant
to impact only the non-UI side compositing code path. As a matter of fact, when the code
was reviewed at https://bugs.webkit.org/show_bug.cgi?id=151224, it was protected by a
#if PLATFORM(MAC), before getting modified the wrong way before landing. In practice, we
would have often restored the scroll position by the time the load is finished so it would
not cause a flash in most cases. However, with PSON enabled and the layer tree freezing we
do on process-swap, the first post-scroll restoration layer tree commit may now occur a
little bit later and we would lose the race more often.

  • UIProcess/WebProcessProxy.cpp:

(WebKit::WebProcessProxy::updateBackForwardItem):

  • UIProcess/WebProcessProxy.h:

When adding PageCache support to PSON, we used to navigate the "suspended" page to
about:blank. This would lead to unwanted WebProcessProxy::updateBackForwardItem()
calls from the WebProcess which we wanted to ignore. We thus added logic to ignore
updateBackForwardItem() IPC from the old WebProcess after a swap. The issue with this
is that we sometimes miss/ignore legit updates to the HistoryItem from the old process,
in particular with regards to the scroll position and the pageScaleFactor. So if you
swiped and then quickly enough did a cross-site navigation, the UIProcess'
WebBackForwardList would not get updated with the latest scroll position and we would
thus fail to restore it later on. To address the issue, we now stop ignoring updates
from the old WebProcess after a swap. This logic is no longer needed since we no longer
navigate the old page to about:blank after a swap, we merely suspend it "in place".

Tools:

Add API test coverage.

  • TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
Location:
trunk
Files:
9 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebKit/ChangeLog

    r240159 r240161  
     12019-01-18  Chris Dumez  <cdumez@apple.com>
     2
     3        Regression(PSON) Scroll position is not always restored properly when navigating back
     4        https://bugs.webkit.org/show_bug.cgi?id=193578
     5        <rdar://problem/47386331>
     6
     7        Reviewed by Tim Horton.
     8
     9        Fix issues causing the scroll position to not be restored at all (or incorrectly) when
     10        navigating back cross-site with PSON enabled. Also make sure that the swipe gesture
     11        snapshot really stays up until we've restored the scroll position.
     12
     13        Note that even after those changes, I can still sometimes reproduce a white flash when
     14        swiping back to Google search results (scroll position being correct now). This is
     15        tracked by <rdar://problem/47071684> and happens even if I disable PSON entirely.
     16
     17        * Shared/SessionState.cpp:
     18        (WebKit::FrameState::encode const):
     19        (WebKit::FrameState::decode):
     20        * Shared/SessionState.h:
     21        * WebProcess/WebCoreSupport/SessionStateConversion.cpp:
     22        (WebKit::toFrameState):
     23        (WebKit::applyFrameState):
     24        obscuredInsets is present on the HistoryItem in the WebProcess but was never passed to
     25        or stored by the UIProcess on the WebBackForwardListItem. obscuredInsets is needed to
     26        properly restore the scrollPosition (position was 70px off on my iPad without this).
     27        With PSON enabled, if you swipe back cross-process and the previous page was not put
     28        into PageCache, then the HistoryItem is gone on the WebProcess side. What happens is
     29        that the UIProcess sends its WebBackForwardListItem to the WebProcess, which restores
     30        the HistoryItem there, and then asks it to load it. The obscuredInsets was getting lost
     31        in the process since the UIProcess never knew about it.
     32
     33        * UIProcess/Cocoa/ViewGestureController.cpp:
     34        (WebKit::ViewGestureController::didReachMainFrameLoadTerminalState):
     35        Drop logic that was causing the ViewGestureController to not wait for the scroll position
     36        to be restored before taking down the snapshot, when UI-side compositing is enabled.
     37        If you look at the comment above the code, you'll see that the code in question was meant
     38        to impact only the non-UI side compositing code path. As a matter of fact, when the code
     39        was reviewed at https://bugs.webkit.org/show_bug.cgi?id=151224, it was protected by a
     40        #if PLATFORM(MAC), before getting modified the wrong way before landing. In practice, we
     41        would have often restored the scroll position by the time the load is finished so it would
     42        not cause a flash in most cases. However, with PSON enabled and the layer tree freezing we
     43        do on process-swap, the first post-scroll restoration layer tree commit may now occur a
     44        little bit later and we would lose the race more often.
     45
     46        * UIProcess/WebProcessProxy.cpp:
     47        (WebKit::WebProcessProxy::updateBackForwardItem):
     48        * UIProcess/WebProcessProxy.h:
     49        When adding PageCache support to PSON, we used to navigate the "suspended" page to
     50        about:blank. This would lead to unwanted WebProcessProxy::updateBackForwardItem()
     51        calls from the WebProcess which we wanted to ignore. We thus added logic to ignore
     52        updateBackForwardItem() IPC from the old WebProcess after a swap. The issue with this
     53        is that we sometimes miss/ignore legit updates to the HistoryItem from the old process,
     54        in particular with regards to the scroll position and the pageScaleFactor. So if you
     55        swiped and then quickly enough did a cross-site navigation, the UIProcess'
     56        WebBackForwardList would not get updated with the latest scroll position and we would
     57        thus fail to restore it later on. To address the issue, we now stop ignoring updates
     58        from the old WebProcess after a swap. This logic is no longer needed since we no longer
     59        navigate the old page to about:blank after a swap, we merely suspend it "in place".
     60
    1612019-01-18  Wenson Hsieh  <wenson_hsieh@apple.com>
    262
  • trunk/Source/WebKit/Shared/SessionState.cpp

    r239427 r240161  
    129129    encoder << contentSize;
    130130    encoder << scaleIsInitial;
     131    encoder << obscuredInsets;
    131132#endif
    132133
     
    176177        return WTF::nullopt;
    177178    if (!decoder.decode(result.scaleIsInitial))
     179        return WTF::nullopt;
     180    if (!decoder.decode(result.obscuredInsets))
    178181        return WTF::nullopt;
    179182#endif
  • trunk/Source/WebKit/Shared/SessionState.h

    r239427 r240161  
    111111    WebCore::IntSize contentSize;
    112112    bool scaleIsInitial { false };
     113    WebCore::FloatBoxExtent obscuredInsets;
    113114#endif
    114115
  • trunk/Source/WebKit/UIProcess/Cocoa/ViewGestureController.cpp

    r238818 r240161  
    194194    m_snapshotRemovalTracker.cancelOutstandingEvent(SnapshotRemovalTracker::VisuallyNonEmptyLayout);
    195195
    196     // With Web-process scrolling, we check if the scroll position restoration succeeded by comparing the
    197     // requested and actual scroll position. It's possible that we will never succeed in restoring
    198     // the exact scroll position we wanted, in the case of a dynamic page, but we know that by
    199     // main frame load time that we've gotten as close as we're going to get, so stop waiting.
    200     // We don't want to do this with UI-side scrolling because scroll position restoration is baked into the transaction.
    201     // FIXME: It seems fairly dirty to type-check the DrawingArea like this.
    202     if (auto drawingArea = m_webPageProxy.drawingArea()) {
    203         if (is<RemoteLayerTreeDrawingAreaProxy>(drawingArea))
    204             m_snapshotRemovalTracker.cancelOutstandingEvent(SnapshotRemovalTracker::ScrollPositionRestoration);
    205     }
    206 
    207196    checkForActiveLoads();
    208197}
  • trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp

    r240046 r240161  
    586586#endif
    587587
    588 bool WebProcessProxy::hasProvisionalPageWithID(uint64_t pageID) const
    589 {
    590     for (auto* provisionalPage : m_provisionalPages) {
    591         if (provisionalPage->page().pageID() == pageID)
    592             return true;
    593     }
    594     return false;
    595 }
    596 
    597588void WebProcessProxy::updateBackForwardItem(const BackForwardListItemState& itemState)
    598589{
    599     if (auto* item = WebBackForwardListItem::itemForID(itemState.identifier)) {
    600         // This update could be coming from a web process that is not the active process for
    601         // the back/forward items page.
    602         // e.g. The old web process is navigating to about:blank for suspension.
    603         // We ignore these updates.
    604         if (m_pageMap.contains(item->pageID()) || hasProvisionalPageWithID(item->pageID()))
    605             item->setPageState(itemState.pageState);
    606     }
     590    if (auto* item = WebBackForwardListItem::itemForID(itemState.identifier))
     591        item->setPageState(itemState.pageState);
    607592}
    608593
  • trunk/Source/WebKit/UIProcess/WebProcessProxy.h

    r240046 r240161  
    335335    void logDiagnosticMessageForResourceLimitTermination(const String& limitKey);
    336336
    337     bool hasProvisionalPageWithID(uint64_t pageID) const;
    338 
    339337    enum class IsWeak { No, Yes };
    340338    template<typename T> class WeakOrStrongPtr {
  • trunk/Source/WebKit/WebProcess/WebCoreSupport/SessionStateConversion.cpp

    r239461 r240161  
    9797    frameState.contentSize = historyItem.contentSize();
    9898    frameState.scaleIsInitial = historyItem.scaleIsInitial();
     99    frameState.obscuredInsets = historyItem.obscuredInsets();
    99100#endif
    100101
     
    174175    historyItem.setContentSize(frameState.contentSize);
    175176    historyItem.setScaleIsInitial(frameState.scaleIsInitial);
     177    historyItem.setObscuredInsets(frameState.obscuredInsets);
    176178#endif
    177179
  • trunk/Tools/ChangeLog

    r240156 r240161  
     12019-01-18  Chris Dumez  <cdumez@apple.com>
     2
     3        Regression(PSON) Scroll position is not always restored properly when navigating back
     4        https://bugs.webkit.org/show_bug.cgi?id=193578
     5        <rdar://problem/47386331>
     6
     7        Reviewed by Tim Horton.
     8
     9        Add API test coverage.
     10
     11        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
     12
    1132019-01-18  Youenn Fablet  <youenn@apple.com>
    214
  • trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

    r240046 r240161  
    38473847}
    38483848
     3849static const char* tallPageBytes = R"PSONRESOURCE(
     3850<!DOCTYPE html>
     3851<html>
     3852<head>
     3853<meta name='viewport' content='width=device-width, initial-scale=1'>
     3854<style>
     3855body {
     3856    margin: 0;
     3857    width: 100%;
     3858    height: 10000px;
     3859}
     3860</style>
     3861</head>
     3862<body>
     3863<a id="testLink" href="pson://www.apple.com/main.html">Test</a>
     3864</body>
     3865</html>
     3866)PSONRESOURCE";
     3867
     3868TEST(ProcessSwap, ScrollPositionRestoration)
     3869{
     3870    auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
     3871    [processPoolConfiguration setProcessSwapsOnNavigation:YES];
     3872    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
     3873
     3874    auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
     3875    [webViewConfiguration setProcessPool:processPool.get()];
     3876    auto handler = adoptNS([[PSONScheme alloc] init]);
     3877    [handler addMappingFromURLString:@"pson://www.webkit.org/main.html" toData:tallPageBytes];
     3878    [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
     3879
     3880    [[webViewConfiguration preferences] _setUsesPageCache:NO];
     3881
     3882    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
     3883    auto delegate = adoptNS([[PSONNavigationDelegate alloc] init]);
     3884    [webView setNavigationDelegate:delegate.get()];
     3885
     3886    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
     3887    [webView loadRequest:request];
     3888    TestWebKitAPI::Util::run(&done);
     3889    done = false;
     3890
     3891    [webView evaluateJavaScript:@"scroll(0, 5000)" completionHandler: [&] (id result, NSError *error) {
     3892        done = true;
     3893    }];
     3894    TestWebKitAPI::Util::run(&done);
     3895    done = false;
     3896
     3897    [webView evaluateJavaScript:@"testLink.click()" completionHandler: nil];
     3898
     3899    TestWebKitAPI::Util::run(&done);
     3900    done = false;
     3901
     3902    [webView evaluateJavaScript:@"window.scrollY" completionHandler: [&] (id result, NSError *error) {
     3903        EXPECT_EQ(0, [result integerValue]);
     3904        done = true;
     3905    }];
     3906    TestWebKitAPI::Util::run(&done);
     3907    done = false;
     3908
     3909    [webView goBack];
     3910    TestWebKitAPI::Util::run(&done);
     3911    done = false;
     3912
     3913    [webView evaluateJavaScript:@"window.scrollY" completionHandler: [&] (id result, NSError *error) {
     3914        EXPECT_EQ(5000, [result integerValue]);
     3915        done = true;
     3916    }];
     3917    TestWebKitAPI::Util::run(&done);
     3918    done = false;
     3919}
     3920
    38493921#endif // PLATFORM(MAC)
    38503922
Note: See TracChangeset for help on using the changeset viewer.