Bug 133885 - [iOS][wk2] Swiping back briefly shows the previous page before loading the new one
Summary: [iOS][wk2] Swiping back briefly shows the previous page before loading the ne...
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
Keywords: InRadar
Depends on:
Reported: 2014-06-13 15:35 PDT by Tim Horton
Modified: 2014-06-23 03:01 PDT (History)
4 users (show)

See Also:

patch (11.35 KB, patch)
2014-06-13 16:07 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
patch (11.33 KB, patch)
2014-06-13 16:08 PDT, Tim Horton
simon.fraser: review-
Details | Formatted Diff | Diff
patch (13.39 KB, patch)
2014-06-16 16:08 PDT, Tim Horton
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
Comment 2 Tim Horton 2014-06-13 16:08:25 PDT
Created attachment 233087 [details]
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]

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; }


> 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++);

Comment 5 Tim Horton 2014-06-16 16:08:04 PDT
Created attachment 233199 [details]
Comment 6 Tim Horton 2014-06-16 16:46:23 PDT
Comment 7 Radar WebKit Bug Importer 2014-06-23 03:01:27 PDT