Attachment 359499 Details for Bug 193578 – Patch

[patch] Patch

bug-193578-20190118094053.patch (text/plain), 13.99 KB, created by Chris Dumez on 2019-01-18 09:40:54 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-01-18 09:40:54 PST

Size: 13.99 KB

patch

obsolete

>Subversion Revision: 240114
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index e492f8af27f8e1849826495a30f68393b87ff200..2845a1c19981e3b3038ecde98e547eacba93fd9b 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        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. Howerver, 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-16  Youenn Fablet  <youenn@apple.com>
> 
>         Add a new SPI for controlling getUserMedia
>diff --git a/Source/WebKit/Shared/SessionState.cpp b/Source/WebKit/Shared/SessionState.cpp
>index b6c42098978a8aad08995851010de482b42df742..538c02284531374324186f234c83b5a3a7caa8bd 100644
>--- a/Source/WebKit/Shared/SessionState.cpp
>+++ b/Source/WebKit/Shared/SessionState.cpp
>@@ -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))
>diff --git a/Source/WebKit/Shared/SessionState.h b/Source/WebKit/Shared/SessionState.h
>index e7bfbb4643f9e398e52c12862d634ca950472e2c..d26791da99c36d284ff2e7d50ce082678d0f08b0 100644
>--- a/Source/WebKit/Shared/SessionState.h
>+++ b/Source/WebKit/Shared/SessionState.h
>@@ -110,6 +110,7 @@ struct FrameState {
>     WebCore::FloatSize minimumLayoutSizeInScrollViewCoordinates;
>     WebCore::IntSize contentSize;
>     bool scaleIsInitial { false };
>+    WebCore::FloatBoxExtent obscuredInsets;
> #endif
> 
>     Vector<FrameState> children;
>diff --git a/Source/WebKit/UIProcess/Cocoa/ViewGestureController.cpp b/Source/WebKit/UIProcess/Cocoa/ViewGestureController.cpp
>index d4faa93e3d913caefc40ca17fe8ab4b933be66b5..0fffd9a4574426f4eda96382ac78a2f6908805cb 100644
>--- a/Source/WebKit/UIProcess/Cocoa/ViewGestureController.cpp
>+++ b/Source/WebKit/UIProcess/Cocoa/ViewGestureController.cpp
>@@ -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();
> }
> 
>diff --git a/Source/WebKit/UIProcess/WebProcessProxy.cpp b/Source/WebKit/UIProcess/WebProcessProxy.cpp
>index 6be9dc399e01488da678fb4568ef36b21f3c4876..1b3f80c1783d474072c2732dc239ab17f32de200 100644
>--- a/Source/WebKit/UIProcess/WebProcessProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebProcessProxy.cpp
>@@ -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)
>diff --git a/Source/WebKit/UIProcess/WebProcessProxy.h b/Source/WebKit/UIProcess/WebProcessProxy.h
>index 5042630de371d85a828374348aa9691739c9cd70..ff9e52cc31f0f808a3f03d6c1cc9b0c783f21749 100644
>--- a/Source/WebKit/UIProcess/WebProcessProxy.h
>+++ b/Source/WebKit/UIProcess/WebProcessProxy.h
>@@ -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:
>diff --git a/Source/WebKit/WebProcess/WebCoreSupport/SessionStateConversion.cpp b/Source/WebKit/WebProcess/WebCoreSupport/SessionStateConversion.cpp
>index 400d8325edee57c8f45c668f591f3eef0dd1f17c..63c36a3db69a82ad3bea660f6b06dfe763c55617 100644
>--- a/Source/WebKit/WebProcess/WebCoreSupport/SessionStateConversion.cpp
>+++ b/Source/WebKit/WebProcess/WebCoreSupport/SessionStateConversion.cpp
>@@ -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) {
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index abbe4c1dc148c60b967d182f84d36a791a88bd91..d3f7edc745af988e0fe876e8363cef8c4b85a846 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        Add API test coverage.
>+
>+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
>+
> 2019-01-17  Zalan Bujtas  <zalan@apple.com>
> 
>         [LFC][BFC] An element with transform is a containing block for positioned descendants.
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>index 17c3e22ec7b3c14b2ef07a3ca545ad57090be205..afa29c6a712f07c83b914b80a08c4ce6f09375ec 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>@@ -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

Flags:

thorton: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 193578: 359499