REGRESSION(PSON) Scroll position is sometimes not restored on history navigation
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 22 Feb 2019 18:24:57 +0000 (18:24 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 22 Feb 2019 18:24:57 +0000 (18:24 +0000)
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:

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

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/API/Cocoa/WKBackForwardListItem.mm
Source/WebKit/UIProcess/API/Cocoa/WKBackForwardListItemPrivate.h
Source/WebKit/WebProcess/WebPage/WebPage.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

index 286d055..c047f2b 100644 (file)
@@ -1,3 +1,27 @@
+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>
 
         [WPE][GTK] No API documentation generated for WebKitUserContentFilterStore
index cd818ef..6d8d8b3 100644 (file)
     return nullptr;
 }
 
+- (CGPoint) _scrollPosition
+{
+    return CGPointMake(_item->pageState().mainFrameState.scrollPosition.x(), _item->pageState().mainFrameState.scrollPosition.y());
+}
+
 #pragma mark WKObject protocol implementation
 
 - (API::Object&)_apiObject
index 6238771..3d30ac8 100644 (file)
@@ -32,6 +32,8 @@
 // For testing only.
 - (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
 
 #endif
index 727e01e..192eee4 100644 (file)
@@ -2704,6 +2704,9 @@ void WebPage::setNeedsFontAttributes(bool needsFontAttributes)
 
 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);
         historyItem->setWasRestoredFromSession(restoredByAPIRequest == WasRestoredByAPIRequest::Yes);
index 1005a25..0227c99 100644 (file)
@@ -1,5 +1,17 @@
 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>
+
         Unreviewed, disable API test added in r241928 on iOS.
 
         The cache is not enabled on devices with less than 3GB of RAM.
index a94dd88..aad141b 100644 (file)
@@ -29,6 +29,7 @@
 #import "Test.h"
 #import "TestNavigationDelegate.h"
 #import "TestWKWebView.h"
+#import <WebKit/WKBackForwardListItemPrivate.h>
 #import <WebKit/WKContentRuleListStore.h>
 #import <WebKit/WKNavigationDelegatePrivate.h>
 #import <WebKit/WKNavigationPrivate.h>
@@ -4861,6 +4862,8 @@ TEST(ProcessSwap, GoBackToSuspendedPageWithMainFrameIDThatIsNotOne)
     EXPECT_EQ(pid1, pid5);
 }
 
+#endif // PLATFORM(MAC)
+
 static const char* tallPageBytes = R"PSONRESOURCE(
 <!DOCTYPE html>
 <html>
@@ -4875,11 +4878,30 @@ body {
 </style>
 </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>
 </html>
 )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)
 {
     auto processPoolConfiguration = psonProcessPoolConfiguration();
@@ -4891,8 +4913,6 @@ TEST(ProcessSwap, ScrollPositionRestoration)
     [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()];
@@ -4908,6 +4928,10 @@ TEST(ProcessSwap, ScrollPositionRestoration)
     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];
 
     TestWebKitAPI::Util::run(&done);
@@ -4924,15 +4948,38 @@ TEST(ProcessSwap, ScrollPositionRestoration)
     TestWebKitAPI::Util::run(&done);
     done = false;
 
+    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;
+
+    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(5000, [result integerValue]);
+        EXPECT_EQ(0, [result integerValue]);
         done = true;
     }];
     TestWebKitAPI::Util::run(&done);
     done = false;
-}
 
-#endif // PLATFORM(MAC)
+    [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\"}}]";