1449956 - |Undo Close Tab| or |Restore All Tabs| fails if |New Tab| entry is in Closed tab list

Status: NEW

(Firefox :: Session Restore, defect, P3)

Description

[Alice0775 White]

Reproducible: always:


Steps To Reproduce:
1. Open tabs like [New Tab][New Tab][Web page1][Web page2][...]
2. Right click on 1st tab and |Close Other Tabs|
3. Try |Undo Close Tab| from tab context menu and repeat step 3 if any
    --- observe
4. Try |Restore All Tabs| from Alt > History > Recent Closed Tabs
    --- observe

Actual Results:
Unable to restore [Web page1], [Web page2], [...]

Expected Results:
The command should work as expected.

Comment 1

[Alice0775 White]

V59 and 60 are Affected .
61 is also affected, but it is slightly strange behavior.

Comment 2

[Alex Thayer [:alexical] (she/her)]

What I'm seeing here is the "Restore All Tabs" implementation just calls undoCloseTab in a loop, which removes the New Tab page if it's the only tab open before we start our restore. What this means is that if our tabs look like this:

[New Tab] | Tab A | Tab B

and we close Tab A and Tab B, then restore all, we

1. Restore Tab A
  - Remove [New Tab]
2. Restore [New Tab]

Which uses up our two iterations of the loop, leaving Tab B unrestored. If your tab list looks like

[New Tab 1] | [New Tab 2] | Tab A | Tab B | Tab C | ... | Tab N

Then we just get stuck restoring New Tab 1 then New Tab 2 then New Tab 1, etc.

Dao, let me know if P3 seems reasonable for things like this. It's low impact but should be trivial to fix.

Comment 3

[Dão Gottwald [:dao]]

Sounds about right except that this seems like a session restore bug. Empty tabs shouldn't end up in the closed tabs list:

https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/browser/components/sessionstore/SessionStore.jsm#4530

Comment 4

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #3)
> Empty tabs shouldn't end up in the closed tabs list:
> 
> https://searchfox.org/mozilla-central/rev/
> 2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/browser/components/sessionstore/
> SessionStore.jsm#4530

Hmm, onTabClose doesn't even consider _shouldSaveTabState when adding to _closedTabs.

Comment 5

[Rares Doghi, Desktop QA]

Hi, i managed to reproduce this issue from the latest nightly build all the way down to 56.0 (64-bit), after closing the 2 websites and the empty new tab when i click restore tabs from the recently closed tabs menu, it will launch a single website instead of restoring all tabs. Not sure how to start mozregression since it occurs in all versions. 
Should i go lower than 56 ?

Comment 6

[Dão Gottwald [:dao]]

(In reply to Rares Doghi from comment #5)
> Hi, i managed to reproduce this issue from the latest nightly build all the
> way down to 56.0 (64-bit), after closing the 2 websites and the empty new
> tab when i click restore tabs from the recently closed tabs menu, it will
> launch a single website instead of restoring all tabs. Not sure how to start
> mozregression since it occurs in all versions. 
> Should i go lower than 56 ?

Yes, 56 isn't that old.

Comment 7

[laszlo.bialis]

The issue is present on Nightly 66.0a1 (20190107214730) also, reproduced with the above steps on Win8 x32.

Comment 8

[Timea Cernea [:tbabos][inactive]]

Reproduced on latest Nightly 67.0a1 (2019-02-04) (32-bit) on Win 8 x32.

Comment 9

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

The severity field for this bug is relatively low, S3. However, the bug has 10 votes.
:dao, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 10

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.