597360 - Closing the last group results in weird behavior (window closes after pressing Ctrl+T, unresponsiveness, tabview asserts...)

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Steffen Wilberg]

Mozilla/5.0 (Windows NT 6.0; WOW64; rv:2.0b7pre) Gecko/20100916 Firefox/4.0b7pre

Closing the last group results in weird behavior (window closes, unresponsiveness, tabview asserts...).

Tested with a new profile:

1. Open tabcandy.
--> it displays the open tabs in a group, and the intro video.
2. Close all tab groups in tabcandy by clicking on the "x" in the top-right corner of the group.
--> Panorama is now pretty empty, showing only the tabcandy button on the top right, the search box below, and the Undo Close Group box.
Notice that Esc, Ctrl+Space, and the tabcandy button don't work.
The search box works, but that doesn't help...
3. Press Ctrl+T
--> The window is closed! Luckily, I had the error console open, and could do window.open()

Second try:
0. In about:config, set browser.tabs.closeWindowWithLastTab to false.
1. and 2. like before. The intro video is back again as well.
3. Press Ctrl+T to open a new tab.
--> The new tab is displayed instead of tabcandy.
4. Press Ctrl+Space
--> A single new tab is displayed, outside any group.
5. Click on that new tab, click on the tabcandy button, hit Esc, hit Ctrl+Space.
--> nothing happens.
6. Draw a new group over it
--> the tab moves away from the new group.
7. Move the tab into the new group, and click the tab.
--> it becomes larger, and shrinks again.
8. Click again for a second (and maybe a third) time.
--> it opens up.
9. Go back to tabcandy, move that tab outside the group.
--> the (then empty) group is removed.
10. Click on the tab.
--> from here on, I get different results, sometimes the tab opens up (unlike step 5), and I can go back to tabcandy, draw a group above it, and the tab moves into the group (unlike step 6)
--> but sometimes, I get the same behaviour as steps 6 and 7 above.

After step 3, the error console displays:
tabview assert: should already be linked([object XULElement],[object Event])@chrome://browser/content/tabview.js:4769
((function (tab) {if (tab.ownerDocument.defaultView != gWindow || tab.pinned) {return;}self.update(tab);}),1,[object Array])@resource:///modules/tabview/AllTabs.jsm:127
([object Event])@resource:///modules/tabview/AllTabs.jsm:125
_tabAttrModified([object XULElement])@chrome://browser/content/tabbrowser.xml:895
updateCurrentBrowser()@chrome://browser/content/tabbrowser.xml:862
onselect([object Event])@chrome://browser/content/browser.xul:1
@:0
set_selectedIndex(1)@chrome://global/content/bindings/tabbox.xml:654
set_selectedPanel([object XULElement])@chrome://global/content/bindings/tabbox.xml:673
set_selectedIndex(1)@chrome://global/content/bindings/tabbox.xml:394
set_selectedItem([object XULElement])@chrome://global/content/bindings/tabbox.xml:426
set_selectedTab([object XULElement])@chrome://global/content/bindings/tabbox.xml:106
set_selectedTab([object XULElement])@chrome://browser/content/tabbrowser.xml:1902
_blurTab([object XULElement])@chrome://browser/content/tabbrowser.xml:1648
_endRemoveTab([object XULElement])@chrome://browser/content/tabbrowser.xml:1517
removeTab([object XULElement])@chrome://browser/content/tabbrowser.xml:1403
TabItem_close()@chrome://browser/content/tabview.js:4540
([object Object],1,[object Array])@chrome://browser/content/tabview.js:4086
([object Object],0,[object Array])@chrome://browser/content/tabview.js:4084
GroupItems_removeHiddenGroups()@chrome://browser/content/tabview.js:4081
UI_hideTabView()@chrome://browser/content/tabview.js:6805
UI_onTabSelect([object XULElement])@chrome://browser/content/tabview.js:6961
([object XULElement],[object Event])@chrome://browser/content/tabview.js:6893
((function (tab) {if (tab.ownerDocument.defaultView != gWindow) {return;}self.onTabSelect(tab);}),0,[object Array])@resource:///modules/tabview/AllTabs.jsm:127
([object Event])@resource:///modules/tabview/AllTabs.jsm:125
updateCurrentBrowser()@chrome://browser/content/tabbrowser.xml:860
onselect([object Event])@chrome://browser/content/browser.xul:1
set_selectedIndex(1)@chrome://global/content/bindings/tabbox.xml:654
set_selectedPanel([object XULElement])@chrome://global/content/bindings/tabbox.xml:673
set_selectedIndex(1)@chrome://global/content/bindings/tabbox.xml:394
set_selectedItem([object XULElement])@chrome://global/content/bindings/tabbox.xml:426
set_selectedTab([object XULElement])@chrome://global/content/bindings/tabbox.xml:106
set_selectedTab([object XULElement])@chrome://browser/content/tabbrowser.xml:1902
loadOneTab("about:blank",(void 0))@chrome://browser/content/tabbrowser.xml:1038
BrowserOpenTab()@chrome://browser/content/browser.js:6992
oncommand([object XULCommandEvent])@chrome://browser/content/browser.xul:1

