Bug 186168 - Regression(r230876): Swipe navigation snapshot may get removed too early
Summary: Regression(r230876): Swipe navigation snapshot may get removed too early
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-31 16:48 PDT by Chris Dumez
Modified: 2018-06-01 13:58 PDT (History)
6 users (show)

See Also:


Attachments
WIP Patch (7.86 KB, patch)
2018-05-31 16:53 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (9.11 KB, patch)
2018-05-31 17:00 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-sierra-wk2 (3.03 MB, application/zip)
2018-05-31 18:21 PDT, EWS Watchlist
no flags Details
Patch (12.25 KB, patch)
2018-06-01 10:13 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.24 MB, application/zip)
2018-06-01 11:02 PDT, EWS Watchlist
no flags Details
Patch (12.30 KB, patch)
2018-06-01 11:51 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (12.29 KB, patch)
2018-06-01 11:52 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.