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

VERIFIED FIXED in Firefox 4.0b11

Status

defect
P2
normal
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: Natch, Assigned: raymondlee)

Tracking

Trunk
Firefox 4.0b11
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

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

Attachments

(2 attachments, 8 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Updated

8 years ago
blocking2.0: --- → ?
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?
Yup, reproduced. Going to write a test to automate this sequence now.

I second the nomination as blocker.
Keywords: helpwanted
OS: Windows Vista → All
Hardware: x86 → All
(Reporter)

Comment 3

8 years ago
(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.
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.
blocking2.0: ? → -
(Reporter)

Comment 5

8 years ago
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.

Updated

8 years ago
Blocks: 608028
Priority: -- → P3
(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?
(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.
(Reporter)

Comment 8

8 years ago
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.
(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?
blocking2.0: - → ?
Priority: P3 → --
(Reporter)

Comment 10

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

Comment 11

8 years ago
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.
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.
blocking2.0: ? → final+
Whiteboard: [softblocker]
Assignee: nobody → mitcho
Priority: -- → P2
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
Posted video The group that won't die (obsolete) —
Moreover, once you walk through this STR, you can get some other crazy things to happen too... here's an example.
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.
Attachment #504009 - Attachment is obsolete: true
Attachment #504029 - Attachment description: Video of STR in comment 1, plus bonus footage → Video of given STR, plus bonus footage
softblocker = critical
Severity: normal → critical
Severity: critical → normal
Posted file Test which runs STR (obsolete) —
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"
Attachment #504877 - Flags: feedback?(ian)
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.
(Assignee)

Comment 19

8 years ago
Posted patch Patch 1 (obsolete) — Splinter Review
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.
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!
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.
Attachment #504877 - Attachment is obsolete: true
Attachment #504877 - Flags: feedback?(ian)
(Assignee)

Comment 22

8 years ago
Posted patch Merged the patch and test (obsolete) — Splinter Review
Sent it to try and waiting for the result
Attachment #505018 - Attachment is obsolete: true
Attachment #505060 - Attachment is obsolete: true
(Assignee)

Comment 23

8 years ago
Posted patch Merged the patch and test (obsolete) — Splinter Review
Attachment #505337 - Attachment is obsolete: true
Attachment #505338 - Flags: review?(ian)
I am no hero. Raymond is the hero.
Assignee: mitcho → raymond
(Assignee)

Comment 25

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

The tests failed on try debug build. Investigating...
Attachment #505338 - Flags: review?(ian) → review-
(Assignee)

Comment 26

8 years ago
Posted patch v2 (obsolete) — Splinter Review
Passed Try!
Attachment #505338 - Attachment is obsolete: true
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!
Attachment #505415 - Flags: review?(ian) → review+
(Assignee)

Comment 28

8 years ago
Posted patch Patch for check-in (obsolete) — Splinter Review
Removed the .parent check. Ready for check-n.
Attachment #505415 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Whiteboard: [softblocker] → [softblocker][passed try][needs landing]
Status: NEW → ASSIGNED
(Assignee)

Comment 30

8 years ago
This is the updated version. Just sent it to try and waiting for the result.
Attachment #505649 - Attachment is obsolete: true

Comment 31

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

Comment 32

8 years ago
bugspam. Removing b10
No longer blocks: 608028
Whiteboard: [softblocker][passed try][needs landing] → [softblocker]
(Assignee)

Comment 33

8 years ago
For some reasons, the try doesn't show the test results.  Sending to it again
(Assignee)

Comment 34

8 years ago
Passed Try!
(Assignee)

Updated

8 years ago
Keywords: checkin-needed

Comment 35

8 years ago
http://hg.mozilla.org/mozilla-central/rev/196efe558754
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b11
verified with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre
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.