Closed Bug 1345857 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Navigation, defect)

55 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 + fixed
firefox54 + fixed
firefox55 + fixed

People

(Reporter: bugzilla, Assigned: nika)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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.

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
Component: Untriaged → Document Navigation
Ever confirmed: true
Flags: needinfo?(michael)
Keywords: regression
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → All
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;
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)
Flags: needinfo?(michael)
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+
MozReview-Commit-ID: BhAbhSHgXWt
Attachment #8845656 - Attachment is obsolete: true
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e4c135cb424
Clone TabData entries when restoring tabs, r=mikedeboer
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8070655e440
Clone TabData entries when restoring tabs, r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/e8070655e440
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Tracking 53/54/55+. Once this settles on Nightly we should probably nominate for 53.
There was a typo in the previous patch which I corrected in this one.

MozReview-Commit-ID: BhAbhSHgXWt
Attachment #8846723 - Attachment is obsolete: true
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 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+
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.