Regression(PSON) Scroll position is not always restored properly when navigating...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 18 Jan 2019 19:47:33 +0000 (19:47 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 18 Jan 2019 19:47:33 +0000 (19:47 +0000)
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:

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

Source/WebKit/ChangeLog
Source/WebKit/Shared/SessionState.cpp
Source/WebKit/Shared/SessionState.h
Source/WebKit/UIProcess/Cocoa/ViewGestureController.cpp
Source/WebKit/UIProcess/WebProcessProxy.cpp
Source/WebKit/UIProcess/WebProcessProxy.h
Source/WebKit/WebProcess/WebCoreSupport/SessionStateConversion.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

index 692389c..04862d5 100644 (file)
@@ -1,3 +1,63 @@
+2019-01-18  Chris Dumez  <cdumez@apple.com>
+
+        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.
+
+        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".
+
 2019-01-18  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         Remove some last vestiges of assisted node terminology in WebKit
index b6c4209..538c022 100644 (file)
@@ -128,6 +128,7 @@ void FrameState::encode(IPC::Encoder& encoder) const
     encoder << minimumLayoutSizeInScrollViewCoordinates;
     encoder << contentSize;
     encoder << scaleIsInitial;
+    encoder << obscuredInsets;
 #endif
 
     encoder << children;
@@ -176,6 +177,8 @@ Optional<FrameState> FrameState::decode(IPC::Decoder& decoder)
         return WTF::nullopt;
     if (!decoder.decode(result.scaleIsInitial))
         return WTF::nullopt;
+    if (!decoder.decode(result.obscuredInsets))
+        return WTF::nullopt;
 #endif
 
     if (!decoder.decode(result.children))
index e7bfbb4..d26791d 100644 (file)
@@ -110,6 +110,7 @@ struct FrameState {
     WebCore::FloatSize minimumLayoutSizeInScrollViewCoordinates;
     WebCore::IntSize contentSize;
     bool scaleIsInitial { false };
+    WebCore::FloatBoxExtent obscuredInsets;
 #endif
 
     Vector<FrameState> children;
index d4faa93..0fffd9a 100644 (file)
@@ -193,17 +193,6 @@ void ViewGestureController::didReachMainFrameLoadTerminalState()
     // enough for us too.
     m_snapshotRemovalTracker.cancelOutstandingEvent(SnapshotRemovalTracker::VisuallyNonEmptyLayout);
 
-    // With Web-process scrolling, we check if the scroll position restoration succeeded by comparing the
-    // requested and actual scroll position. It's possible that we will never succeed in restoring
-    // the exact scroll position we wanted, in the case of a dynamic page, but we know that by
-    // main frame load time that we've gotten as close as we're going to get, so stop waiting.
-    // We don't want to do this with UI-side scrolling because scroll position restoration is baked into the transaction.
-    // FIXME: It seems fairly dirty to type-check the DrawingArea like this.
-    if (auto drawingArea = m_webPageProxy.drawingArea()) {
-        if (is<RemoteLayerTreeDrawingAreaProxy>(drawingArea))
-            m_snapshotRemovalTracker.cancelOutstandingEvent(SnapshotRemovalTracker::ScrollPositionRestoration);
-    }
-
     checkForActiveLoads();
 }
 
index 6be9dc3..1b3f80c 100644 (file)
@@ -585,25 +585,10 @@ bool WebProcessProxy::fullKeyboardAccessEnabled()
 }
 #endif
 
-bool WebProcessProxy::hasProvisionalPageWithID(uint64_t pageID) const
-{
-    for (auto* provisionalPage : m_provisionalPages) {
-        if (provisionalPage->page().pageID() == pageID)
-            return true;
-    }
-    return false;
-}
-
 void WebProcessProxy::updateBackForwardItem(const BackForwardListItemState& itemState)
 {
-    if (auto* item = WebBackForwardListItem::itemForID(itemState.identifier)) {
-        // This update could be coming from a web process that is not the active process for
-        // the back/forward items page.
-        // e.g. The old web process is navigating to about:blank for suspension.
-        // We ignore these updates.
-        if (m_pageMap.contains(item->pageID()) || hasProvisionalPageWithID(item->pageID()))
-            item->setPageState(itemState.pageState);
-    }
+    if (auto* item = WebBackForwardListItem::itemForID(itemState.identifier))
+        item->setPageState(itemState.pageState);
 }
 
 #if ENABLE(NETSCAPE_PLUGIN_API)
