Open Bug 1257182 Opened 4 years ago Updated 11 months ago

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

Categories

(Firefox :: Session Restore, defect, P3)

29 Branch
Unspecified
Windows 7
defect
Points:
5

Tracking

()

ASSIGNED
Iteration:
50.4 - Aug 1
Tracking Status
firefox45 --- wontfix
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- fix-optional
firefox-esr38 --- wontfix
firefox-esr45 --- wontfix
firefox50 --- fix-optional

People

(Reporter: alice0775, Assigned: beachjar)

References

(Blocks 2 open bugs)

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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
Blocks: fx-qx
Summary: "Restore All Tabs" broken when there are "New Tab" in the tab strip. → "Restore All Tabs" can fail when there are pre-existing tabs
Should this be updated?
"Version: 	29 Branch"
On fx team radar but not in the next couple of releases (per dolske)
Does this happen only with e10s enabled?
Flags: needinfo?(alice0775)
(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.
Flags: needinfo?(alice0775)
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! :)
Attachment #8742196 - Flags: feedback?(dao+bmo)
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.
Attached patch Alt fix, v1 (no test) (obsolete) — Splinter Review
Alternate fix.

Doesn't look like there's much test coverage for undo-closing tabs, so this needs some tests written.
Attachment #8742617 - Flags: feedback?(selee)
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.
Attachment #8742617 - Flags: feedback?(selee) → feedback+
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.
Attachment #8742196 - Attachment is obsolete: true
Attachment #8742196 - Flags: feedback?(dao+bmo)
(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.
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Attached patch Patch v2 (obsolete) — Splinter Review
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
Attachment #8742617 - Attachment is obsolete: true
Attachment #8748579 - Flags: review?(dao+bmo)
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?
Attachment #8748579 - Flags: review?(dao+bmo) → review+
Priority: -- → P2
Excuse me, what stops this from being landed?
(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.
(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.
Attached patch Patch v3Splinter Review
Would have landed this now, if any of the trees were open
Attachment #8748579 - Attachment is obsolete: true
Attachment #8755401 - Flags: checkin?
Blair, any news about this bug? Thanks

Updating the tracking flags to match the reality. If we had the bug in 38...
Stealing.
Assignee: bmcbride → mdeboer
Iteration: --- → 50.4 - Aug 1
Points: --- → 5
Flags: needinfo?(bmcbride)
Wow, I totally missed this for such a long time! I must've been distracted :-/

Alright, on it again!
Assignee: mdeboer → beachjar
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
Priority: P2 → P3
You need to log in before you can comment on or make changes to this bug.