Panorama closes browser window when closing "undo close group"

VERIFIED FIXED in Firefox 4.0b10

Status

defect
P2
critical
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: florian.neubauer.neuroscience, Assigned: ttaubert)

Tracking

Trunk
Firefox 4.0b10
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [softblocker][fx4-fixed-bugday])

Attachments

(1 attachment, 4 obsolete attachments)

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
Reporter

Comment 1

9 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.
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?
Reporter

Comment 3

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

Comment 5

9 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.
Severity: major → critical
Component: Tabbed Browser → TabCandy
Keywords: crash
QA Contact: tabbed.browser → tabcandy
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.
Reporter

Comment 7

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

Comment 8

9 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.
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.
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Ever confirmed: true
Whiteboard: DUPEME
Reporter

Comment 10

9 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.
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
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).
This might be more like bug 597188, except you don't need to be in PB mode.
Reporter

Comment 14

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

Comment 15

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

9 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.
Assignee: nobody → aza
Blocks: 598154
Priority: -- → P2
Keywords: crash
Summary: Panorama crashes browser, 100% reproducible, after action sequence → Panorama closes browser window when closing "undo close group"
blocking2.0: ? → betaN+
Keywords: crash
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."

Updated

9 years ago
Assignee: aza → ian
Assignee: ian → nobody
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED

Comment 19

9 years ago
bugspam (moving b9 to b10)
Blocks: 608028

Comment 20

9 years ago
bugspam (removing b9)
No longer blocks: 598154
Reporter

Comment 21

9 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).
Whiteboard: DUPEME → DUPEME [softblocker]
Posted patch patch v1 (obsolete) — Splinter Review
Passed try.
Attachment #503716 - Flags: review?(ian)
Whiteboard: DUPEME [softblocker] → [softblocker]
Attachment #503716 - Attachment is obsolete: true
Attachment #504309 - Flags: review?(ian)
Attachment #503716 - Flags: review?(ian)
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.
Attachment #504309 - Flags: review?(ian) → review+
OS: Windows XP → All
Hardware: x86 → All
Target Milestone: --- → Firefox 4.0b10
Version: unspecified → Trunk
Posted patch patch v2 (obsolete) — Splinter Review
(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 :)
Attachment #504309 - Attachment is obsolete: true
Attachment #504885 - Flags: review?(ian)
Passed try!
Duplicate of this bug: 576427
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.
Attachment #504885 - Flags: review?(ian) → review-
Posted patch patch v3 (obsolete) — Splinter Review
(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 :)
Attachment #504885 - Attachment is obsolete: true
Attachment #505310 - Flags: review?(ian)
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 :)
Attachment #505310 - Flags: review?(ian) → review+
Attachment #505310 - Flags: approval2.0?
Attachment #505310 - Flags: approval2.0?
Comment added.
Attachment #505310 - Attachment is obsolete: true
Whiteboard: [softblocker] → [softblocker][needs landing]
(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?
I was sure I pushed again but I can't find it in TBPL. Removing checkin-needed until we have another try push passed.
Keywords: checkin-needed
Whiteboard: [softblocker][needs landing] → [softblocker]

Comment 34

9 years ago
bugspam. Moving b10 to b11
Blocks: 627096

Comment 35

9 years ago
bugspam. Removing b10
No longer blocks: 608028
Passed try.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/ef150764ac97
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 38

8 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
Status: RESOLVED → VERIFIED
Whiteboard: [softblocker] → [softblocker][fx4-fixed-bugday]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.