Error: browser.webProgress is undefined
Source File: chrome://browser/content/tabbrowser.xml
Line: 1470
which is the last line of this code:
// Remove the tab's filter and progress listener.
const filter = this.mTabFilters[aTab._tPos];
browser.webProgress.removeProgressListener(filter);

After step 6, the error console displays:
Warning: Error in parsing value for 'size'.  Declaration dropped.
Source File: chrome://browser/content/tabview.html
Line: 0

Warning: Error in parsing value for 'position'.  Declaration dropped.
Source File: chrome://browser/content/tabview.html
Line: 0

(several times)

tabview assert: should already be linkedTabItem_setBounds([object Object],false)@chrome://browser/content/tabview.js:4465
([object Object],0,[object Array])@chrome://browser/content/tabview.js:3077
GroupItem_arrange([object Object])@chrome://browser/content/tabview.js:3075
GroupItem_setBounds([object Object])@chrome://browser/content/tabview.js:2511
([object Object],0,[object Array])@chrome://browser/content/tabview.js:1466
Item_pushAway()@chrome://browser/content/tabview.js:1462
Drag_stop()@chrome://browser/content/tabview.js:5484
Item_snap()@chrome://browser/content/tabview.js:1534
GroupItem([object Array],[object Object])@chrome://browser/content/tabview.js:2331
finalize([object MouseEvent])@chrome://browser/content/tabview.js:7248
([object MouseEvent])@chrome://browser/content/tabview.js:699
([object MouseEvent])@chrome://browser/content/tabview.js:669

(twice)

After step 7, the error console displays:
Error: browser.webProgress is undefined
Source File: chrome://browser/content/tabbrowser.xml
Line: 1470

I also saw this:
tabview assert: should already be linkedTabItem_setBounds([object Object],false)@chrome://browser/content/tabview.js:4465
([object Object],1,[object Array])@chrome://browser/content/tabview.js:3077
GroupItem_arrange([object Object])@chrome://browser/content/tabview.js:3075
GroupItem_setBounds([object Object])@chrome://browser/content/tabview.js:2511
([object Object],0,[object Array])@chrome://browser/content/tabview.js:2033
Items_unsquish()@chrome://browser/content/tabview.js:2032
()@chrome://browser/content/tabview.js:2589
(16)@chrome://browser/content/tabview.js:607

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Closes the last group, the undo button gets displayed, presses cmd/ctrl+t and the window would remain and the new tab would be opened.

Comment 2

[Ian Gilman [:iangilman]]

Comment on attachment 495785 [details] [diff] [review]
v1

It seems to me the root of the problem is that it's possible for a hidden group to be the activeGroupItem. Let's fix that: make it impossible for a hidden group to be activeGroupItem (deactivate it when it becomes hidden, for instance, much like we already do with the active tab if it's in a group that gets hidden). 

I'm not against also checking in GroupItems.newTab just to verify that a hidden activeGroupItem hasn't slipped through (though if the above is taken care of, presumably we don't need it). If you do do so, however, rather than creating a fresh group immediately, you should just fall through to the rest of the newTab logic.

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 4

[Ian Gilman [:iangilman]]

Comment on attachment 496130 [details] [diff] [review]
v1

Looks good. Comments:

>+      if (closestTabItem.parent)
>+        GroupItems.setActiveGroupItem(closestTabItem.parent);
>+      else
>+        GroupItems.setActiveOrphanTab(closestTabItem); 

You should also setActiveGroupItem(null) if it's an orphan, right?

>+        let orphanedTabs = this.getOrphanedTabs();
>+        // add the new tabItem to the first visible group item
>         // set the orphan tab, and create a new group for orphan tab and 
>         // new tabItem
>-        let orphanedTabs = this.getOrphanedTabs();
>         if (orphanedTabs.length > 0)
>           orphanTabItem = orphanedTabs[0];

The "add the new tabItem to the first visible group item" comment is out of place.

R+ with those addressed.

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: v1

Sent to try and waiting for the results

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 7

[Raymond Lee [:raymondlee]]

(In reply to comment #6)
> Created attachment 496570 [details] [diff] [review]
> Patch for check-in

Passed Try!

Comment 8

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/01169ff998ba

Comment 9

[Tracy Walker [:tracy]]

Verified in recent nightly minefield build