Duplicated tab shares live back/forwards history list with original, rather than a copy

RESOLVED FIXED in Firefox 53

Status

()

Core
Document Navigation
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: Alex Vallat, Assigned: mystor)

Tracking

({regression})

55 Branch
mozilla55
regression
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 unaffected, firefox53+ fixed, firefox54+ fixed, firefox55+ fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

11 months ago
str
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170306004003

Steps to reproduce:

Navigate to page A.
Navigate to page B.
Middle click the Refresh button to duplicate the tab.
In the new tab, navigate to page C.
In the original tab, right click the Back button to show the history list


Actual results:

The history list of the original tab shows page C (above the current page B)

Further, if you then navigate to page D (still in the original tab), switch back to the new (duplicate) tab and right click the Back button, you can see that it shows the current entry as D, despite the tab still showing page C


Expected results:

The duplicate tab should have a copy of the history, not a live share of it. Navigation in the new tab should not affect the back/forwards history list of the original tab, and navigation in the original tab should not affect the new one.

Comment 1

11 months ago
regression-window

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a524931c115bd49b76786d29e9231be458d95d78&tochange=84e635f18d703b7e359df0b38de10b892a852718

regressed by bug 1310771.
Blocks: 1310771
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox52: --- → unaffected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
tracking-firefox53: --- → ?
tracking-firefox54: --- → ?
tracking-firefox55: --- → ?
Component: Untriaged → Document Navigation
Ever confirmed: true
Flags: needinfo?(michael)
Keywords: regression
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → All
(Reporter)

Comment 2

11 months ago
I would recommend taking a look at https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/TabState.jsm#182

TabStateInternal.copyFromCache should probably do:

tabData.entries = value.entries.concat();

(or similar array shallow clone) instead of

tabData.entries = value.entries;
(Assignee)

Comment 3

11 months ago
Created attachment 8845656 [details] [diff] [review]
Clone TabData entries when restoring tabs

Before my GroupedSHistory patch, after restoring tabs, the newly restored tab would `collect()`, sending down a complete version of its history. This history would then replace the existing history object, meaning that the entries field was not shared.

After that patch, `collect()` now does a "partial" update, meaning that it changes the entries object in-place. This was to account for the fact that a child tab may not know its full history anymore.

This means that the bug where two tabs could share a entries object became visible.

I have added clones of the entries object in 2 different places where I think it makes sense, which should help avoid this mistake in the future I think :).

MozReview-Commit-ID: BhAbhSHgXWt
Attachment #8845656 - Flags: review?(mdeboer)
(Assignee)

Updated

11 months ago
Flags: needinfo?(michael)
(Assignee)

Updated

11 months ago
Assignee: nobody → michael
Comment on attachment 8845656 [details] [diff] [review]
Clone TabData entries when restoring tabs

Review of attachment 8845656 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the added test!

::: browser/components/sessionstore/SessionStore.jsm
@@ +3571,5 @@
>      // Update the persistent tab state cache with |tabData| information.
>      TabStateCache.update(browser, {
> +      // NOTE: Copy the entries array shallowly, so as to not screw with the
> +      // original tabData's history when getting history updates.
> +      history: {entries: tabData.entries.concat(), index: tabData.index},

Please use `[...tabData.entries]`

::: browser/components/sessionstore/TabState.jsm
@@ +181,5 @@
>        if (key === "history") {
> +        // Make a shallow copy of the entries array. We (currently) don't update
> +        // entries in place, so we don't have to worry about performing a deep
> +        // copy.
> +        tabData.entries = value.entries.concat();

Same here.
Attachment #8845656 - Flags: review?(mdeboer) → review+
(Assignee)

Comment 5

11 months ago
Created attachment 8846723 [details] [diff] [review]
Clone TabData entries when restoring tabs

MozReview-Commit-ID: BhAbhSHgXWt
(Assignee)

Updated

11 months ago
Attachment #8845656 - Attachment is obsolete: true

Comment 6

11 months ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e4c135cb424
Clone TabData entries when restoring tabs, r=mikedeboer

Comment 8

11 months ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8070655e440
Clone TabData entries when restoring tabs, r=mikedeboer

Comment 9

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e8070655e440
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Tracking 53/54/55+. Once this settles on Nightly we should probably nominate for 53.
tracking-firefox53: ? → +
tracking-firefox54: ? → +
tracking-firefox55: ? → +
(Assignee)

Comment 11

11 months ago
Created attachment 8847168 [details] [diff] [review]
Clone TabData entries when restoring tabs

There was a typo in the previous patch which I corrected in this one.

MozReview-Commit-ID: BhAbhSHgXWt
(Assignee)

Updated

11 months ago
Attachment #8846723 - Attachment is obsolete: true
(Assignee)

Comment 12

11 months ago
Comment on attachment 8847168 [details] [diff] [review]
Clone TabData entries when restoring tabs

Approval Request Comment
[Feature/Bug causing the regression]: bug 1310771
[User impact if declined]: Duplicated tabs will share history stored in SessionStore.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only makes a copy of the entries array to prevent the object from being shared. 
[String changes made/needed]: None
Flags: needinfo?(michael)
Attachment #8847168 - Flags: approval-mozilla-beta?
Attachment #8847168 - Flags: approval-mozilla-aurora?

Comment 13

11 months ago
Comment on attachment 8847168 [details] [diff] [review]
Clone TabData entries when restoring tabs

Fix a regression that duplicated tabs will share history stored in SessionStore and also include tests. Beta53+ & Aurora54+.
Attachment #8847168 - Flags: approval-mozilla-beta?
Attachment #8847168 - Flags: approval-mozilla-beta+
Attachment #8847168 - Flags: approval-mozilla-aurora?
Attachment #8847168 - Flags: approval-mozilla-aurora+

Comment 14

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/cbe7d83a336d
status-firefox54: affected → fixed
Flags: in-testsuite+

Comment 15

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/dac80a77222c
status-firefox53: affected → fixed
status-firefox-esr52: --- → unaffected
Setting qe-verify- based on Michael's assessment on manual testing needs (see Comment 12) and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.