[e10s] Scroll position is lost after restoring session

VERIFIED FIXED in Firefox 45

Status

()

VERIFIED FIXED
3 years ago
a year ago

People

(Reporter: alice0775, Assigned: mconley)

Tracking

({dataloss, regression})

45 Branch
Firefox 45
dataloss, regression
Points:
---

Firefox Tracking Flags

(e10s?, firefox44 unaffected, firefox45+ verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
[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
Flags: needinfo?(mconley)
(Reporter)

Updated

3 years ago
tracking-e10s: --- → ?
Summary: Scroll position is lost after restoring session → [e10s] Scroll position is lost after restoring session
(Reporter)

Comment 1

3 years ago
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
(Assignee)

Comment 2

3 years ago
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.
Assignee: nobody → mconley
Flags: needinfo?(mconley)
(Assignee)

Comment 3

3 years ago
Created attachment 8693084 [details]
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
Attachment #8693084 - Flags: review?(ttaubert)
(Assignee)

Comment 4

3 years ago
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.
(Assignee)

Comment 5

3 years ago
Created attachment 8693111 [details]
MozReview Request: Bug 1228518 - Regression test. r?ttaubert

Bug 1228518 - Regression test. r?ttaubert
Attachment #8693111 - Flags: review?(ttaubert)
(Assignee)

Comment 6

3 years ago
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 on attachment 8693111 [details]
MozReview Request: Bug 1228518 - Regression test. r?ttaubert

https://reviewboard.mozilla.org/r/26397/#review23949
Attachment #8693111 - Flags: review?(ttaubert) → review+
Attachment #8693084 - Flags: review?(ttaubert) → review+
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 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ea5faab36ba7
https://hg.mozilla.org/mozilla-central/rev/3b8b9e4abbaa
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45

Updated

3 years ago
QA Whiteboard: [good first verify]
tracking-firefox45: ? → +

Comment 11

3 years ago
status-firefox45: fixed → ferified
Dris, you mean that you confirmed that this bug is actually fixed? Thanks
Flags: needinfo?(dris.mahmoudi)

Comment 13

3 years ago
(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
Flags: needinfo?(dris.mahmoudi)
status-firefox45: fixed → verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.