622872 - Broken experience with the following STR (leads to frozen UI)

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Nochum Sossonko [:Natch]]

STR:

1) Open blank tab
2) Open panorama, isolate the tab by dragging it out of the group
3) Hit [x] top-left close window button to exit panorama
4) Close the tab via [x] on tab, re-enter panorama screen
5) Open new tab from within panorama via ff button->New Tab
6) Re-enter panorama (via panorama button) and close the group that holds the blank tab
6) Click on original tab, animation will indicate that the tab is opening, but it doesn't.

ER:
Tab opens and panorama's state indicates one tab open in one tab group.

AR:
Tab doesn't open until the second click and panorama thinks there's more than one tab open (as can be seen by [x] on the last remaining tab in the window). When the tab is closed via the [x] on the tab, the browser freezes.

Comment 1

[Michael Yoshitaka Erlewine [:mitcho]]

I couldn't reproduce with exactly these STR as there's no [x] button on the tab if there's only one tab in the window, at least on Mac. However, I could produce what is most likely the same effect in a slightly different way... will try to document.

The effect of freezing for me was an unresponsive script error citing:

Script: chrome://browser/content/tabbrowser.xml:1552

Nochum, is that what you see?

Comment 2

[Michael Yoshitaka Erlewine [:mitcho]]

Yup, reproduced. Going to write a test to automate this sequence now.

I second the nomination as blocker.

Comment 3

[Nochum Sossonko [:Natch]]

(In reply to comment #1)
> I couldn't reproduce with exactly these STR as there's no [x] button on the tab
> if there's only one tab in the window, at least on Mac. However, I could

On Windows, in this particular case, there is an [x].

> produce what is most likely the same effect in a slightly different way... will
> try to document.
> 
> The effect of freezing for me was an unresponsive script error citing:
> 
> Script: chrome://browser/content/tabbrowser.xml:1552
> 
> Nochum, is that what you see?

Yes.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

If the only way to hit this is the STR listed in comment 0, then I don't think this blocks, though we'd approve a patch most likely.

Mitcho: if you think this could be hit more frequently/easily, please renominate.

Comment 5

[Nochum Sossonko [:Natch]]

FYI, the AR aren't the same, but the symptoms are similar for these (very easy to hit) STR:

1) Open panorama
2) Click and hold the mousedown button on empty space in the window.
3) Create a new tab from the ff button.

AR:

When you go back into panorama, this group do not show up anywhere.

ER:

There should be either a new group for the newly opened tab or the tab should be placed in the old group.

Comment 6

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #5)
> FYI, the AR aren't the same, but the symptoms are similar for these (very easy
> to hit) STR:
> 
> 1) Open panorama
> 2) Click and hold the mousedown button on empty space in the window.
> 3) Create a new tab from the ff button.
> 
> AR:
> 
> When you go back into panorama, this group do not show up anywhere.
> 
> ER:
> 
> There should be either a new group for the newly opened tab or the tab should
> be placed in the old group.

Natch, I'm not following your directions here. Could you explain it a slightly different way, or make a video?

In particular, I don't know what you mean by "click and hold the button" part... how long? What happens immediately after you do that? What did you expect?

Comment 7

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #6)

Also, sorry, Natch, if this is not the same bug (or doesn't seem to be on the surface), please open another bug for it.

Comment 8

[Nochum Sossonko [:Natch]]

The hold-n-click just means you have to hold the mouse button down for like a full second so that focus is moved away from the active group in panorama. This where the problem lies, when panorama inserts a tab into no-group land because no group is selected when the new tab menu item is pressed. If you understand it now, and think it warrants another bug, I'll file.

Comment 9

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #8)
> The hold-n-click just means you have to hold the mouse button down for like a
> full second so that focus is moved away from the active group in panorama. This
> where the problem lies, when panorama inserts a tab into no-group land because
> no group is selected when the new tab menu item is pressed. If you understand
> it now, and think it warrants another bug, I'll file.

Natch, is this on beta 8 or on a nightly?

Comment 10

[Nochum Sossonko [:Natch]]

Nightly. Don't know about Beta 8, I'm not a beta user ;)

