1554512 - Scroll position of duplicated tab/Undo closed tab lost if the page was scrolling to the top.

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect, P1)

Description

[Alice0775 White]

Reproducible: always

Steps To Reproduce:

1. Open long page (e.g https://en.wikipedia.org/wiki/Main_Page)
2. Scroll the page
3. Duplicate the tab (middle click on reload button)
   --- observe, the scroll position of the duplicated tab is preserved as expected.
4. Scroll to top on the duplicated page
5. Duplicate the tab (middle click on reload button)
   --- observe bug

Actual Results:
Scroll position of duplicated tab lost.
The scroll position is the one that the previous scroll position is not top.

Expected Results:
The scroll position should be top.
The scroll position of the duplicated tab should be preserved even if the scroll position is top

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4bb1186d2d9e4bfa6c0238fab94b3d60e72aee55&tochange=3288c43195a26565edec3d59a8f8bce6018be592

Regressed by: 3288c43195a26565edec3d59a8f8bce6018be592 Alphan Chen — Bug 1474130 - Implement ScrollPosition/Privacy/DocCapability listeners in C++ r=peterv

Alphan,
Your patch Seems to cause the regression. Can you please look into this?

Comment 1

[Alice0775 White]

Other Steps To Reproduce:

1. Open long page (e.g https://en.wikipedia.org/wiki/Main_Page)
2. Scroll the page
3. Close the tab
4. Undo Closed Tab
   ---Observe, the scroll position of the undo close tab is preserved as expected.
5. Scroll to top
6. Close the tab
7. Undo Closed Tab
   --- observe bug, the scroll position of undo closed tab lost.

Actual Results:
Scroll position of the undo close tab lost.
The scroll position is the one that the previous scroll position is not top.

Expected Results:
The scroll position should be top.
The scroll position should be preserved even if the scroll position was top

Comment 2

[Alphan Chen [:alchen]]

I can reproduce it on my local side.
Will figure it out today.

Comment 3

[Alphan Chen [:alchen]]

Update current status:
First, I am still trying to figure out the reason.

Update what I found:
The symptom only happened when the page is scrolling to the top.
The scrollPosition of the top is "0,0".

In the original design, we will store the top position("0,0") by using "null".
If we replace the "null" with "0,0", the symptom disappears.
It can fix this symptom. However, we need the corresponding change for testcase as well.

Comment 4

[Alphan Chen [:alchen]]

I think I found the root cause.

When doing duplicateTab(), we will trigger a TabStateFlush and restore the tab using the latest tabState.

However, we will get a TabState before the TabStateFlush.
https://searchfox.org/mozilla-central/rev/ddb81c7a43ffada1f6cb4200c4f625e50e44dcf3/browser/components/sessionstore/SessionStore.jsm#2649
After the TabStateFlush, we will use TabState.copyFromCache() to update the TabState.
https://searchfox.org/mozilla-central/rev/ddb81c7a43ffada1f6cb4200c4f625e50e44dcf3/browser/components/sessionstore/SessionStore.jsm#2670

In the behavior of this bug, there is scroll position(key="scroll") in the TabState before TabStateFlush. (e.g. 0,100)
When doing the TabStateFlush, we delete key "scroll" due to it is null.(top position)
https://searchfox.org/mozilla-central/source/browser/components/sessionstore/TabStateCache.jsm#165
Due to the implentation of TabState.copyFromCache(), we cannot update the scroll position to null.
https://searchfox.org/mozilla-central/rev/ddb81c7a43ffada1f6cb4200c4f625e50e44dcf3/browser/components/sessionstore/TabState.jsm#153

It makes sense that the symptom will disappear if we change the storing value of top scroll position from null to "0,0".

Comment 5

[Alphan Chen [:alchen]]

Attachment: Bug 1554512 - Add a handing for scroll position in copyFromCache()

Need to update the scroll position if it is deleted in the TabStateCachae.

Comment 6

[Alphan Chen [:alchen]]

The reason why we won't meet this problem before:
Without my patch, the scrollposition is not deleted after the TabStateFlush().
My patch just changes the sequence and hits the bug.

Comment 7

[Alphan Chen [:alchen]]

(In reply to Alphan Chen [:alchen] from comment #6)

The reason why we won't meet this problem before:
Without my patch, the scrollposition is not deleted after the TabStateFlush().
My patch just changes the behavior and hit the bug.

It is deleted "before" the TabStateFlush().

Comment 8

[Mike de Boer [:mikedeboer]]

Alphan, is your patch ready for review? If so, can you assign blocking reviewers to it? If you don't do that, we're bound to miss the review request!
(Tip: you can automatically assign blocking reviewers by adding an '!' to the reviewers nick in your commit message.)

Comment 9

[Alphan Chen [:alchen]]

(In reply to Mike de Boer [:mikedeboer] from comment #8)

Alphan, is your patch ready for review? If so, can you assign blocking reviewers to it? If you don't do that, we're bound to miss the review request!
(Tip: you can automatically assign blocking reviewers by adding an '!' to the reviewers nick in your commit message.)

Yes, the current patch is a fix for losing scroll position.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/03dde2acc06f
Add a handing for scroll position in copyFromCache() r=mikedeboer

Comment 11

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/03dde2acc06f

Comment 12

[Julien Cristau [:jcristau]]

Please request uplift to beta when you're comfortable doing so.

Comment 13

[Alphan Chen [:alchen]]

Comment on attachment 9067963 [details]
Bug 1554512 - Add a handing for scroll position in copyFromCache()

Beta/Release Uplift Approval Request

• User impact if declined: Will lose scrolling position after specific steps as bug describes
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Only add handling for scrolling position for a specific condition in TabState.jsm:copyFromCache()
• String changes made/needed:

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 9067963 [details]
Bug 1554512 - Add a handing for scroll position in copyFromCache()

session restore fix, approved for 68.0b8

Comment 15

[Oana Pop-Rus]

https://hg.mozilla.org/releases/mozilla-beta/rev/023ce53e3a94

Comment 16

[Negritas Sergiu, Desktop QA]

Following the STR from the description, I reproduced the issue using Fx 68.0b9 (2019-05-26) on Windows 10. I can confirm this issue is fixed , Iverified using Fx 69.0b13 on the same environment.

Comment 17

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

Thank you, Sergiu!