Closed Bug 588712 Opened 9 years ago Closed 9 years ago

After restarting, showing tab groups results in cornered new tabs

Categories

(Firefox Graveyard :: Panorama, defect, P1)

defect

Tracking

(blocking2.0 final+)

VERIFIED FIXED
Firefox 4.0b7
Tracking Status
blocking2.0 --- final+

People

(Reporter: Mardak, Assigned: raymondlee)

References

Details

(Whiteboard: fixed by bug 587990)

Attachments

(1 file, 4 obsolete files)

If you open new tabs after restarting Firefox and have a tab that hasn't been grouped selected when showing groups, all the new tabs will end up in the corner as ungrouped tabs.
Nominating as blocking - it's very confusing when this happens, and the ways to get out of it (e.g. draw a group around all the ungrouped tabs piled up in the corner) are not obvious.
blocking2.0: --- → ?
Agreed; zpao, will this be fixed by any pending work on your end, or does this have nothing to do with how sessionStore is storing TabCandy info? (not an accusation, I'm just forgetting where those bugs are!)
blocking2.0: ? → final+
Would have helped if I'd cc'd zpao - zpao, please see comment 2! :)
This could be fixed with Dietrich's work in bug 588217, so bouncing to him.
Ed, can you clarify the steps to reproduce?

Also, this doesn't sound related to the session restore changes.
1) Group some tabs
2) Restart firefox
3) Open and select a new tab
4) Open Panorama

