1789652 - Firefox View - clicking on a recently closed tab opens a different tab

Status: RESOLVED FIXED

(Firefox :: Firefox View, defect, P1)

Description

[Vicky Chin [:vchin]]

STR:

1. Click on a recently closed tab

Outcome:
Focus switches to a completely unrelated tab and the tab disappears from the Recently Closed list

Expected Results:
The expected tab shows up

106.0a1 (2022-09-06) on MacOS

Comment 1

[Vicky Chin [:vchin]]

Worth noting that the list of tabs were closed via the tree style tab add-on.

Comment 2

[Ray Fambro]

This is likely not caused not caused by the addon. I'm able to reproduce on MacOS as well.

Comment 3

[:Gijs (he/him)]

(In reply to Ray Fambro from comment #2)

This is likely not caused not caused by the addon. I'm able to reproduce on MacOS as well.

Can you provide steps to reproduce this? I cannot reproduce easily.

Comment 4

[Ray Fambro]

Interesting. I was able to reproduce again in my existing FxView window where I had 13 old tabs in the "Recently closed" section (all urls accessed within the last 2 hours). I should note that I don't believe that I refreshed this window since yesterday when we were still working through some other bug fixes.

Upon opening a new window with blank "Recently closed" history just now, I'm no longer able to reproduce this issue.

Comment 5

[:Gijs (he/him)]

I just reproduced this. Let me see if I can narrow down steps.

Comment 6

[Sarah Clements [:sclements]]

I've found a way to reproduce this reliably with a local build (ensure ./mach watch is running):
Start with a clean profile if you have a bunch of old tabs sitting around because its probably easier to see the issue, but I've found I don't need to do this every time to reproduce it.

1. open 4 tabs
2. close 3 tabs (leave 1 open, so you don't close the browser)
3. look at fxview
4. restart fxview with the mach watch shortcut (cmd+opt+r on mac)
5. click on fxview
6. click on a middle tab in the recently closed list
7. close the newly opened tab before going back to fxview
8. Go back to fxview. The newly closed tab doesn't show up.
9. Try reopening another tab and the wrong tab opens; you might also see that the url has updated for domain but doesn't match the title

I think the bug is here. We're adding tabs to this.closedTabsData by newTab.closedID but the problem is that ID is not unique and multiple entries closed around the same time will have the same ID. So in that loop, we end up adding entries to this.closedTabsData (but only if a particular closedID doesn't exist) and in that same loop overwriting the urls of existing entries we've just added with erroneous tabsToUpdate.

Comment 7

[:Gijs (he/him)]

(In reply to Sarah Clements [:sclements] from comment #6)

I think the bug is here. We're adding tabs to this.closedTabsData by newTab.closedID but the problem is that ID is not unique and multiple entries closed around the same time will have the same ID.

Uh, that sounds bad. https://searchfox.org/mozilla-central/rev/275630dfceb88eee07e4c7d38cd021dd39c2ab23/browser/components/sessionstore/SessionStore.jsm#2802 looks like it's monotonically increasing. But perhaps when using session restore and getting back tabs from the previous session, the IDs can conflict?

Comment 8

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #7)

(In reply to Sarah Clements [:sclements] from comment #6)

I think the bug is here. We're adding tabs to this.closedTabsData by newTab.closedID but the problem is that ID is not unique and multiple entries closed around the same time will have the same ID.

Uh, that sounds bad. https://searchfox.org/mozilla-central/rev/275630dfceb88eee07e4c7d38cd021dd39c2ab23/browser/components/sessionstore/SessionStore.jsm#2802 looks like it's monotonically increasing. But perhaps when using session restore and getting back tabs from the previous session, the IDs can conflict?

Yeah, looks like we just restart counting at 0. But the closed tabs from the previous session can also include items with the same closedId...

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1787565

Comment 10

[Sarah Clements [:sclements]]

Attachment: Bug 1789652 - Overwrite closedIDs when restoring sessions r=Gijs

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d057e8cf08cf
Overwrite closedIDs when restoring sessions r=Gijs

Comment 12

[Marian-Vasile Laza]

Backed out changeset d057e8cf08cf (Bug 1789652) for causing bc failures on browser_closedId.js.
Backout link
Push with failures <--> bc2
Failure Log

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a17392e12c8
Overwrite closedIDs when restoring sessions r=Gijs

Comment 14

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/4a17392e12c8