Changeset 240161 in webkit
- Timestamp:
- Jan 18, 2019 11:47:33 AM (5 years ago)
- Location:
- trunk
- Files:
-
- 9 edited
Legend:
- Unmodified
- Added
- Removed
-
trunk/Source/WebKit/ChangeLog
r240159 r240161 1 2019-01-18 Chris Dumez <cdumez@apple.com> 2 3 Regression(PSON) Scroll position is not always restored properly when navigating back 4 https://bugs.webkit.org/show_bug.cgi?id=193578 5 <rdar://problem/47386331> 6 7 Reviewed by Tim Horton. 8 9 Fix issues causing the scroll position to not be restored at all (or incorrectly) when 10 navigating back cross-site with PSON enabled. Also make sure that the swipe gesture 11 snapshot really stays up until we've restored the scroll position. 12 13 Note that even after those changes, I can still sometimes reproduce a white flash when 14 swiping back to Google search results (scroll position being correct now). This is 15 tracked by <rdar://problem/47071684> and happens even if I disable PSON entirely. 16 17 * Shared/SessionState.cpp: 18 (WebKit::FrameState::encode const): 19 (WebKit::FrameState::decode): 20 * Shared/SessionState.h: 21 * WebProcess/WebCoreSupport/SessionStateConversion.cpp: 22 (WebKit::toFrameState): 23 (WebKit::applyFrameState): 24 obscuredInsets is present on the HistoryItem in the WebProcess but was never passed to 25 or stored by the UIProcess on the WebBackForwardListItem. obscuredInsets is needed to 26 properly restore the scrollPosition (position was 70px off on my iPad without this). 27 With PSON enabled, if you swipe back cross-process and the previous page was not put 28 into PageCache, then the HistoryItem is gone on the WebProcess side. What happens is 29 that the UIProcess sends its WebBackForwardListItem to the WebProcess, which restores 30 the HistoryItem there, and then asks it to load it. The obscuredInsets was getting lost 31 in the process since the UIProcess never knew about it. 32 33 * UIProcess/Cocoa/ViewGestureController.cpp: 34 (WebKit::ViewGestureController::didReachMainFrameLoadTerminalState): 35 Drop logic that was causing the ViewGestureController to not wait for the scroll position 36 to be restored before taking down the snapshot, when UI-side compositing is enabled. 37 If you look at the comment above the code, you'll see that the code in question was meant 38 to impact only the non-UI side compositing code path. As a matter of fact, when the code 39 was reviewed at https://bugs.webkit.org/show_bug.cgi?id=151224, it was protected by a 40 #if PLATFORM(MAC), before getting modified the wrong way before landing. In practice, we 41 would have often restored the scroll position by the time the load is finished so it would 42 not cause a flash in most cases. However, with PSON enabled and the layer tree freezing we 43 do on process-swap, the first post-scroll restoration layer tree commit may now occur a 44 little bit later and we would lose the race more often. 45 46 * UIProcess/WebProcessProxy.cpp: 47 (WebKit::WebProcessProxy::updateBackForwardItem): 48 * UIProcess/WebProcessProxy.h: 49 When adding PageCache support to PSON, we used to navigate the "suspended" page to 50 about:blank. This would lead to unwanted WebProcessProxy::updateBackForwardItem() 51 calls from the WebProcess which we wanted to ignore. We thus added logic to ignore 52 updateBackForwardItem() IPC from the old WebProcess after a swap. The issue with this 53 is that we sometimes miss/ignore legit updates to the HistoryItem from the old process, 54 in particular with regards to the scroll position and the pageScaleFactor. So if you 55 swiped and then quickly enough did a cross-site navigation, the UIProcess' 56 WebBackForwardList would not get updated with the latest scroll position and we would 57 thus fail to restore it later on. To address the issue, we now stop ignoring updates 58 from the old WebProcess after a swap. This logic is no longer needed since we no longer 59 navigate the old page to about:blank after a swap, we merely suspend it "in place". 60 1 61 2019-01-18 Wenson Hsieh <wenson_hsieh@apple.com> 2 62 -
trunk/Source/WebKit/Shared/SessionState.cpp
r239427 r240161 129 129 encoder << contentSize; 130 130 encoder << scaleIsInitial; 131 encoder << obscuredInsets; 131 132 #endif 132 133 … … 176 177 return WTF::nullopt; 177 178 if (!decoder.decode(result.scaleIsInitial)) 179 return WTF::nullopt; 180 if (!decoder.decode(result.obscuredInsets)) 178 181 return WTF::nullopt; 179 182 #endif -
trunk/Source/WebKit/Shared/SessionState.h
r239427 r240161 111 111 WebCore::IntSize contentSize; 112 112 bool scaleIsInitial { false }; 113 WebCore::FloatBoxExtent obscuredInsets; 113 114 #endif 114 115 -
trunk/Source/WebKit/UIProcess/Cocoa/ViewGestureController.cpp
r238818 r240161 194 194 m_snapshotRemovalTracker.cancelOutstandingEvent(SnapshotRemovalTracker::VisuallyNonEmptyLayout); 195 195 196 // With Web-process scrolling, we check if the scroll position restoration succeeded by comparing the197 // requested and actual scroll position. It's possible that we will never succeed in restoring198 // the exact scroll position we wanted, in the case of a dynamic page, but we know that by199 // main frame load time that we've gotten as close as we're going to get, so stop waiting.200 // We don't want to do this with UI-side scrolling because scroll position restoration is baked into the transaction.201 // FIXME: It seems fairly dirty to type-check the DrawingArea like this.202 if (auto drawingArea = m_webPageProxy.drawingArea()) {203 if (is<RemoteLayerTreeDrawingAreaProxy>(drawingArea))204 m_snapshotRemovalTracker.cancelOutstandingEvent(SnapshotRemovalTracker::ScrollPositionRestoration);205 }206 207 196 checkForActiveLoads(); 208 197 } -
trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp
r240046 r240161 586 586 #endif 587 587 588 bool WebProcessProxy::hasProvisionalPageWithID(uint64_t pageID) const589 {590 for (auto* provisionalPage : m_provisionalPages) {591 if (provisionalPage->page().pageID() == pageID)592 return true;593 }594 return false;595 }596 597 588 void WebProcessProxy::updateBackForwardItem(const BackForwardListItemState& itemState) 598 589 { 599 if (auto* item = WebBackForwardListItem::itemForID(itemState.identifier)) { 600 // This update could be coming from a web process that is not the active process for 601 // the back/forward items page. 602 // e.g. The old web process is navigating to about:blank for suspension. 603 // We ignore these updates. 604 if (m_pageMap.contains(item->pageID()) || hasProvisionalPageWithID(item->pageID())) 605 item->setPageState(itemState.pageState); 606 } 590 if (auto* item = WebBackForwardListItem::itemForID(itemState.identifier)) 591 item->setPageState(itemState.pageState); 607 592 } 608 593 -
trunk/Source/WebKit/UIProcess/WebProcessProxy.h
r240046 r240161 335 335 void logDiagnosticMessageForResourceLimitTermination(const String& limitKey); 336 336 337 bool hasProvisionalPageWithID(uint64_t pageID) const;338 339 337 enum class IsWeak { No, Yes }; 340 338 template<typename T> class WeakOrStrongPtr { -
trunk/Source/WebKit/WebProcess/WebCoreSupport/SessionStateConversion.cpp
r239461 r240161 97 97 frameState.contentSize = historyItem.contentSize(); 98 98 frameState.scaleIsInitial = historyItem.scaleIsInitial(); 99 frameState.obscuredInsets = historyItem.obscuredInsets(); 99 100 #endif 100 101 … … 174 175 historyItem.setContentSize(frameState.contentSize); 175 176 historyItem.setScaleIsInitial(frameState.scaleIsInitial); 177 historyItem.setObscuredInsets(frameState.obscuredInsets); 176 178 #endif 177 179 -
trunk/Tools/ChangeLog
r240156 r240161 1 2019-01-18 Chris Dumez <cdumez@apple.com> 2 3 Regression(PSON) Scroll position is not always restored properly when navigating back 4 https://bugs.webkit.org/show_bug.cgi?id=193578 5 <rdar://problem/47386331> 6 7 Reviewed by Tim Horton. 8 9 Add API test coverage. 10 11 * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm: 12 1 13 2019-01-18 Youenn Fablet <youenn@apple.com> 2 14 -
trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
r240046 r240161 3847 3847 } 3848 3848 3849 static const char* tallPageBytes = R"PSONRESOURCE( 3850 <!DOCTYPE html> 3851 <html> 3852 <head> 3853 <meta name='viewport' content='width=device-width, initial-scale=1'> 3854 <style> 3855 body { 3856 margin: 0; 3857 width: 100%; 3858 height: 10000px; 3859 } 3860 </style> 3861 </head> 3862 <body> 3863 <a id="testLink" href="pson://www.apple.com/main.html">Test</a> 3864 </body> 3865 </html> 3866 )PSONRESOURCE"; 3867 3868 TEST(ProcessSwap, ScrollPositionRestoration) 3869 { 3870 auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]); 3871 [processPoolConfiguration setProcessSwapsOnNavigation:YES]; 3872 auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]); 3873 3874 auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]); 3875 [webViewConfiguration setProcessPool:processPool.get()]; 3876 auto handler = adoptNS([[PSONScheme alloc] init]); 3877 [handler addMappingFromURLString:@"pson://www.webkit.org/main.html" toData:tallPageBytes]; 3878 [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"]; 3879 3880 [[webViewConfiguration preferences] _setUsesPageCache:NO]; 3881 3882 auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]); 3883 auto delegate = adoptNS([[PSONNavigationDelegate alloc] init]); 3884 [webView setNavigationDelegate:delegate.get()]; 3885 3886 NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]]; 3887 [webView loadRequest:request]; 3888 TestWebKitAPI::Util::run(&done); 3889 done = false; 3890 3891 [webView evaluateJavaScript:@"scroll(0, 5000)" completionHandler: [&] (id result, NSError *error) { 3892 done = true; 3893 }]; 3894 TestWebKitAPI::Util::run(&done); 3895 done = false; 3896 3897 [webView evaluateJavaScript:@"testLink.click()" completionHandler: nil]; 3898 3899 TestWebKitAPI::Util::run(&done); 3900 done = false; 3901 3902 [webView evaluateJavaScript:@"window.scrollY" completionHandler: [&] (id result, NSError *error) { 3903 EXPECT_EQ(0, [result integerValue]); 3904 done = true; 3905 }]; 3906 TestWebKitAPI::Util::run(&done); 3907 done = false; 3908 3909 [webView goBack]; 3910 TestWebKitAPI::Util::run(&done); 3911 done = false; 3912 3913 [webView evaluateJavaScript:@"window.scrollY" completionHandler: [&] (id result, NSError *error) { 3914 EXPECT_EQ(5000, [result integerValue]); 3915 done = true; 3916 }]; 3917 TestWebKitAPI::Util::run(&done); 3918 done = false; 3919 } 3920 3849 3921 #endif // PLATFORM(MAC) 3850 3922
Note: See TracChangeset
for help on using the changeset viewer.