Bug 133885

Summary: [iOS][wk2] Swiping back briefly shows the previous page before loading the new one
Product: WebKit Reporter: Tim Horton <thorton>
Component: WebKit2Assignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, benjamin, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
simon.fraser: review-
patch simon.fraser: review+

Description Tim Horton 2014-06-13 15:35:29 PDT
Sometimes we will get a layer tree commit from the Web Process -> UI process with layers (and a render tree size) from the previous page AFTER the UI process has started the navigation to the new page (because layer tree flush is very asynchronous). This will cause ViewGestureControllerIOS to think that the render tree size limit has been hit, and remove the snapshot, revealing the old page.

Since we already synchronize layer tree commits between processes, we just need a way to identify commits by ID and know which commit current actions will be applied to.
Comment 1 Tim Horton 2014-06-13 16:07:56 PDT
Created attachment 233086 [details]
patch
Comment 2 Tim Horton 2014-06-13 16:08:25 PDT
Created attachment 233087 [details]
patch
Comment 3 Tim Horton 2014-06-13 16:08:54 PDT
Transaction ID is reset upon crash because we tear down the drawing area (and make a new Web process). I've tested that this works!
Comment 4 Simon Fraser (smfr) 2014-06-16 14:31:28 PDT
Comment on attachment 233087 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233087&action=review

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.h:209
> +    void setTransactionID(uint64_t transactionID) { m_transactionID = transactionID; }

Should this be provided to the constructor?

> Source/WebKit2/UIProcess/DrawingAreaProxy.h:89
> +    virtual uint64_t nextTransactionID() const { ASSERT_NOT_REACHED(); return 0; }

Weird to have this one.

> Source/WebKit2/UIProcess/ios/ViewGestureControllerIOS.mm:231
> +    m_targetTransactionID = m_webPageProxy.drawingArea()->nextTransactionID();

"target" doesn't communicate what you're going to use this transaction ID for.

> Source/WebKit2/UIProcess/ios/ViewGestureControllerIOS.mm:253
> +    if (m_targetRenderTreeSize && renderTreeSize > m_targetRenderTreeSize && m_webPageProxy.drawingArea()->currentTransactionID() >= m_targetTransactionID)

I want to see a call here like "isCurrentTransaction()" or "isStaleTransaction" or something.

> Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.h:80
> +    virtual uint64_t nextTransactionID() const override { return m_currentTransactionID + 1; }

Remove?

> Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.h:94
> +    uint64_t m_currentTransactionID;

I feel like this should be lastVisibleTransactionID or something.

> Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:312
> +    m_currentTransactionID++;

Rather than just increment this count, I feel like it should copy the ID obtained from the last transaction somehow.

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:282
> +    layerTransaction.setTransactionID(m_currentTransactionID++);

getNextTransactionID()?
Comment 5 Tim Horton 2014-06-16 16:08:04 PDT
Created attachment 233199 [details]
patch
Comment 6 Tim Horton 2014-06-16 16:46:23 PDT
http://trac.webkit.org/changeset/170035
Comment 7 Radar WebKit Bug Importer 2014-06-23 03:01:27 PDT
<rdar://problem/17416499>