- Revision
- 240161
- Author
- cdu...@apple.com
- Date
- 2019-01-18 11:47:33 -0800 (Fri, 18 Jan 2019)
Log Message
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.
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:
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (240160 => 240161)
--- trunk/Source/WebKit/ChangeLog 2019-01-18 19:38:56 UTC (rev 240160)
+++ trunk/Source/WebKit/ChangeLog 2019-01-18 19:47:33 UTC (rev 240161)
@@ -1,3 +1,63 @@
+2019-01-18 Chris Dumez <cdu...@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_hs...@apple.com>
Remove some last vestiges of assisted node terminology in WebKit
Modified: trunk/Source/WebKit/Shared/SessionState.cpp (240160 => 240161)
--- trunk/Source/WebKit/Shared/SessionState.cpp 2019-01-18 19:38:56 UTC (rev 240160)
+++ trunk/Source/WebKit/Shared/SessionState.cpp 2019-01-18 19:47:33 UTC (rev 240161)
@@ -128,6 +128,7 @@
encoder << minimumLayoutSizeInScrollViewCoordinates;
encoder << contentSize;
encoder << scaleIsInitial;
+ encoder << obscuredInsets;
#endif
encoder << children;
@@ -176,6 +177,8 @@
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))
Modified: trunk/Source/WebKit/Shared/SessionState.h (240160 => 240161)
--- trunk/Source/WebKit/Shared/SessionState.h 2019-01-18 19:38:56 UTC (rev 240160)
+++ trunk/Source/WebKit/Shared/SessionState.h 2019-01-18 19:47:33 UTC (rev 240161)
@@ -110,6 +110,7 @@
WebCore::FloatSize minimumLayoutSizeInScrollViewCoordinates;
WebCore::IntSize contentSize;
bool scaleIsInitial { false };
+ WebCore::FloatBoxExtent obscuredInsets;
#endif
Vector<FrameState> children;
Modified: trunk/Source/WebKit/UIProcess/Cocoa/ViewGestureController.cpp (240160 => 240161)
--- trunk/Source/WebKit/UIProcess/Cocoa/ViewGestureController.cpp 2019-01-18 19:38:56 UTC (rev 240160)
+++ trunk/Source/WebKit/UIProcess/Cocoa/ViewGestureController.cpp 2019-01-18 19:47:33 UTC (rev 240161)
@@ -193,17 +193,6 @@
// 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();
}
Modified: trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp (240160 => 240161)
--- trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp 2019-01-18 19:38:56 UTC (rev 240160)
+++ trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp 2019-01-18 19:47:33 UTC (rev 240161)
@@ -585,25 +585,10 @@
}
#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)
Modified: trunk/Source/WebKit/UIProcess/WebProcessProxy.h (240160 => 240161)
--- trunk/Source/WebKit/UIProcess/WebProcessProxy.h 2019-01-18 19:38:56 UTC (rev 240160)
+++ trunk/Source/WebKit/UIProcess/WebProcessProxy.h 2019-01-18 19:47:33 UTC (rev 240161)
@@ -334,8 +334,6 @@
void logDiagnosticMessageForResourceLimitTermination(const String& limitKey);
- bool hasProvisionalPageWithID(uint64_t pageID) const;
-
enum class IsWeak { No, Yes };
template<typename T> class WeakOrStrongPtr {
public:
Modified: trunk/Source/WebKit/WebProcess/WebCoreSupport/SessionStateConversion.cpp (240160 => 240161)
--- trunk/Source/WebKit/WebProcess/WebCoreSupport/SessionStateConversion.cpp 2019-01-18 19:38:56 UTC (rev 240160)
+++ trunk/Source/WebKit/WebProcess/WebCoreSupport/SessionStateConversion.cpp 2019-01-18 19:47:33 UTC (rev 240161)
@@ -96,6 +96,7 @@
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 @@
historyItem.setMinimumLayoutSizeInScrollViewCoordinates(frameState.minimumLayoutSizeInScrollViewCoordinates);
historyItem.setContentSize(frameState.contentSize);
historyItem.setScaleIsInitial(frameState.scaleIsInitial);
+ historyItem.setObscuredInsets(frameState.obscuredInsets);
#endif
for (const auto& childFrameState : frameState.children) {
Modified: trunk/Tools/ChangeLog (240160 => 240161)
--- trunk/Tools/ChangeLog 2019-01-18 19:38:56 UTC (rev 240160)
+++ trunk/Tools/ChangeLog 2019-01-18 19:47:33 UTC (rev 240161)
@@ -1,3 +1,15 @@
+2019-01-18 Chris Dumez <cdu...@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 <you...@apple.com>
Add a new SPI to request for cache storage quota increase
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (240160 => 240161)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2019-01-18 19:38:56 UTC (rev 240160)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2019-01-18 19:47:33 UTC (rev 240161)
@@ -3846,6 +3846,78 @@
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=""
+</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
