311136 - Tab drag&drop can confuse tabbrowser

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Simon Bünzli]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051004 Firefox/1.4.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051004 Firefox/1.4.1

If you heavily drag and drop tabs around, it can happen that tabbrowser loses
count of the tabs and behaves strangely. Symptoms include: a tab can't be moved
to the end; the tab at the end can't be closed with Ctrl+W; when closing the
last tab, a warning about closing 2 tabs is shown; the tabbar can get
"disconnected" from the browser.

This has been reproduced on today's nightly, on a clean profile, in safe-mode.
So it's surely not related to extension problems (no dupe of the FLST bug 304456).

Reproducible: Always

Steps to Reproduce:
1. Open your homepage and two additional (blank) tabs.
2. Move the third tab to the start and close it.
3. Move the tab with your homepage to the end.
4. Close both tabs with Ctrl+W.
Actual Results:  
Step 3 fails and step 4 results in the warning about closing multiple tabs (if
it works at all). Occasionally, after step 3 the browser is not usable anymore,
since the tabbar seems disconnected (not tabbable, no active tab).

Expected Results:  
Steps 3 and 4 succeed without warnings or errors in the console (I've seen once:
Error: uncaught exception: 2147500037 and some errors in tabbrowser.xml due to
the absence of a tab - such as failing to call removeTab).

Comment 1

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051004
Firefox/1.4.1 ID:2005100419

Step 3 fails indeed. 
Step 4: With browser.tabs.autoHide to false Firefox refused to react to Ctr+W
(no problem with browser.tabs.autoHide to true), but in all cases I got the
CloseTab warning when I wanted to close Firefox, although there were no tabs
open anymore.
Tested on a new profile.

Errors after Ctr+W and after attempting to close Firefox: 
Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIWebProgress.removeProgressListener]" 
nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
chrome://global/content/bindings/tabbrowser.xml :: removeTab :: line 1220" 
data: no]

Comment 2

[Ria Klaassen (not reading all bugmail)]

Not in trunk at first sight.

Regression between 1.8b5_2005100321 and 1.8b5_2005100405. 

Comment 3

[Simon Bünzli]

Still present in today's build. Marking as regression, requesting blocking of
the next release (presumably RC1) and CCing Dorando since it's his code and he
might know the problem.

Comment 4

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

See bug 311204 and bug 311200.

Comment 5

[Mike Connor [:mconnor]]

Attachment: patch

You know, I don't know why this breaks now, I guess something exposed this? 
It'd be nice to know why, but this should have been in the original impl
anyway.  This seems to fix bug both 311200 and 311204 as well, but testing is
good.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

r=vladimir

Comment 7

[Mike Connor [:mconnor]]

fixed on trunk, we need this for RC1

Comment 8

[Dorando]

(In reply to comment #5)
> You know, I don't know why this breaks now, I guess something exposed this? 
It seems like the removed tabs are not fully removed from the DOM:

1. Open many tabs
2. Close all others
3. Check <tabbrowser>.mTabContainer.childNodes with DOMi

Comment 9

[Jesse Ruderman]

This patch doesn't make sense to me.  foo.insertBefore(bar, null) is defined to
be the same as foo.appendChild(bar).

Comment 10

[Jesse Ruderman]

This patch doesn't make sense to me.  foo.insertBefore(bar, null) is defined to
be the same as foo.appendChild(bar), so the original code seems correct.

Does the patch for bug 311295 fix this bug on the Gecko 1.8 branch?

Comment 11

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b5) Gecko/20051006
Firefox/1.4.1 ID:2005100600

this patch fixes This both bug 311200 and bug 311204

Comment 12

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

The patch for bug 311295 (checked in on the branch only) does fix this (and bug
311204, but not bug 311200), so as Jesse said the patch in this bug isn't needed.

Comment 13

[Jesse Ruderman]

The patch in this bug might fix a JavaScript strict warning (bug 311200).

Comment 14

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

(In reply to comment #13)
> The patch in this bug might fix a JavaScript strict warning (bug 311200).

It's more than what's needed though, see bug 311200 comment 3.

Comment 15

[Mike Connor [:mconnor]]

not needed for branch.

Comment 16

[Jesse Ruderman]

Not needed for branch because the patch in bug 311295 took care of the regression.

Comment 17

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

If the patch wasn't needed for the branch, because a branch-only regression was fixed there, then why was it needed for the trunk, where the regression (bug 311295) was never present? Can the patch be backed out?

Comment 18

[Mike Connor [:mconnor]]

Yes.

Comment 19

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

I backed out the patch, branch and trunk are now the same.