Bug 186168 – Regression(r230876): Swipe navigation snapshot may get removed too early

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Swipe navigation snapshot may get removed too early after r230876, causing the view to flickr.

[Chris Dumez]

<rdar://problem/39743617>

[Chris Dumez]

Created attachment 341708 [details]
WIP Patch

[Chris Dumez]

Created attachment 341709 [details]
WIP Patch

[Tim Horton]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Chris Dumez]

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.

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 341767 [details]
Patch

[Tim Horton]

(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.

[Chris Dumez]

(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.

[Tim Horton]

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)

[Tim Horton]

Sure, yes

[EWS Watchlist]

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

[EWS Watchlist]

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

[Chris Dumez]

(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.

[Tim Horton]

(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!

[Tim Horton]

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!)

[Chris Dumez]

Created attachment 341774 [details]
Patch

[Chris Dumez]

Created attachment 341775 [details]
Patch

[Chris Dumez]

(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.

[Chris Dumez]

(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.

[WebKit Commit Bot]

Comment on attachment 341775 [details]
Patch

Clearing flags on attachment: 341775

Committed r232416: <https://trac.webkit.org/changeset/232416>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.