1257182 - "Restore All Tabs" can fail when there are pre-existing tabs

Status: RESOLVED WORKSFORME

(Firefox :: Session Restore, defect, P3)

Description

[Alice0775 White]

Build Identifier: 
https://hg.mozilla.org/mozilla-central/rev/5e14887312d4523ab59c3f6c6c94a679cf42b496
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0 ID:20160315030230

"Restore All Tabs" broken when there are "New Tab"(s) in  the tab strip.
There is one tab failing in restoring.

Reproducible: Always

Steps to Reproduce:
1. Open 2 tabs with valid urls
2. Open "New Tab" , then [URL1] [URL2] [New Tab] now
3. Close all Tabs except [New Tab]
4. Alt > History > Recently Closed Tabs > Restore All Tabs

Actual Results:
 "Restore All Tabs" fails.
 There is one tab failing in restoring.

Expected Results:
 All Tabs should be restored.


Regression Window:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=db5091d58b58&tochange=5d45e6b035a3

Regressed by: Bug 942374

Comment 1

[Worcester12345]

Should this be updated?
"Version: 	29 Branch"

Comment 2

[Daniel Veditz [:dveditz]]

On fx team radar but not in the next couple of releases (per dolske)

Comment 3

[Dão Gottwald [:dao]]

Does this happen only with e10s enabled?

Comment 4

[Alice0775 White]

(In reply to Dão Gottwald [:dao] from comment #3)
> Does this happen only with e10s enabled?

The problem occurs regardless of e10s on/off.

Comment 5

[Sean Lee [:seanlee][:weilonge]]

Attachment: 0001-Bug-1257182-Fix-not-all-tabs-can-be-restored-when-cl.patch

Hello Dao,

This trial patch can fix the restore all tabs issue.
If the blank tab is removed at the end of undo Tabs iteration, the feature works well.

Could you help to give this patch a feedback?
If I ask wrong person, could you help to forward this?

Thank you! :)

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8742196 [details] [diff] [review]
0001-Bug-1257182-Fix-not-all-tabs-can-be-restored-when-cl.patch

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

Ugh, my apologies, Sean - I should have assigned this to myself when I started working on it last week. I'll attach my alternate fix for you to look at, and describe why I've done it that way.

I had thought about fixing it the way you have here, but I think it only hides the underlying problem. There's a race condition when a tab is getting restored, where isTabEmpty() wrongly thinks the tab is empty. isTabempty() is used in a few places, so I think that race-condition could be hit in other ways (such as rapidly pressing Ctrl-Shift-T to undo-close multiple tabs in quick succession).

::: browser/base/content/browser.js
@@ -6284,5 @@
>    if (SessionStore.getClosedTabCount(window) > (aIndex || 0)) {
>      tab = SessionStore.undoCloseTab(window, aIndex || 0);
> -
> -    if (blankTabToRemove)
> -      gBrowser.removeTab(blankTabToRemove);

Unfortunately, removing this breaks the case where if you have one empty tab open in a window, and try to undo-close just one tab. It's meant to replace the blank tab with the re-opened tab - with this code removed, the blank tab is left there.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Alt fix, v1 (no test)

Alternate fix.

Doesn't look like there's much test coverage for undo-closing tabs, so this needs some tests written.

Comment 8

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8742617 [details] [diff] [review]
Alt fix, v1 (no test)

Hi Unfocused,

Thanks a lot for your response and patch!

I've tried this patch and it works! You are researching more than me in this bug! Just take it :)

How do you think if the function for restoreAllTabs.setAttribute("oncommand", ...); can be simplified in one function wrapper?
or is there any reason to write an iteration for oncommand?

Thanks.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8742196 [details] [diff] [review]
0001-Bug-1257182-Fix-not-all-tabs-can-be-restored-when-cl.patch

Thanks. I agree that Blair's approach is better though.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Sean Lee [:seanlee][:weilonge] from comment #8)
> take it :)

Ok, thanks :) 

> How do you think if the function for
> restoreAllTabs.setAttribute("oncommand", ...); can be simplified in one
> function wrapper?
> or is there any reason to write an iteration for oncommand?

