Closed Bug 603721 Opened 14 years ago Closed 13 years ago

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

Categories

(Firefox Graveyard :: Panorama, defect, P2)

x86_64
Linux
defect

Tracking

(blocking2.0 final+)

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

People

(Reporter: justin.lebar+bug, Assigned: mitcho)

References

Details

(Keywords: dataloss, Whiteboard: [softblocker][fx4-fixed-bugday])

Attachments

(1 file, 2 obsolete files)

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.
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.
blocking2.0: --- → ?
When there are app tabs, we shouldn't have groups with no tabs die.
Blocks: 597043
Priority: -- → P2
Target Milestone: --- → Firefox 4.0b8
(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.
Assignee: nobody → ian
(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.
Status: NEW → ASSIGNED
Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #487430 - Flags: feedback?(raymond)
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 &&
Attachment #487430 - Flags: feedback?(raymond) → feedback+
(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.
Attachment #487430 - Flags: review?(dolske)
Blocks: 588597
No longer blocks: 588597
This patch requires the changes in bug 600665.
Depends on: 600665
Blocks: 605935
blocking2.0: ? → final+
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?
Attachment #487430 - Flags: review?(dolske) → review+
(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.
No longer blocks: 605935
Blocks: 598154
Beta8 has 1 bug left on it, moving our blockers to b9
No longer blocks: 597043
Keywords: dataloss
bugspam (moving b9 to b10)
Blocks: 608028
bugspam (removing b9)
No longer blocks: 598154
Whiteboard: [softblocker]
softblocker = critical
Severity: normal → critical
Severity: critical → normal
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #487430 - Attachment is obsolete: true
Opening this blocker up.
Assignee: ian → nobody
Status: ASSIGNED → NEW
We believe this bug will be fixed by bug 612470.
bugspam. Moving b10 to b11
Blocks: 627096
bugspam. Removing b10
No longer blocks: 608028
Assignee: nobody → mitcho
Taking this, just so it's not lonely, but if someone wants it, let me know.
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?
Attachment #505468 - Attachment is obsolete: true
No longer depends on: 600665
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
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Verified fixed with Firefox 4b11build3
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.