Summary: | Regression(r230876): Swipe navigation snapshot may get removed too early | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, ggaren, rniwa, thorton, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2018-05-31 16:48:58 PDT
Created attachment 341708 [details]
WIP Patch
Created attachment 341709 [details]
WIP Patch
Comment on attachment 341709 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341709&action=review > Source/WebKit/UIProcess/ios/ViewGestureControllerIOS.mm:299 > + m_provisionalLoadCallback = [this] { Might as well tuck this down inside just above dispatchAfterEnsuringDrawing, since otherwise we were just going to remove it immediately anyway. Comment on attachment 341709 [details] WIP Patch Attachment 341709 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/7912232 New failing tests: swipe/pushState-cached-back-swipe.html Created attachment 341721 [details]
Archive of layout-test-results from ews105 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 341709 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341709&action=review >> Source/WebKit/UIProcess/ios/ViewGestureControllerIOS.mm:299 >> + m_provisionalLoadCallback = [this] { > > Might as well tuck this down inside just above dispatchAfterEnsuringDrawing, since otherwise we were just going to remove it immediately anyway. Sorry, I do not understands this comment. The dispatchAfterEnsuringDrawing call mis *inside* this lambda. (In reply to Chris Dumez from comment #7) > Comment on attachment 341709 [details] > WIP Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=341709&action=review > > >> Source/WebKit/UIProcess/ios/ViewGestureControllerIOS.mm:299 > >> + m_provisionalLoadCallback = [this] { > > > > Might as well tuck this down inside just above dispatchAfterEnsuringDrawing, since otherwise we were just going to remove it immediately anyway. > > Sorry, I do not understands this comment. The dispatchAfterEnsuringDrawing > call mis *inside* this lambda. In English this time :) Sorry, I do not understand this comment. The dispatchAfterEnsuringDrawing call is *inside* this lambda. Created attachment 341767 [details]
Patch
(In reply to Chris Dumez from comment #8) > (In reply to Chris Dumez from comment #7) > > Comment on attachment 341709 [details] > > WIP Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=341709&action=review > > > > >> Source/WebKit/UIProcess/ios/ViewGestureControllerIOS.mm:299 > > >> + m_provisionalLoadCallback = [this] { > > > > > > Might as well tuck this down inside just above dispatchAfterEnsuringDrawing, since otherwise we were just going to remove it immediately anyway. > > > > Sorry, I do not understands this comment. The dispatchAfterEnsuringDrawing > > call mis *inside* this lambda. > > In English this time :) > > Sorry, I do not understand this comment. The dispatchAfterEnsuringDrawing > call is *inside* this lambda. Sure, and it should be. But snapshotRemovalTracker and the if !drawingArea path both do not need to be. (In reply to Tim Horton from comment #10) > (In reply to Chris Dumez from comment #8) > > (In reply to Chris Dumez from comment #7) > > > Comment on attachment 341709 [details] > > > WIP Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=341709&action=review > > > > > > >> Source/WebKit/UIProcess/ios/ViewGestureControllerIOS.mm:299 > > > >> + m_provisionalLoadCallback = [this] { > > > > > > > > Might as well tuck this down inside just above dispatchAfterEnsuringDrawing, since otherwise we were just going to remove it immediately anyway. > > > > > > Sorry, I do not understands this comment. The dispatchAfterEnsuringDrawing > > > call mis *inside* this lambda. > > > > In English this time :) > > > > Sorry, I do not understand this comment. The dispatchAfterEnsuringDrawing > > call is *inside* this lambda. > > Sure, and it should be. But snapshotRemovalTracker and the if !drawingArea > path both do not need to be. I would still need to keep the null check inside the lambda right? In case the drawing area becomes null after the callback has been set but before the callback is called. Basically what I’m saying is... SnapshotRemovalTracker tracks “events that have to happen before you remove the snapshot”. Instead of deferring it’s creation, we should defer one of the events (specifically, the dispatchAfterEnsuringDrawing and the event it triggers) Sure, yes Comment on attachment 341767 [details] Patch Attachment 341767 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/7927235 New failing tests: imported/w3c/web-platform-tests/streams/piping/error-propagation-forward.html Created attachment 341772 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
(In reply to Tim Horton from comment #12) > Basically what I’m saying is... SnapshotRemovalTracker tracks “events that > have to happen before you remove the snapshot”. Instead of deferring it’s > creation, we should defer one of the events (specifically, the > dispatchAfterEnsuringDrawing and the event it triggers) The events we currently wait for are: SnapshotRemovalTracker::RenderTreeSizeThreshold | SnapshotRemovalTracker::RepaintAfterNavigation | SnapshotRemovalTracker::MainFrameLoad | SnapshotRemovalTracker::SubresourceLoads | SnapshotRemovalTracker::ScrollPositionRestoration As far as I can tell, we only want to wait for ALL these events if they are associated to the new page load. (In reply to Chris Dumez from comment #16) > (In reply to Tim Horton from comment #12) > > Basically what I’m saying is... SnapshotRemovalTracker tracks “events that > > have to happen before you remove the snapshot”. Instead of deferring it’s > > creation, we should defer one of the events (specifically, the > > dispatchAfterEnsuringDrawing and the event it triggers) > > The events we currently wait for are: > SnapshotRemovalTracker::RenderTreeSizeThreshold > | SnapshotRemovalTracker::RepaintAfterNavigation > | SnapshotRemovalTracker::MainFrameLoad > | SnapshotRemovalTracker::SubresourceLoads > | SnapshotRemovalTracker::ScrollPositionRestoration > > As far as I can tell, we only want to wait for ALL these events if they are > associated to the new page load. Hmmmmmm, that’s a good point! OK! Comment on attachment 341767 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341767&action=review > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2832 > +- (void)_didStartProvisionalLoadForMainFrame Are we going to get this if we’re coming from the page cache? (I hope so!) Created attachment 341774 [details]
Patch
Created attachment 341775 [details]
Patch
(In reply to Tim Horton from comment #18) > Comment on attachment 341767 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=341767&action=review > > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2832 > > +- (void)_didStartProvisionalLoadForMainFrame > > Are we going to get this if we’re coming from the page cache? (I hope so!) I am pretty sure but I will verify before landing. (In reply to Chris Dumez from comment #21) > (In reply to Tim Horton from comment #18) > > Comment on attachment 341767 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=341767&action=review > > > > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2832 > > > +- (void)_didStartProvisionalLoadForMainFrame > > > > Are we going to get this if we’re coming from the page cache? (I hope so!) > > I am pretty sure but I will verify before landing. YEs, we get the item from the page cache when committing the provisional load. We definitely send the didStartProvisional load beforehand. Comment on attachment 341775 [details] Patch Clearing flags on attachment: 341775 Committed r232416: <https://trac.webkit.org/changeset/232416> All reviewed patches have been landed. Closing bug. |