613541 - Panorama closes browser window when closing "undo close group"

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[florian.neubauer.neuroscience]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b7) Gecko/20100101 Firefox/4.0b7
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b7) Gecko/20100101 Firefox/4.0b7

1) Have more than 1 tab.
2) Open panorama using panorama button.
3) Close the tab group in panorama.
4) Hit the grey "close panorama" button.
5) Hit the "undo close group" button
--> crash.

Reproducible: Always

Actual Results:  
crashes

Expected Results:  
panorama closes, expect blank new tab in ff

Comment 1

[florian.neubauer.neuroscience]

Improved description of sequence:

1) Open panorama using panorama button.
2) Close entire tab group in panorama.
3) Hit grey "close panorama" button.
4) Hit the "X" (close button) of the "undo close group" button.

Comment 2

[juan becerra [:juanb]]

Florian, does the browser crash or does it close? Do you see any crash instances in about:crashes? If you have more than one window open, do you also see this problem?

Comment 3

[florian.neubauer.neuroscience]

2. Improved description of sequence - I am sorry - it is my first bug.

1) Open panorama using panorama button.
2) Close entire tab group in panorama.
3) Hit the "X" (close button) of the "undo close group" button.

Entire browser crashes reliably.

Comment 4

[Matthias Versen [:Matti]]

Please post a crash ID from about:crashes
https://developer.mozilla.org/en/How_to_get_a_stacktrace_for_a_bug_report

Comment 5

[florian.neubauer.neuroscience]

@juan: There are no entries in the about:crashes log. 

HOWEVER: When I start ff again, I get the message window:
"Firefox is having trouble recovering your windows and tabs."

In the past I had seen this window only after crashes therefore I assumed it is a crash. 

Sum: Even if what I see is not a crash there is a bugs then:
Re-opening the browser should not result in an error message.

Comment 6

[juan becerra [:juanb]]

Florian, I'll try to reproduce this problem. Are you using any add-ons? Which? This still might be a dupe of a problem where this Panorama interaction was closing the browser unintentionally.

Comment 7

[florian.neubauer.neuroscience]

Yes, one of my add-ons has to be blamed.
I disabled all add-ons and all plug-ins, and the problem disappeared.

I have more add-ons and plug-ins than I was aware of. 
I quickly re-enabled just "adblock plus" and "noscript" - they are not the cause.
I do not have the time right now to figure out which of the other add-ons it was.

Sorry for filing an add-on bug here.

Comment 8

[florian.neubauer.neuroscience]

Update: It IS a firefox bug.

All plug-ins and all add-ons disabled.

Now, to get the bug I do the same sequence of actions.
It just has to be done quicker to get the error message next time you open the browser!

So the plug-ins and add-ons probably just delayed something in the shut down process and therefore I got the bug every time, now I get it only if I click the above sequence quickly.

Comment 9

[juan becerra [:juanb]]

I've confirmed the problem on my XP vm. I'll check for a regression range. It sounds a lot like bug 595521. I'm nominating for blocking because it is an unintended result of closing that button.

Comment 10

[florian.neubauer.neuroscience]

- Triplechecked: All plug-ins and all add-ons disabled, default appearance.
- Closing the last tab group and afterwards clicking quickly (!) the close button of the "undo close group" button leads in 100% of trials to the error page "Firefox is having trouble recovering your windows and tabs" after re-opening the browser.
- However, I could never get it in safe mode (0% as opposed to 100% of trials in normal mode)
- I am ready to send any other settings/log files to one of your experts if you want.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Can we get a stacktrace of the crash? Since breakpad doesn't work windbg should be helpful here:

https://developer.mozilla.org/en/how_to_get_a_stacktrace_with_windbg

Comment 12

[juan becerra [:juanb]]

I tried having two windows open per comment #2, and when I follow the steps to reproduce this, the browser doesn't quit or crash. The other window remains open. I'm not sure this is similar to bug 595521 after all, but the behavior doesn't feel right.

