Bug 186168

Summary: Regression(r230876): Swipe navigation snapshot may get removed too early
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: 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 Flags
WIP Patch
none
WIP Patch
none
Archive of layout-test-results from ews105 for mac-sierra-wk2
none
Patch
none
Archive of layout-test-results from ews106 for mac-sierra-wk2
none
Patch
none
Patch none

Description Chris Dumez 2018-05-31 16:48:58 PDT
Swipe navigation snapshot may get removed too early after r230876, causing the view to flickr.
Comment 1 Chris Dumez 2018-05-31 16:49:14 PDT
<rdar://problem/39743617>
Comment 2 Chris Dumez 2018-05-31 16:53:54 PDT
Created attachment 341708 [details]
WIP Patch
Comment 3 Chris Dumez 2018-05-31 17:00:54 PDT
Created attachment 341709 [details]
WIP Patch
Comment 4 Tim Horton 2018-05-31 17:18:22 PDT
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 5 EWS Watchlist 2018-05-31 18:21:46 PDT
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
Comment 6 EWS Watchlist 2018-05-31 18:21:47 PDT
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 7 Chris Dumez 2018-06-01 07:48:24 PDT
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.
Comment 8 Chris Dumez 2018-06-01 09:45:18 PDT
(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.
Comment 9 Chris Dumez 2018-06-01 10:13:10 PDT
Created attachment 341767 [details]
Patch
Comment 10 Tim Horton 2018-06-01 10:38:08 PDT
(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.
Comment 11 Chris Dumez 2018-06-01 10:43:48 PDT
(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.
Comment 12 Tim Horton 2018-06-01 10:48:40 PDT
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)
Comment 13 Tim Horton 2018-06-01 10:48:52 PDT
Sure, yes
Comment 14 EWS Watchlist 2018-06-01 11:02:13 PDT
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
Comment 15 EWS Watchlist 2018-06-01 11:02:15 PDT
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
Comment 16 Chris Dumez 2018-06-01 11:02:56 PDT
(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.
Comment 17 Tim Horton 2018-06-01 11:43:40 PDT
(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 18 Tim Horton 2018-06-01 11:44:59 PDT
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!)
Comment 19 Chris Dumez 2018-06-01 11:51:15 PDT
Created attachment 341774 [details]
Patch
Comment 20 Chris Dumez 2018-06-01 11:52:33 PDT
Created attachment 341775 [details]
Patch
Comment 21 Chris Dumez 2018-06-01 11:52:54 PDT
(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.
Comment 22 Chris Dumez 2018-06-01 11:55:10 PDT
(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 23 WebKit Commit Bot 2018-06-01 13:58:34 PDT
Comment on attachment 341775 [details]
Patch

Clearing flags on attachment: 341775

Committed r232416: <https://trac.webkit.org/changeset/232416>
Comment 24 WebKit Commit Bot 2018-06-01 13:58:35 PDT
All reviewed patches have been landed.  Closing bug.