expected: new tab is in a group
actual: new tab is in the corner
Please clarify: are tabs being placed in the bottom right but then slowly moving to their appropriate respective groups (bug 591709, now marked as a duplicate of bug 590268 as that should solve it)? Or are they being placed in the bottom right and then not moving anywhere, in which case their group assignments were actually lost?
Duplicate of this bug: 593311
(In reply to comment #7)
> Please clarify: are tabs being placed in the bottom right but then slowly
> moving to their appropriate respective groups (bug 591709, now marked as a
> duplicate of bug 590268 as that should solve it)? Or are they being placed in
> the bottom right and then not moving anywhere, in which case their group
> assignments were actually lost?

I am seeing tabs placed in the bottom right and not moving anywhere - their group assignments were lost.  If I open one of the "cornered" tabs, then it appears alone in the tab bar (even if it was together with other tabs were the tab bar before activating Panorama).
(In reply to comment #9)
> I am seeing tabs placed in the bottom right and not moving anywhere - their
> group assignments were lost.  If I open one of the "cornered" tabs, then it
> appears alone in the tab bar (even if it was together with other tabs were the
> tab bar before activating Panorama).

Thanks for clarifying.
You can also trigger this by restarting Firefox, opening a tab, selecting it, pressing ctrl-` (to switch groups).
If you close one of these cornered tabs, you get:

tabview assert: must already be linked([object XULElement])@chrome://browser/content/tabview.js:4557
([object XULElement])@chrome://browser/content/tabview.js:4538
([object XULElement],[object Event])@chrome://browser/content/tabview.js:4474
((function (tab) {if (tab.ownerDocument.defaultView != gWindow) {return;}self.update(tab);}),0,[object Array])@resource:///modules/tabview/AllTabs.jsm:127
([object Event])@resource:///modules/tabview/AllTabs.jsm:125
_tabAttrModified([object XULElement])@chrome://browser/content/tabbrowser.xml:880
updateCurrentBrowser()@chrome://browser/content/tabbrowser.xml:847
onselect([object Event])@chrome://browser/content/browser.xul:1
The stack of how these items end up calling positionNewTabAtBottom(tabItem):

GroupItems_newTab([object Object])@chrome://browser/content/tabview.js:3548
TabItem([object XULElement])@chrome://browser/content/tabview.js:4003
TabItems_link([object XULElement])@chrome://browser/content/tabview.js:4619
([object XULElement],2,[object Array])@chrome://browser/content/tabview.js:4494
TabItems_init()@chrome://browser/content/tabview.js:4490
UI_init()@chrome://browser/content/tabview.js:6267
@chrome://browser/content/tabview.js:7147
Attached patch wip v1 (obsolete) — Splinter Review
Attachment #472799 - Flags: feedback?(ian)
Comment on attachment 472799 [details] [diff] [review]
wip v1

Does this change actually fix it? It seems a harmless enough change (except for causing redundant work), but doesn't really seem like it should fix the problem. If it does, perhaps it's a symptom of a deeper cause?
The reason why it breaks now is because the newly opened tab is not part of any group, so when activating panorama, there's no active group set (nor active orphan tab) when linking the new tab. The patch relaxes the requirement of the active group being the selected tab to be that of visible tabs.
Priority: -- → P1
I suppose the real fix is to not create these orphan tabs and always put them in a group. And if we don't think we have have an active group, we'll need to somehow pick one.
(In reply to comment #17)
> I suppose the real fix is to not create these orphan tabs and always put them
> in a group. And if we don't think we have have an active group, we'll need to
> somehow pick one.

Agreed. bug 595893
Comment on attachment 472799 [details] [diff] [review]
wip v1

Ok, gotcha; nice catch.
Attachment #472799 - Flags: review?(dietrich)
Attachment #472799 - Flags: feedback?(ian)
Attachment #472799 - Flags: feedback+
Blocks: 594094
Attached patch v1 (obsolete) — Splinter Review
Adding test for r?
Assignee: nobody → edilee
Attachment #472799 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #474925 - Flags: review?(dietrich)
Attachment #472799 - Flags: review?(dietrich)
Attached patch v1.1 (obsolete) — Splinter Review
Use about: instead of about:config (switched to :config for debugging earlier)
Attachment #474925 - Attachment is obsolete: true
Attachment #474926 - Flags: review?(dietrich)
Attachment #474925 - Flags: review?(dietrich)
Attachment #474926 - Flags: review?(dietrich)
Attachment #474926 - Flags: review+
Attachment #474926 - Flags: approval2.0+
Attached patch for checkin (obsolete) — Splinter Review
Attachment #474926 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/a6b06ebcbae2
Instead of only setting the active group on reconnect of the selected tab, do so for any visible tabs.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Keywords: helpwanted
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b6
Backed out: http://hg.mozilla.org/mozilla-central/rev/e9fbcd30350b

This showed up on Linux debug and Linux 64 debug. The test failure in browser_privatebrowsing_protocolhandler.js showed up on Linux 64 opt.

s: talos-r3-fed-007
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 373235 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 140 instances of AtomImpl with size 20 bytes each (2800 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of BackstagePass with size 24 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of CSSImportRuleImpl with size 48 bytes each (96 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 46 instances of CSSImportantRule with size 16 bytes each (736 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 11 instances of CSSNameSpaceRuleImpl with size 44 bytes each (484 bytes total)
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_protocolhandler.js | application timed out after 330 seconds with no output
PROCESS-CRASH | chrome://mochikit/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_protocolhandler.js | application crashed (minidump found)
Thread 0 (crashed)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | missing output line for total leaks!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Duplicate of this bug: 588846
This test failure appears to be the same as the one for bug 587990; should probably have the same fix. See Ehsan's comment:

https://bugzilla.mozilla.org/show_bug.cgi?id=587990#c45
Perhaps use the same sort of window close observation Sean just now used in the test for bug 595236?
Assignee: edilee → raymond
Raymond probably already knows, but bug 587990 implements what I have in this patch and more. All that this patch really adds then is a test, but it seems to fail with the test timeout.
Depends on: 587990
Attached patch v1.2Splinter Review
Attachment #475104 - Attachment is obsolete: true
Attachment #475740 - Flags: feedback?(edilee)
Still not sure why the various attempts to get the test passing aren't working. The main code fix came with the dependent bug so all that's left here is landing the test if it starts working.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: fixed by bug 587990
verified with recent builds of minefield
Status: RESOLVED → VERIFIED
Let's keep an eye out for this one, because I got the pesky mini-tabs cornered on the top left of my Panorama view after I restarted after applying today's nightly on Mac.

It didn't happen on my Windows 7 machine.
"I got the pesky mini-tabs cornered
on the top left of my Panorama view after I restarted after applying today's
nightly" -> happened to me yesterday and today on my Win7 machine...
Attachment #475740 - Flags: feedback?(edilee)
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.