Closed Bug 1554512 Opened 5 months ago Closed 5 months ago

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

Categories

(Firefox :: Session Restore, defect, P1)

68 Branch
Desktop
Windows 10
defect

Tracking

()

VERIFIED FIXED
Firefox 69
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- unaffected
firefox67.0.1 --- unaffected
firefox68 --- fixed
firefox69 --- verified

People

(Reporter: alice0775, Assigned: alchen)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(1 file)

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?

Flags: needinfo?(alchen)
Has Regression Range: --- → yes
Has STR: --- → yes
Keywords: regression
Regressed by: 1474130

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

Summary: Scroll position of duplicated tab lost if the page was scrolling to the top. → Scroll position of duplicated tab/Undo closed tab lost if the page was scrolling to the top.

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

Assignee: nobody → alchen
Status: NEW → ASSIGNED
Flags: needinfo?(alchen)
QA Whiteboard: [qa-regression-triage]

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.

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".

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

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.

(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().

Flags: needinfo?(mdeboer)
Flags: needinfo?(mdeboer)

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.)

Flags: needinfo?(alchen)
Priority: -- → P1

(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.

Flags: needinfo?(alchen)
Keywords: checkin-needed

Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/03dde2acc06f
Add a handing for scroll position in copyFromCache() r=mikedeboer

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69
QA Whiteboard: [qa-regression-triage]

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

Flags: needinfo?(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:
Flags: needinfo?(alchen)
Attachment #9067963 - Flags: approval-mozilla-beta?

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

session restore fix, approved for 68.0b8

Attachment #9067963 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [good first verify]
Blocks: 1564412

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.

Thank you, Sergiu!

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.