Changeset 241950 in webkit

Timestamp:

Feb 22, 2019 10:24:57 AM (5 years ago)

Author:

Chris Dumez

Message:

REGRESSION(PSON) Scroll position is sometimes not restored on history navigation
​https://bugs.webkit.org/show_bug.cgi?id=194924
<rdar://problem/48216125>

Reviewed by Geoffrey Garen.

Source/WebKit:

When process-swapping, we would create a new WebPage in the new process, which would
call restoreSessionInternal() to restore the HistoryItems based on the UIProcess's
backforward list. The issue is that this session restoring would send HistoryItem
updates back to the UIProcess. Without PSON, this would be unnecessary but harmless.
With PSON though, this may end up overwriting values set by the previous process,
such as the scroll position.

Address the issue by temporarily disabling the HistoryItem update notifications to
the UIProcess while restoring a session.

• UIProcess/API/Cocoa/WKBackForwardListItem.mm:

(-[WKBackForwardListItem _scrollPosition]):

• UIProcess/API/Cocoa/WKBackForwardListItemPrivate.h:
• WebProcess/WebPage/WebPage.cpp:

(WebKit::WebPage::restoreSessionInternal):

Tools:

Add API test coverage.

• TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

Location:

trunk

Files:

• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/API/Cocoa/WKBackForwardListItem.mm (modified) (1 diff)
• Source/WebKit/UIProcess/API/Cocoa/WKBackForwardListItemPrivate.h (modified) (1 diff)
• Source/WebKit/WebProcess/WebPage/WebPage.cpp (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (modified) (7 diffs)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-02-22  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION(PSON) Scroll position is sometimes not restored on history navigation
+        https://bugs.webkit.org/show_bug.cgi?id=194924
+        <rdar://problem/48216125>
+
+        Reviewed by Geoffrey Garen.
+
+        When process-swapping, we would create a new WebPage in the new process, which would
+        call restoreSessionInternal() to restore the HistoryItems based on the UIProcess's
+        backforward list. The issue is that this session restoring would send HistoryItem
+        updates back to the UIProcess. Without PSON, this would be unnecessary but harmless.
+        With PSON though, this may end up overwriting values set by the previous process,
+        such as the scroll position.
+
+        Address the issue by temporarily disabling the HistoryItem update notifications to
+        the UIProcess while restoring a session.
+
+        * UIProcess/API/Cocoa/WKBackForwardListItem.mm:
+        (-[WKBackForwardListItem _scrollPosition]):
+        * UIProcess/API/Cocoa/WKBackForwardListItemPrivate.h:
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::restoreSessionInternal):
+
 2019-02-21  Adrian Perez de Castro  <aperez@igalia.com>
 

trunk/Source/WebKit/UIProcess/API/Cocoa/WKBackForwardListItem.mm

@@ -72 +72 @@
 }
 
+- (CGPoint) _scrollPosition
+{
+    return CGPointMake(_item->pageState().mainFrameState.scrollPosition.x(), _item->pageState().mainFrameState.scrollPosition.y());
+}
+
 #pragma mark WKObject protocol implementation
 

trunk/Source/WebKit/UIProcess/API/Cocoa/WKBackForwardListItemPrivate.h

@@ -33 +33 @@
 - (CGImageRef)_copySnapshotForTesting WK_API_AVAILABLE(macosx(10.12.3), ios(10.3));
 
+@property (nonatomic) CGPoint _scrollPosition WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
+
 @end
 

trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp

@@ -2705 +2705 @@
 void WebPage::restoreSessionInternal(const Vector<BackForwardListItemState>& itemStates, WasRestoredByAPIRequest restoredByAPIRequest, WebBackForwardListProxy::OverwriteExistingItem overwrite)
 {
+    // Since we're merely restoring HistoryItems from the UIProcess, there is no need to send HistoryItem update notifications back to the UIProcess.
+    // Also, with process-swap on navigation, these updates may actually overwrite important state in the UIProcess such as the scroll position.
+    SetForScope<void (*)(WebCore::HistoryItem&)> bypassHistoryItemUpdateNotifications(WebCore::notifyHistoryItemChanged, [](WebCore::HistoryItem&){});
     for (const auto& itemState : itemStates) {
         auto historyItem = toHistoryItem(itemState);

trunk/Tools/ChangeLog

@@ +1 @@
+2019-02-22  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION(PSON) Scroll position is sometimes not restored on history navigation
+        https://bugs.webkit.org/show_bug.cgi?id=194924
+        <rdar://problem/48216125>
+
+        Reviewed by Geoffrey Garen.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
 2019-02-22  Chris Dumez  <cdumez@apple.com>
 

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

@@ -30 +30 @@
 #import "TestNavigationDelegate.h"
 #import "TestWKWebView.h"
+#import <WebKit/WKBackForwardListItemPrivate.h>
 #import <WebKit/WKContentRuleListStore.h>
 #import <WebKit/WKNavigationDelegatePrivate.h>
@@ -4862 +4863 @@
 }
 
+#endif // PLATFORM(MAC)
+
 static const char* tallPageBytes = R"PSONRESOURCE(
 <!DOCTYPE html>
@@ -4876 +4879 @@
 </head>
 <body>
+<script>
+// Pages with dedicated workers do not go into page cache.
+var myWorker = new Worker('worker.js');
+</script>
 <a id="testLink" href="pson://www.apple.com/main.html">Test</a>
 </body>
@@ -4881 +4888 @@
 )PSONRESOURCE";
 
+static unsigned waitUntilScrollPositionIsRestored(WKWebView *webView)
+{
+    unsigned scrollPosition = 0;
+    do {
+        [webView evaluateJavaScript:@"window.scrollY" completionHandler: [&] (id result, NSError *error) {
+            scrollPosition = [result integerValue];
+            done = true;
+        }];
+        TestWebKitAPI::Util::run(&done);
+        done = false;
+    } while (!scrollPosition);
+
+    return scrollPosition;
+}
+
 TEST(ProcessSwap, ScrollPositionRestoration)
 {
@@ -4891 +4913 @@
     [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()]);
@@ -4908 +4928 @@
     TestWebKitAPI::Util::run(&done);
     done = false;
+
+    do {
+        TestWebKitAPI::Util::sleep(0.05);
+    } while (lroundf([[[webView backForwardList] currentItem] _scrollPosition].y) != 5000);
 
     [webView evaluateJavaScript:@"testLink.click()" completionHandler: nil];
@@ -4925 +4949 @@
     done = false;
 
-    [webView evaluateJavaScript:@"window.scrollY" completionHandler: [&] (id result, NSError *error) {
-        EXPECT_EQ(5000, [result integerValue]);
+    auto scrollPosition = waitUntilScrollPositionIsRestored(webView.get());
+    EXPECT_EQ(5000U, scrollPosition);
+
+    [webView evaluateJavaScript:@"scroll(0, 4000)" completionHandler: [&] (id result, NSError *error) {
         done = true;
     }];
     TestWebKitAPI::Util::run(&done);
     done = false;
-}
-
-#endif // PLATFORM(MAC)
+
+    do {
+        TestWebKitAPI::Util::sleep(0.05);
+    } while (lroundf([[[webView backForwardList] currentItem] _scrollPosition].y) != 4000);
+
+    [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;
+
+    scrollPosition = waitUntilScrollPositionIsRestored(webView.get());
+    EXPECT_EQ(4000U, scrollPosition);
+}
 
 static NSString *blockmeFilter = @"[{\"action\":{\"type\":\"block\"},\"trigger\":{\"url-filter\":\".*blockme.html\"}}]";