Comment 11

[Nochum Sossonko [:Natch]]

Ok, so bug 619937 fixed the second str, first str in description still hold. Note, this totally bakes the panorama UI as well as the Firefox UI. Even if it is an edge-case, I don't know of another edge-case bug that completely borks the main chrome UI like this does (and isn't a blocker). It's not just a hang, it's a messed up UI/UX.

Comment 12

[Dietrich Ayala (:dietrich)]

Blocking... softly. The STR seem contrived, not something that regular usage might ever hit. However, the result is terrible. So I'm marking blocking+ mostly because I'm worried that there are other STR that might lead to the same place.

Comment 13

[Michael Yoshitaka Erlewine [:mitcho]]

Here, for reference, is a console message I get when I follow the STR:

tabview assert: should already be linkedTabItem_setBounds([object Object],false)@chrome://browser/content/tabview.js:4934
GroupItem_arrange_children_each([object Object],0,[object Array])@chrome://browser/content/tabview.js:3254
GroupItem_arrange([object Object])@chrome://browser/content/tabview.js:3247
GroupItem_add([object Object],[object Object])@chrome://browser/content/tabview.js:2977
([object Object],0,[object Array])@chrome://browser/content/tabview.js:2372
GroupItem([object Array],[object Object])@chrome://browser/content/tabview.js:2371
GroupItems_newTab([object Object],[object Object])@chrome://browser/content/tabview.js:4137
TabItem__reconnect()@chrome://browser/content/tabview.js:4845
TabItem([object XULElement],[object Object])@chrome://browser/content/tabview.js:4691
TabItems_link([object XULElement])@chrome://browser/content/tabview.js:5481
([object XULElement],[object Event])@chrome://browser/content/tabview.js:5308
((function (tab) {if (tab.ownerDocument.defaultView != gWindow || tab.pinned) {return;}self.link(tab);}),1,[object Array])@resource:///modules/tabview/AllTabs.jsm:127
([object Event])@resource:///modules/tabview/AllTabs.jsm:125
addTab("about:blank",(void 0))@chrome://browser/content/tabbrowser.xml:1291
loadOneTab("about:blank",(void 0))@chrome://browser/content/tabbrowser.xml:1080
BrowserOpenTab()@chrome://browser/content/browser.js:7233
oncommand([object XULCommandEvent])@chrome://browser/content/browser.xul:1

Comment 14

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: The group that won't die

Moreover, once you walk through this STR, you can get some other crazy things to happen too... here's an example.

Comment 15

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Video of given STR, plus bonus footage

This video, for those who want to see how it's done, is the steps in the STR in comment 1, as well as what happens if you wait around... there's some weird behavior if you try to continue using Panorama in this post-STR state, like The Group That Won't Die.

Note also another oddity not noted in the original report: when the second new tab is created, it's created in a new group (that's fine) which is drawn as if it's the *second* tab in the group. Spooky.

Comment 16

[Michael Yoshitaka Erlewine [:mitcho]]

softblocker = critical

Comment 17

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Test which runs STR

Okay, I believe I've accurately written a mochitest which follows the STR exactly. I'll reformat the STR to follow the step numbers in my test:

Part 1: Open blank tab, open panorama, isolate the tab by dragging it out of the group, zoom into it. Call this newTab.
Part 2: Close the tab, re-entering panorama screen
Part 3: Open new tab from within panorama "New Tab" menu item. Call this secondNewTab.
Part 4: Re-enter panorama
Part 5: There are groups now: the original and the new one which includes secondNewTab. Close the new group (closeAll).
Part 6: Actually close the group (closeHidden)... equivalent to closing the "undo close group" thing.
Part 7: Click on the original tab. We should be able to zoom into it.

Here's what happens when we run this test now:

(Start of course with one group with one tab.)

> Part 1: Open blank tab, open panorama, isolate the tab by dragging it out of the group, zoom into it. Call this newTab.
> Part 2: Close the tab, re-entering panorama screen
> Part 3: Open new tab from within panorama "New Tab" menu item. Call this secondNewTab.