This might not be a crash, but a normal browser quit which isn't handled by session store (just a guess).

Comment 13

[juan becerra [:juanb]]

This might be more like bug 597188, except you don't need to be in PB mode.

Comment 14

[florian.neubauer.neuroscience]

Now I got the error message also in safe mode.
Only modification: I was clicking the sequence even faster.

Comment 15

[florian.neubauer.neuroscience]

This all seems to me to be part of a more complex problem. I do not know anything about the architecture of firefox, but I discovered the following related bug just now:

A) Firefox 4b7 in normal mode, all add-ons and plug-ins disabled:
1) close the last open tab group in panorama
2) wait >10 seconds for browser closing
3) open browser again
--> I get the error page "Firefox is having trouble recovering your windows and tabs"

B) Firefox 4b7 in safe mode:
1) close the last open tab group in panorama
2) wait >10 seconds for browser closing
3) open browser again
--> Browser opens with the tabs which were open last INSTEAD of opening with my home page (I have set in tools>options: "When Firefox Starts: Show my home page").

Comment 16

[juan becerra [:juanb]]

Aza, it looks like the browser quitting is an unintended consequence of closing the "Undo Close Group" button (similar to bug 597188). Having a blank tab come up would feel less unexpected.

What should happen in this scenario?

Comment 17

[Kevin Hanes]

Both closing FF and and dragging out of Panorama into a new tab seem asymmetrical for the action the user performs (hitting the little x). In lieu of being able to do the "right thing" (leave the user in a blank Panorama), we should probably just make it impossible to kill that last "undo close group".

Either way, it seems like something we should fix sooner rather than later.

Comment 18

[Aza Raskin [:aza]]

Closing the last group should open a new blank tab. This follows from the precept: "If there is only one thing to do, the computer should do it automatically."

Comment 19

[Kevin Hanes]

bugspam (moving b9 to b10)

Comment 20

[Kevin Hanes]

bugspam (removing b9)

Comment 21

[florian.neubauer.neuroscience]

