1228518 - [e10s] Scroll position is lost after restoring session

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Alice0775 White]

[Tracking Requested - why for this release]:

Build Identifier:
https://hg.mozilla.org/mozilla-central/rev/c321d84038519dcf1670d59fd2c5c00ad8a85a55
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0 ID:20151126030226

The problem is easily reproduce.

Steps to reproduce:
1. Open many tab and scroll
   e.g, Open three tabs and scroll each tab to the bottom.
   [Tab 1]  https://wiki.mozilla.org/L10n:Contribute
   [Tab 2]  https://wiki.mozilla.org/L10n:Home_Page
   [Tab 3 and selected]  https://www.mozilla.org/en-US/firefox/nightly/firstrun/ 
2. Close browser
3. Restart Browser and execute "Restore Previous Session"
4. Select [Tab 2]

Actual Results:
Scroll position is lost

Expected Results:
Scroll position should persist


Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=db77a24c37698b4e71f8ba0fba170a0c157fc1f9&tochange=8b1fc0961a07

https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=a0db720c980e3fbacf92c03566e8ffea5fdefd2d&tochange=924d421d766a

Regressed by: Bug 1209689

Comment 1

[Alice0775 White]

via local build,
Last Good: 2465c9539de1
First Bad: 243353ae9c08

Triggered by: 243353ae9c08	Mike Conley — Bug 1209689 - Tabs that haven't yet been restored should not crash. r=Mossop

Comment 2

[Mike Conley (:mconley) (:⚙️)]

I can reproduce this without a full browser restart or session restore:

STR:

1) Start with a window A. Open the same links from comment 0. Shrink your window if you need to force them to scroll.
2) Open a new window B.
3) Scroll each tab to the bottom
4) Select the third tab, and then close window A.
3) Go to History > Restore Closed Windows, and restore window A
4) Once the window respawns and the third tab has finished loading, select the second tab to restore it on demand

ER:

The tab should scroll to the bottom

AR:

The tab does not scroll to the bottom. Strangely, the first tab, once selected, will.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1228518 - Don't send SessionStore:update for initial about:blank page load. r?ttaubert

Bug 1228518 - Don't send SessionStore:update for initial about:blank page load. r?ttaubert

Comment 4

[Mike Conley (:mconley) (:⚙️)]

So here's my understanding of what's going on here:

1) Background tabs are created, and history / scroll positions are sent down.
2) Once the initial about:blank has loaded for those unrestored background tabs, they trigger an update message to be queued via the FrameTreeInternal's onStateChange method, since we see the about:blank start and stop loading.
3) Inside the onFrameTreeReset handler for ScrollPositionListener, we queue up a collection for the null value
4) We eventually send an update to the parent, which updates the TabStateCache for that browser with the now null scroll position

Here's where e10s and non-e10s diverge.

With non-e10s, when we do a restore, the tabData inside ContentRestore is used to set the scroll position again to the initial value, so we correctly scroll the user to where they were before.

For the e10s case, we switch remoteness on content restore, so that causes us to read the TabStateCache for that browser (which now has the null scroll position, thus clearing the scroll position data in the cache), flip the remoteness of the browser, and then send down the message to the child without the scroll position.

This isn't just true for scroll position - we appear to do the same thing for at least pagestyle and storage as well.

I think the best solution might just be to not send updates for that initial about:blank load, which my latest patch does. It seems to work. I'm writing a regression test now.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1228518 - Regression test. r?ttaubert

Bug 1228518 - Regression test. r?ttaubert

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8693084 [details]
MozReview Request: Bug 1228518 - Don't send SessionStore:update for initial about:blank page load. r?ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26395/diff/1-2/

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8693111 [details]
MozReview Request: Bug 1228518 - Regression test. r?ttaubert

https://reviewboard.mozilla.org/r/26397/#review23949

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8693084 [details]
MozReview Request: Bug 1228518 - Don't send SessionStore:update for initial about:blank page load. r?ttaubert

https://reviewboard.mozilla.org/r/26395/#review23947

Seems like the right thing to do.

::: browser/components/sessionstore/FrameTree.jsm:222
(Diff revision 2)
> +    // onStateChangewill be fired when loading the initial about:blank URI for

nit: missing space

Comment 9

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/integration/fx-team/rev/ea5faab36ba798635f99791e186d90c380997806
Bug 1228518 - Regression test. r=ttaubert

https://hg.mozilla.org/integration/fx-team/rev/3b8b9e4abbaadf1ccd61cb51c8578e36f9d3e004
Bug 1228518 - Don't send SessionStore:update for initial about:blank page load. r=ttaubert

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea5faab36ba7
https://hg.mozilla.org/mozilla-central/rev/3b8b9e4abbaa

Comment 11

[mahmoudi dris]

status-firefox45: fixed → ferified

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Dris, you mean that you confirmed that this bug is actually fixed? Thanks

Comment 13

[mahmoudi dris]

(In reply to Sylvestre Ledru [:sylvestre] from comment #12)
> Dris, you mean that you confirmed that this bug is actually fixed? Thanks

yes i confirmed that this bug is fixed

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1228518.html