603721 - Creating a new tab in a tab group which only contains app tabs sometimes causes the group to be deleted

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Justin Lebar (not reading bugmail)]

STR:
 * Create two app tabs.
 * Create a new tab pointing to google.com.
 * Open Panorama, create a new tab group, and focus it by clicking the favicon of one of your two app tabs.  Now you should only see your two app tabs.
 * Press ctrl+t to open a new tab.  Close it with ctrl+w.
 * Press ctrl+t again.

Expected results:
 * Same as the first time you pressed ctrl+t, the new tab (and only the new tab) appears.  Panorama shows that we still have two tab groups.

Actual results:
 * The new tab appears along with the tab pointing to google.com.  Panorama shows that we only have one tab group.

Comment 1

[juan becerra [:juanb]]

While using the latest nightly from 10/12, I can reproduce almost identically, except that in * 3 I see my two app tabs and the google tab as well.

Comment 2

[Aza Raskin [:aza]]

When there are app tabs, we shouldn't have groups with no tabs die.

Comment 3

[Ian Gilman [:iangilman]]

(In reply to comment #2)
> When there are app tabs, we shouldn't have groups with no tabs die.

I disagree; this is covered by bug 596781.

Comment 4

[Ian Gilman [:iangilman]]

(In reply to comment #3)
> (In reply to comment #2)
> > When there are app tabs, we shouldn't have groups with no tabs die.
> 
> I disagree; this is covered by bug 596781.

Actually, I believe they shouldn't auto-close if you're in the group, but they should if you're in the Panorama UI.

Comment 5

[Ian Gilman [:iangilman]]

Attachment: patch v1

Comment 6

[Ian Gilman [:iangilman]]

Comment on attachment 487430 [details] [diff] [review]
patch v1

>-      if (!this._children.length && !this.locked.close && !this.getTitle() && !options.dontClose) {
>+      if (!this._children.length && 
>+          !this.locked.close && 
>+          !this.getTitle() && 
>+          !options.dontClose &&
>+          (UI._isTabViewVisible() || !gBrowser._numPinnedTabs)) {

That last line is the crux of the fix. The rest of the patch is some overdue cleanup that was necessary in order to get the test to pass.

Comment 7

[Raymond Lee [:raymondlee]]

Comment on attachment 487430 [details] [diff] [review]
patch v1

Looks good.

Not related to this bug.  I don't see the locked.close would be TRUE in the code. Does we still need that?   
+          !this.locked.close &&

Comment 8

[Ian Gilman [:iangilman]]

(In reply to comment #7)
> Not related to this bug.  I don't see the locked.close would be TRUE in the
> code. Does we still need that?   
> +          !this.locked.close &&

In fact, locked isn't used at all any more. Filed bug 609163.

Comment 9

[Ian Gilman [:iangilman]]

This patch requires the changes in bug 600665.

Comment 10

[Justin Dolske [:Dolske]]

Comment on attachment 487430 [details] [diff] [review]
patch v1

>+    this._eventListeners.attrModified = function(tab) {
>+      if (tab.ownerDocument.defaultView != gWindow)
>+        return;

Hmm, I see this in a few spots (in existing code). How would the tab's window not be the browser's window? Seems like that would imply event listeners in one window are getting events from another window, which shouldn't ever happen?

Comment 11

[Ian Gilman [:iangilman]]

(In reply to comment #10)
> Comment on attachment 487430 [details] [diff] [review]
> patch v1
> 
> >+    this._eventListeners.attrModified = function(tab) {
> >+      if (tab.ownerDocument.defaultView != gWindow)
> >+        return;
> 
> Hmm, I see this in a few spots (in existing code). How would the tab's window
> not be the browser's window? Seems like that would imply event listeners in one
> window are getting events from another window, which shouldn't ever happen?

We're using AllTabs.jsm which by design gives you events from all windows.

Comment 12

[Kevin Hanes]

Beta8 has 1 bug left on it, moving our blockers to b9

Comment 13

[Kevin Hanes]

bugspam (moving b9 to b10)

Comment 14

[Kevin Hanes]

bugspam (removing b9)

Comment 16

[Michael Yoshitaka Erlewine [:mitcho]]

softblocker = critical

Comment 17

[Ian Gilman [:iangilman]]

Attachment: patch v2

So I wrote this patch on top of bug 600665, but it's pretty evident that that bug isn't going to land for Fx4, so now it needs to be made to be independent to that patch. In addition it's pretty old, so there's probably some good old fashioned rot as well.

At any rate, I'm obviously not getting around to it (currently buried in reviews for the foreseeable future), so I'm attaching the current state in the hopes that someone (tim?) can take it on home.

Comment 18

[Michael Yoshitaka Erlewine [:mitcho]]

Opening this blocker up.

Comment 19

[Ian Gilman [:iangilman]]

We believe this bug will be fixed by bug 612470.

Comment 20

[Kevin Hanes]

bugspam. Moving b10 to b11

Comment 21

[Kevin Hanes]

bugspam. Removing b10

Comment 22

[Michael Yoshitaka Erlewine [:mitcho]]

Taking this, just so it's not lonely, but if someone wants it, let me know.

Comment 23

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Video: this is fixed by bug 612470

I verified that the patch to bug 612470 (which should be ready to land soon) fixes this.

What should we do in this situation? Should we dupe this bug to that one, or create just a test with this bug's STR?

Comment 24

[Patrick Walton (:pcwalton)]

Going to mark this fixed now that bug 612470 has landed. Please reopen if the bug resurfaces.

http://hg.mozilla.org/mozilla-central/rev/ea8bf490e66d

Comment 25

[u279076]

Verified fixed with Firefox 4b11build3