Closing the last tab in the last group instead of closing the last tab group closes the browser immediately. Maybe, when you decide how the browser should behave in the case of closing the last tab group, also have this case in mind. I feel/guess that the response for closing the last tab group should be the same as for closing the last tab in Panorama (and IMHO this should not be closing the browser as closing the last tab outside panorama doesn't close ff either).

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Passed try.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1b (small fixes in head.js)

Comment 24

[Ian Gilman [:iangilman]]

Comment on attachment 504309 [details] [diff] [review]
patch v1b (small fixes in head.js)

>+    // When the last non-empty groupItem is closed and there are no orphan tabs
>+    // create a new blank group.
>+    let remainingGroups = GroupItems.groupItems.filter(function (groupItem) {
>+      return (groupItem != self && !groupItem.hidden &&
>+              groupItem.getChildren().length);

We should do this for the last hidden group, not the first one (in other words remove that !groupItem.hidden check). Let's say you have 3 groups and you close all three of them. 15 seconds later, 2 of their "undo close group" buttons will automatically disappear, but the last one will stick around until the user does something (closes it manually, switches out of Panorama, creates a new tab, etc). We want the behavior from this patch to show itself in response to that last manual action, not the former automatic actions.

>-      // no visible groups, no orphaned tabs and no apps tabs, open a new group
>-      // with a blank tab
>-      if (unhiddenGroups.length == 0 && GroupItems.getOrphanedTabs().length == 0 &&
>-          gBrowser._numPinnedTabs == 0) {
>-        let box = new Rect(20, 20, 250, 200);
>-        let groupItem = new GroupItem([], { bounds: box, immediately: true });
>-        groupItem.newTab();
>+      // no visible groups and no orphaned tabs, open a new group with a blank tab
>+      if (!unhiddenGroups.length && !GroupItems.getOrphanedTabs().length) {
>+        GroupItems.newGroup();

You took out the app tab check here; that needs to go back in... if there's nothing but app tabs and the user chooses to exit Panorama, they need to go to one of those app tabs, not a new blank tab.

>+  let actionCloseLastGroup = function (callback) {
>+    getFirstGroupItem().closeHidden();
>+    callback();
>+  }

Seems funny to be closing the first groupItem in a routine called actionCloseLastGroup... 

>diff --git a/browser/base/content/test/tabview/head.js b/browser/base/content/test/tabview/head.js
>--- a/browser/base/content/test/tabview/head.js
>+++ b/browser/base/content/test/tabview/head.js

Looks like these additions to head.js appear in more than one of your patches. That's fine for development, but when it comes time to land, you'll need to clean them out of all but one and land that one first. 

R+ with the above addressed.

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

(In reply to comment #24)
> We should do this for the last hidden group, not the first one (in other words
> remove that !groupItem.hidden check). Let's say you have 3 groups and you close
> all three of them. 15 seconds later, 2 of their "undo close group" buttons will
> automatically disappear, but the last one will stick around until the user does
> something (closes it manually, switches out of Panorama, creates a new tab,
> etc). We want the behavior from this patch to show itself in response to that
> last manual action, not the former automatic actions.

Fixed.

> You took out the app tab check here; that needs to go back in... if there's
> nothing but app tabs and the user chooses to exit Panorama, they need to go to
> one of those app tabs, not a new blank tab.

Fixed.

> Looks like these additions to head.js appear in more than one of your patches.
> That's fine for development, but when it comes time to land, you'll need to
> clean them out of all but one and land that one first. 

Yeah I know. Luckily these functions did now land in head.js so I removed them.


I'm flagging you for review again because I rewrote the whole test. It is now much clearer and more explicit about what is going on and what behavior is expected. While rewriting this I could remove some flaws that this patch didn't cover and could also improve the behaviour a little bit - so it was definitely worth it :)

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Passed try!

Comment 28

[Ian Gilman [:iangilman]]

Comment on attachment 504885 [details] [diff] [review]
patch v2

I'd say that test pretty much covers it! :) Good catch on reusing empty groups as well. 

>+      // no visible groups and no orphaned tabs: open a new group. open a blank
>+      // tab and return if there are no pinned tabs.
>+      if (!unhiddenGroups.length && !GroupItems.getOrphanedTabs().length) {
>+        let emptyGroups = GroupItems.groupItems.filter(function (groupItem) {
>+          return (!groupItem.hidden && !groupItem.getChildren().length);
>+        });
>+        let group = (emptyGroups.length ? emptyGroups[0] : GroupItems.newGroup());
>+        if (!gBrowser._numPinnedTabs) {
>+          group.newTab();
>+          return;
>+        }

The pinned tabs check might as well be in the outter if; save us some unneeded work.

>+  let assertNewGroupItemCreated = function (groupItem) {
>+    isnot(getGroupItem(0), groupItem, 'new groupItem was created');
>+  }
>+
>+  let assertExistingGroupItemPreserved = function (groupItem) {
>+    is(getGroupItem(0), groupItem, 'existing groupItem was preserved');
>+  }

These routines are named after how they're supposed to be used, not really after what they actually do... could be a source of confusion. I'm not sure what to suggest as an alternative; perhaps just change assertNewGroupItemCreated to assertExistingGroupItemNotPreserved and leave the other as it is?

>+  // setup: 2 non-empty groups
>+  // action: close on group

I imagine that should be "close one group". 

>+  // setup: 1 hidden group, 1 non-empty group
>+  // action: hide non-empty group, exit panorama
>+  // expected: new group with blank tab is created and zoomed into
>+  let testHiddenGroup2 = function () {
>+    let groupItem = getGroupItem(0);
>+    let hiddenGroupItem = createGroupItem(1);
>+    assertNumberOfGroupItems(2);
>+
>+    hideGroupItem(hiddenGroupItem, function () {
>+      hideGroupItem(groupItem, function () {
>+        hideTabView(function () {
>+          assertNumberOfGroupItems(1);
>+          assertNewGroupItemCreated(hiddenGroupItem);
>+          next();
>+        });
>+      });
>+    });
>+  }

Here's a case where the name "assertNewGroupItemCreated" is biting you... seems like the one call should be enough, given its name, but actually you need to call it once for each of the possible groups in order to make sure neither of them were reused.

More random thoughts about tests: 

* I like to verify at the end of every test that we left things the way we should; that way if the next test fails, you know who to blame.

* With so many identical is() calls, I find it helps to analyze failures if there's some indication which sub-test was running at the time. For instance, at the top of each test function inside this test you could set some string to the name of the test, and all the assert functions could use that string.

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

(In reply to comment #28)
> >+      // no visible groups and no orphaned tabs: open a new group. open a blank
> >+      // tab and return if there are no pinned tabs.
> >+      if (!unhiddenGroups.length && !GroupItems.getOrphanedTabs().length) {
> >+        let emptyGroups = GroupItems.groupItems.filter(function (groupItem) {
> >+          return (!groupItem.hidden && !groupItem.getChildren().length);
> >+        });
> >+        let group = (emptyGroups.length ? emptyGroups[0] : GroupItems.newGroup());
> >+        if (!gBrowser._numPinnedTabs) {
> >+          group.newTab();
> >+          return;
> >+        }
> 
> The pinned tabs check might as well be in the outter if; save us some unneeded
> work.

Well, the important part here is that 'GroupItems.newGroup()' can be called regardless of whether there are pinned tabs or not. Do you think that is too much of a side-effect?

> >+  let assertNewGroupItemCreated = function (groupItem) {
> 
> These routines are named after how they're supposed to be used, not really
> after what they actually do... could be a source of confusion. I'm not sure
> what to suggest as an alternative; perhaps just change
> assertNewGroupItemCreated to assertExistingGroupItemNotPreserved and leave the
> other as it is?

You're right, the naming is a bit unfortunate. I renamed them to assertGroupItemRemoved() and assertGroupItemExists(). I think that is exactly what I want to assert here.

> * I like to verify at the end of every test that we left things the way we
> should; that way if the next test fails, you know who to blame.
>
> * With so many identical is() calls, I find it helps to analyze failures if
> there's some indication which sub-test was running at the time. For instance,
> at the top of each test function inside this test you could set some string to
> the name of the test, and all the assert functions could use that string.

Good idea. That would have saved me some time if I did that a bit sooner :)

Comment 30

[Ian Gilman [:iangilman]]

Comment on attachment 505310 [details] [diff] [review]
patch v3

(In reply to comment #29)
> Well, the important part here is that 'GroupItems.newGroup()' can be called
> regardless of whether there are pinned tabs or not. Do you think that is too
> much of a side-effect?

Good point; I hadn't even noticed that. Ok, that's cool. Maybe just add a comment in the code?

> Good idea. That would have saved me some time if I did that a bit sooner :)

Next time :)

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Comment added.

Comment 32

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #31)
> Created attachment 505657 [details] [diff] [review]
> patch for checkin
> 
> Comment added.

Tim, has the latest version of this patch gone through try?

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

I was sure I pushed again but I can't find it in TBPL. Removing checkin-needed until we have another try push passed.

Comment 34

[Kevin Hanes]

bugspam. Moving b10 to b11

Comment 35

[Kevin Hanes]

bugspam. Removing b10

Comment 36

[Tim Taubert [:ttaubert] (inactive)]

Passed try.

Comment 37

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/ef150764ac97

Comment 38

[Ben]

Problem fixed with

Mozilla/5.0 (Windows NT 5.1; rv:2.0b11) Gecko/20100101 Firefox/4.0b11 ID:20110203141415

under Windows XP. I tested that 4.0b10 did crash with the same STR.

Benoît