- Revision
- 241950
- Author
- [email protected]
- Date
- 2019-02-22 10:24:57 -0800 (Fri, 22 Feb 2019)
Log 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:
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (241949 => 241950)
--- trunk/Source/WebKit/ChangeLog 2019-02-22 17:09:27 UTC (rev 241949)
+++ trunk/Source/WebKit/ChangeLog 2019-02-22 18:24:57 UTC (rev 241950)
@@ -1,3 +1,27 @@
+2019-02-22 Chris Dumez <[email protected]>
+
+ 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 <[email protected]>
[WPE][GTK] No API documentation generated for WebKitUserContentFilterStore
Modified: trunk/Source/WebKit/UIProcess/API/Cocoa/WKBackForwardListItem.mm (241949 => 241950)
--- trunk/Source/WebKit/UIProcess/API/Cocoa/WKBackForwardListItem.mm 2019-02-22 17:09:27 UTC (rev 241949)
+++ trunk/Source/WebKit/UIProcess/API/Cocoa/WKBackForwardListItem.mm 2019-02-22 18:24:57 UTC (rev 241950)
@@ -71,6 +71,11 @@
return nullptr;
}
+- (CGPoint) _scrollPosition
+{
+ return CGPointMake(_item->pageState().mainFrameState.scrollPosition.x(), _item->pageState().mainFrameState.scrollPosition.y());
+}
+
#pragma mark WKObject protocol implementation
- (API::Object&)_apiObject
Modified: trunk/Source/WebKit/UIProcess/API/Cocoa/WKBackForwardListItemPrivate.h (241949 => 241950)
--- trunk/Source/WebKit/UIProcess/API/Cocoa/WKBackForwardListItemPrivate.h 2019-02-22 17:09:27 UTC (rev 241949)
+++ trunk/Source/WebKit/UIProcess/API/Cocoa/WKBackForwardListItemPrivate.h 2019-02-22 18:24:57 UTC (rev 241950)
@@ -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
Modified: trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp (241949 => 241950)
--- trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp 2019-02-22 17:09:27 UTC (rev 241949)
+++ trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp 2019-02-22 18:24:57 UTC (rev 241950)
@@ -2704,6 +2704,9 @@
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);
Modified: trunk/Tools/ChangeLog (241949 => 241950)
--- trunk/Tools/ChangeLog 2019-02-22 17:09:27 UTC (rev 241949)
+++ trunk/Tools/ChangeLog 2019-02-22 18:24:57 UTC (rev 241950)
@@ -1,5 +1,17 @@
2019-02-22 Chris Dumez <[email protected]>
+ 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 <[email protected]>
+
Unreviewed, disable API test added in r241928 on iOS.
The cache is not enabled on devices with less than 3GB of RAM.
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (241949 => 241950)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2019-02-22 17:09:27 UTC (rev 241949)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2019-02-22 18:24:57 UTC (rev 241950)
@@ -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 @@
EXPECT_EQ(pid1, pid5);
}
+#endif // PLATFORM(MAC)
+
static const char* tallPageBytes = R"PSONRESOURCE(
<!DOCTYPE html>
<html>
@@ -4875,11 +4878,30 @@
</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=""
</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 @@
[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 @@
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,16 +4948,39 @@
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;
+
+ [webView goBack];
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ scrollPosition = waitUntilScrollPositionIsRestored(webView.get());
+ EXPECT_EQ(4000U, scrollPosition);
}
-#endif // PLATFORM(MAC)
-
static NSString *blockmeFilter = @"[{\"action\":{\"type\":\"block\"},\"trigger\":{\"url-filter\":\".*blockme.html\"}}]";
static const char* contentBlockingAfterProcessSwapTestBytes = R"PSONRESOURCE(