I'd wondered about that - I'm not a fan of having anything more than a function call in an oncommand handler either. But it is consistent with how the Recently Closed Windows menu is built (in the same module, see getWindowsFragment) - which I'd prefer not to touch in this bug. It could perhaps be cleaned up as part of bug 926579.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2

Somewhat delayed thanks to a week off sick.

Added a few tests, since test coverage for undo close tab was severely lacking.

Ended up hitting a bug in BrowserTestUtils.waitForTab, which prevented waiting for multiple tabs at once.

My last Try build showed an issue on OSX which I have passing locally now, so I expect this Try build to pass fine:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b6a659cb45f

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8748579 [details] [diff] [review]
Patch v2

> function isTabEmpty(aTab) {
>   if (aTab.hasAttribute("busy"))
>     return false;
> 
>   if (aTab.hasAttribute("customizemode"))
>     return false;
> 
>+  /* Tab may be in the early stages of being restored - so even though it's
>+     currently blank, we don't want to treat it as such. */
>+  if (SessionStore.isTabRestoring(aTab))
>+    return false;

I think // is the preferred style for this kind of comment.

>+  yield BrowserTestUtils.switchTab(gBrowser, blankTab);
>+  info("Closing tabs...");
>+  yield BrowserTestUtils.removeTab(tab1);
>+  yield BrowserTestUtils.removeTab(tab2);

*sigh* See bug 1253584 comment 3. Do you expect things to break when continuing synchronously after initiating the tab switches and removals?

>+  Assert.equal(gBrowser.tabs.length, 2, "Expect 2 tabs, the blank tab having been closed");

Just use 'is'? Or has Assert.equal different semantics that make more sense here?

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Excuse me, what stops this from being landed?

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #13)
> Excuse me, what stops this from being landed?

Me, unfortunately - I've been away due to health issues. Will try to sort it out today.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Dão Gottwald [:dao] from comment #12)
> 
> >+  yield BrowserTestUtils.switchTab(gBrowser, blankTab);
> >+  info("Closing tabs...");
> >+  yield BrowserTestUtils.removeTab(tab1);
> >+  yield BrowserTestUtils.removeTab(tab2);
> 
> *sigh* See bug 1253584 comment 3. Do you expect things to break when
> continuing synchronously after initiating the tab switches and removals?

Oh, huh. That seems like a footgun given the module & naming, and I couldn't find any documentation about it. And I'd imagine a lot of other people wouldn't have the context either about when it's appropriate to use one or the other - have filed bug 1274958.

At the very least, BrowserTestUtils.removeTab() does seem to be needed for the keypress triggered tests (AFAICT, it's needed to wait for focus).



> >+  Assert.equal(gBrowser.tabs.length, 2, "Expect 2 tabs, the blank tab having been closed");
> 
> Just use 'is'? Or has Assert.equal different semantics that make more sense
> here?

It's my understanding that Assert.jsm is the way forward (some background in bug 873126), as the unified API between test suites and to be consistent with general practice outside of Mozilla.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v3

Would have landed this now, if any of the trees were open

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b3835efbf422

Comment 18

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/9e938575c485 for https://treeherder.mozilla.org/logviewer.html#?job_id=9481549&repo=fx-team and https://treeherder.mozilla.org/logviewer.html#?job_id=9481913&repo=fx-team (or https://treeherder.mozilla.org/logviewer.html#?job_id=9481196&repo=fx-team when it ducked the timeout but hit exceeded the timeout threshold).

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Blair, any news about this bug? Thanks

Updating the tracking flags to match the reality. If we had the bug in 38...

Comment 20

[Mike de Boer [:mikedeboer]]

Stealing.

Comment 21

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cc807e58553

Comment 22

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be1b0773b3d0

Comment 23

[Mike de Boer [:mikedeboer]]

Wow, I totally missed this for such a long time! I must've been distracted :-/

Alright, on it again!

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 25

[u608768]

A lot has changed in session store and I can't reproduce comment #0, so closing as WFM, but please reopen or file a new bug if this still happens.