"tabview assert: should already be linkedTabItem_setBounds", about FIVE TIMES.

At this point, we have a problem. We should be looking at secondNewTab, the tab that we just created via the menu or key shortcut. But it is not. We're instead looking at the original tab.

> Part 4: Re-enter panorama
> Part 5: There are groups now: the original and the new one which includes secondNewTab. Close the new group (closeAll).

(Before we close the new group:) We verify that there are two groups, and two tabs. The original group has one child, the original tab. BUT the second group has two children too... We're double counting! (And this accounts for why the new tab at this point in the video is positioned as if there's another tab in the group with it).

> Part 6: Actually close the group (closeHidden)... equivalent to closing the "undo close group" thing.

JavaScript Warning: "reference to undefined property browser.webProgress"

Comment 18

[Michael Yoshitaka Erlewine [:mitcho]]

I'm happy to continue looking at this, but if someone who better understands our linking of TabItems to xul:tabs (Raymond? Ian? Mardak?) could take a look at this, it may be faster.

Comment 19

[Raymond Lee [:raymondlee]]

Attachment: Patch 1

mitcho:  The problem is that the GroupItems._activeOrphanTab is not clear when the orphan tab is closed.  This makes GroupItems_newTab() to create a new group item which contains an orphan tab item (which is already removed) and a new tab item.  Please update your test if this patch works for you.

Comment 20

[Michael Yoshitaka Erlewine [:mitcho]]

Yes, Raymond, this seems to nip the issue in the bud! Great! Here's what happens when we try to follow the STR now:

> Part 1: Open blank tab, open panorama, isolate the tab by dragging it out of
> the group, zoom into it. Call this newTab.
> Part 2: Close the tab, re-entering panorama screen
> Part 3: Open new tab from within panorama "New Tab" menu item. Call this
> secondNewTab.
> Part 4: Re-enter panorama
> Part 5: There are groups now: the original and the new one which includes
> secondNewTab. Close the new group (closeAll).

Part 5 is different here. The new tab we just created doesn't spawn a new group; instead, it's added to the only group that exists at this point (the original group), which is the expected behavior.

Updating test now. Thanks Raymond!

Comment 21

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Updated test for new verification STR

For some reason it fails this line: "This second new tab is what we're looking at". Manually verifying the STR up to there, I get the expected result.

Will investigate later today.

Comment 22

[Raymond Lee [:raymondlee]]

Attachment: Merged the patch and test

Sent it to try and waiting for the result

Comment 23

[Raymond Lee [:raymondlee]]

Attachment: Merged the patch and test

Comment 24

[Michael Yoshitaka Erlewine [:mitcho]]

I am no hero. Raymond is the hero.

Comment 25

[Raymond Lee [:raymondlee]]

Comment on attachment 505338 [details] [diff] [review]
Merged the patch and test

The tests failed on try debug build. Investigating...

Comment 26

[Raymond Lee [:raymondlee]]

Attachment: v2

Passed Try!

Comment 27

[Ian Gilman [:iangilman]]

Comment on attachment 505415 [details] [diff] [review]
v2

>+      if (!tab._tabViewTabItem.parent && 
>+          tab._tabViewTabItem == GroupItems.getActiveOrphanTab())
>+        GroupItems.setActiveOrphanTab(null);

Seems like the .parent check is redundant. 

Otherwise looks great... good catch!

Comment 28

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Removed the .parent check. Ready for check-n.

Comment 29

[Ian Gilman [:iangilman]]

it bounced when I tried to land it: 

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1295633044.1295635945.11723.gz

Comment 30

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

This is the updated version. Just sent it to try and waiting for the result.

Comment 31

[Kevin Hanes]

bugspam. Moving b10 to b11

Comment 32

[Kevin Hanes]

bugspam. Removing b10

Comment 33

[Raymond Lee [:raymondlee]]

For some reasons, the try doesn't show the test results.  Sending to it again

Comment 34

[Raymond Lee [:raymondlee]]

Passed Try!

Comment 35

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/196efe558754

Comment 36

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre