456002 - Dragging and dropping a tab from a window with 1 tab is broken

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Damian Shaw [Quan]]

Steps to reproduce:

1. Create 2 windows of Firefox, 1 must only have 1 tab
2. Drag and drop that 1 tab to the other window of Firefox
3. Notice one tab that was originally in the 2nd window has now gone to whatever page the tab which drag and dropped had and a new blank unclosable tab has been created in the 2nd window.

Expected results:

A new tab from the 1st window should of been created.

Also the the error console says this:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebProgress.removeProgressListener]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://browser/content/tabbrowser.xml :: _beginRemoveTab :: line 1439"  data: no]

Test on Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.1b1pre) Gecko/20080918073230 Minefield/3.1b1pre ID:20080918073230 and also confirmed on Windows Vista

Comment 1

[philippe (part-time)]

Seen also with 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20080918210621 Minefield/3.1b1pre

likely regression form bug 392870

Comment 2

[Dão Gottwald [:dao]]

Seems to work for me. Are you using an extension related to the tab bar?

Comment 3

[philippe (part-time)]

(In reply to comment #2)
> Seems to work for me. Are you using an extension related to the tab bar?
Default profile.
STR
1. open a window (http://www.mozilla.org/projects/minefield/)
2. open second window, and go to whatever uri.
3. from window 1, drag tab to tab-bar in window 2

AR: 
* a new tab is created 'untitled', the contents of tab1-window2 is replaced by the contents of tab1-window1
* the 'untitled tab' cannot be closed

Comment 4

[Dão Gottwald [:dao]]

Oh yeah, I was using a modified build.

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 339788 [details] [diff] [review]
patch

>             // First, start teardown of the other browser.  Make sure to not
>-            // fire the beforeunload event in the process.
>+            // fire the beforeunload event in the process.  Close the window
>+            // if the dragged tab was the last one.

Since this isn't limited to dragging tabs, this should read: "Close the other window if this was its last tab."

Comment 8

[Tuukka Tolvanen (sp3000)]

attachment 339788 [details] [diff] [review] makes on defaults closing the last document in a window a bit slower (and certainly makes it look noticeably slower) than before by loading a blank doc before closing the window. Could you avoid that temporary tab (/ whatever else is slowing it down)?

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #8)
> attachment 339788 [details] [diff] [review] makes on defaults closing the last document in a window a bit
> slower (and certainly makes it look noticeably slower) than before by loading a
> blank doc before closing the window. Could you avoid that temporary tab (/
> whatever else is slowing it down)?

--> bug 457096

Comment 11

[Dão Gottwald [:dao]]

Gavin: ping?

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 339788 [details] [diff] [review]
patch

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>       <method name="_beginRemoveTab">

>             if (aTab.localName != "tab")
>-              aTab = this.mCurrentTab;
>+              return null;

Why this behavior change?

>+              if (closeWindow)
>+                this.addTab("about:blank");
>+              else
>+                BrowserOpenTab();

Aren't these effectively the same thing? Why is the distinction important?

Comment 14

[Dão Gottwald [:dao]]

(In reply to comment #13)
> >       <method name="_beginRemoveTab">
> 
> >             if (aTab.localName != "tab")
> >-              aTab = this.mCurrentTab;
> >+              return null;
> 
> Why this behavior change?

The current behavior doesn't seem to make sense to me and bugs me every time I look at it (and others, e.g. bz in bug 113934), but it never seemed important enough to file a bug. Do you want me to file a separate bug? Or can you explain why the code is doing what it's doing?

> >+              if (closeWindow)
> >+                this.addTab("about:blank");
> >+              else
> >+                BrowserOpenTab();
> 
> Aren't these effectively the same thing? Why is the distinction important?

BrowserOpenTab focuses the location bar, which we don't need in the former case and could cause visible jitter (as far as I remember, it actually did when I tried it). I can add a comment regarding this.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 339788 [details] [diff] [review]
patch

(In reply to comment #14)
> The current behavior doesn't seem to make sense to me and bugs me every time I
> look at it (and others, e.g. bz in bug 113934), but it never seemed important
> enough to file a bug. Do you want me to file a separate bug? Or can you explain
> why the code is doing what it's doing?

I actually misinterpreted it as a null check; I agree that it doesn't make much sense as it is. Bug 107848 comment 25 suggests that event listeners on non-tab elements used to pass |this| as the parameter, or something. I think we can probably get rid of it, but a new bug would be good.

> I can add a comment regarding this.

Sounds good.

Comment 16

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/1e91efd774f4

filed bug 461013

Comment 17

[Luke Iliffe (Harlequin99)]

If I use the STR in comment #3 At step 3 when I mouse down on the tab in order to drag it I immediately get the following in the error console:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: no]

The D&D operation is however successful in a today's nightly build containing this patch (20081022).

Comment 18

[YUKI "Piro" Hiroshi]

The patch causes a regression.
https://bugzilla.mozilla.org/show_bug.cgi?id=406216

>             // Remove this tab as the owner of any other tabs, since it's going away.
>-            for (var i = 0; i < this.mTabs.length; ++i) {
>-              var tab = this.mTabContainer.childNodes[i];
>+            for (let i = 0; i < l; ++i) {
>+              let tab = this.mTabs[i];

This section does it. The number of tabs is possibly changed by an add-on who listen "TabClose" event. "l" indicates the old number of tabs, so, "this.mTabs[i]" possibly refers invalid item.

Comment 19

[Dão Gottwald [:dao]]

(In reply to comment #18)
> The patch causes a regression.
> https://bugzilla.mozilla.org/show_bug.cgi?id=406216
> 
> >             // Remove this tab as the owner of any other tabs, since it's going away.
> >-            for (var i = 0; i < this.mTabs.length; ++i) {
> >-              var tab = this.mTabContainer.childNodes[i];
> >+            for (let i = 0; i < l; ++i) {
> >+              let tab = this.mTabs[i];
> 
> This section does it. The number of tabs is possibly changed by an add-on who
> listen "TabClose" event. "l" indicates the old number of tabs, so,
> "this.mTabs[i]" possibly refers invalid item.

Can you please file a new bug and make it blocking this one?

Comment 20

[Dão Gottwald [:dao]]

Oh, I guess that's what bug 406216 is about...

Comment 21

[Charles Fenwick]

verified

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1b3pre) Gecko/20090130 Shiretoko/3.1b3pre