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

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Alex Vallat]

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

[Loic]

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

regressed by bug 1310771.

Comment 2

[Alex Vallat]

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;

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: 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

Comment 4

[Mike de Boer [:mikedeboer]]

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.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Clone TabData entries when restoring tabs

MozReview-Commit-ID: BhAbhSHgXWt

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e4c135cb424
Clone TabData entries when restoring tabs, r=mikedeboer

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing at least marionette (test_refresh_firefox.py TestFirefoxRefresh.testReset), functional-ui and browser-chrome:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a105bbb7e4253d81238557ae44351dfbc6dbdfe

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5e4c135cb424eaf4bf3b03956c335593d5c51354&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

The next push ran more browser-chrome test (which fail often): https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=84b7c2585ab88002ed45368ae28b9b55035c85f5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Failure log example: https://treeherder.mozilla.org/logviewer.html#?job_id=83435389&repo=mozilla-inbound
> test_refresh_firefox.py TestFirefoxRefresh.testReset | AssertionError: Sequences differ: [u'about:blank'] != ['about:welcomeback']

Please check also the other failures.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8070655e440
Clone TabData entries when restoring tabs, r=mikedeboer

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e8070655e440

Comment 10

[Marcia Knous [:marcia]]

Tracking 53/54/55+. Once this settles on Nightly we should probably nominate for 53.

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Attachment: Clone TabData entries when restoring tabs

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

MozReview-Commit-ID: BhAbhSHgXWt

Comment 12

[Nika Layzell [:nika] (ni? for response)]

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

Comment 13

[Gerry Chang [:gchang]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cbe7d83a336d

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dac80a77222c

Comment 16

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Michael's assessment on manual testing needs (see Comment 12) and the fact that this fix has automated coverage.