index 5042630..ff9e52c 100644 (file)
@@ -334,8 +334,6 @@ private:
 
     void logDiagnosticMessageForResourceLimitTermination(const String& limitKey);
 
-    bool hasProvisionalPageWithID(uint64_t pageID) const;
-
     enum class IsWeak { No, Yes };
     template<typename T> class WeakOrStrongPtr {
     public:
index 400d832..63c36a3 100644 (file)
@@ -96,6 +96,7 @@ static FrameState toFrameState(const HistoryItem& historyItem)
     frameState.minimumLayoutSizeInScrollViewCoordinates = historyItem.minimumLayoutSizeInScrollViewCoordinates();
     frameState.contentSize = historyItem.contentSize();
     frameState.scaleIsInitial = historyItem.scaleIsInitial();
+    frameState.obscuredInsets = historyItem.obscuredInsets();
 #endif
 
     for (auto& childHistoryItem : historyItem.children()) {
@@ -173,6 +174,7 @@ static void applyFrameState(HistoryItem& historyItem, const FrameState& frameSta
     historyItem.setMinimumLayoutSizeInScrollViewCoordinates(frameState.minimumLayoutSizeInScrollViewCoordinates);
     historyItem.setContentSize(frameState.contentSize);
     historyItem.setScaleIsInitial(frameState.scaleIsInitial);
+    historyItem.setObscuredInsets(frameState.obscuredInsets);
 #endif
 
     for (const auto& childFrameState : frameState.children) {
index 4ce859c..e107a2d 100644 (file)
@@ -1,3 +1,15 @@
+2019-01-18  Chris Dumez  <cdumez@apple.com>
+
+        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.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
 2019-01-18  Youenn Fablet  <youenn@apple.com>
 
         Add a new SPI to request for cache storage quota increase
index 17c3e22..afa29c6 100644 (file)
@@ -3846,6 +3846,78 @@ TEST(ProcessSwap, GoBackToSuspendedPageWithMainFrameIDThatIsNotOne)
     EXPECT_EQ(pid1, pid5);
 }
 
+static const char* tallPageBytes = R"PSONRESOURCE(
+<!DOCTYPE html>
+<html>
+<head>
+<meta name='viewport' content='width=device-width, initial-scale=1'>
+<style>
+body {
+    margin: 0;
+    width: 100%;
+    height: 10000px;
+}
+</style>
+</head>
+<body>
+<a id="testLink" href="pson://www.apple.com/main.html">Test</a>
+</body>
+</html>
+)PSONRESOURCE";
+
+TEST(ProcessSwap, ScrollPositionRestoration)
+{
+    auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
+    [processPoolConfiguration setProcessSwapsOnNavigation:YES];
+    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:tallPageBytes];
+    [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
+
+    [[webViewConfiguration preferences] _setUsesPageCache:NO];
+
+    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;
+
+    [webView evaluateJavaScript:@"scroll(0, 5000)" completionHandler: [&] (id result, NSError *error) {
+        done = true;
+    }];
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    [webView evaluateJavaScript:@"testLink.click()" completionHandler: nil];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    [webView evaluateJavaScript:@"window.scrollY" completionHandler: [&] (id result, NSError *error) {
+        EXPECT_EQ(0, [result integerValue]);
+        done = true;
+    }];
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    [webView goBack];
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    [webView evaluateJavaScript:@"window.scrollY" completionHandler: [&] (id result, NSError *error) {
+        EXPECT_EQ(5000, [result integerValue]);
+        done = true;
+    }];
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+}
+
 #endif // PLATFORM(MAC)
 
 #endif // WK_API_ENABLED