florian.neubauer.neuroscience Reporter
	Description • 14 years ago

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

	florian.neubauer.neuroscience Reporter
	Comment 1 • 14 years ago

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.

	juan becerra [:juanb]
	Comment 2 • 14 years ago

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?

	florian.neubauer.neuroscience Reporter
	Comment 3 • 14 years ago

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.

	Matthias Versen [:Matti]
	Comment 4 • 14 years ago

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

	florian.neubauer.neuroscience Reporter
	Comment 5 • 14 years ago

@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.

	juan becerra [:juanb]
	Comment 6 • 14 years ago

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.

	florian.neubauer.neuroscience Reporter
	Comment 7 • 14 years ago

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.

	florian.neubauer.neuroscience Reporter
	Comment 8 • 14 years ago

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.

	juan becerra [:juanb]
	Comment 9 • 14 years ago

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.

	florian.neubauer.neuroscience Reporter
	Comment 10 • 14 years ago

- 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.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 11 • 14 years ago

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

	juan becerra [:juanb]
	Comment 12 • 14 years ago

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).

	juan becerra [:juanb]
	Comment 13 • 14 years ago

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

	florian.neubauer.neuroscience Reporter
	Comment 14 • 14 years ago

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

	florian.neubauer.neuroscience Reporter
	Comment 15 • 14 years ago

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").

	juan becerra [:juanb]
	Comment 16 • 14 years ago

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?

	Kevin Hanes
	Comment 17 • 14 years ago

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.

	Aza Raskin [:aza]
	Comment 18 • 14 years ago

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."

	Kevin Hanes
	Comment 19 • 14 years ago

bugspam (moving b9 to b10)

	Kevin Hanes
	Comment 20 • 14 years ago

bugspam (removing b9)

	florian.neubauer.neuroscience Reporter
	Comment 21 • 14 years ago

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).

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 22 • 14 years ago

Passed try.

	Ian Gilman [:iangilman]
	Comment 24 • 14 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 25 • 14 years ago

(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 :)

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 26 • 14 years ago

Passed try!

	Ian Gilman [:iangilman]
	Comment 28 • 14 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 29 • 14 years ago

(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 :)

	Ian Gilman [:iangilman]
	Comment 30 • 14 years ago

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 :)

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 31 • 14 years ago

Comment added.

	Michael Yoshitaka Erlewine [:mitcho]
	Comment 32 • 14 years ago

(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?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 33 • 14 years ago

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

	Kevin Hanes
	Comment 34 • 14 years ago

bugspam. Moving b10 to b11

	Kevin Hanes
	Comment 35 • 14 years ago

bugspam. Removing b10

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 36 • 14 years ago

Passed try.

	Ian Gilman [:iangilman]
	Comment 37 • 14 years ago

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

	Ben
	Comment 38 • 14 years ago

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