585785 - Tab closing animation fails if the opening animation hasn't moved yet

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Andreas Jung]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b4pre) Gecko/20100809 Minefield/4.0b4pre
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b4pre) Gecko/20100809 Minefield/4.0b4pre

It seems the fadein attribute is not set on some tabs.
These Tabs are displayed with a width of 1px, because of:
browser.css, line 35:
.tabbrowser-tab:not([pinned]):not([fadein]) {
  max-width: 1px;
  min-width: 1px;
}
It is possible to move these tabs, but not possible to detach them


Reproducible: Sometimes

Steps to Reproduce:
1. Open tabs (in different ways)
   (Tabs created by Flash seem to cause this bug more reliably...)

Actual Results:  
Tabs with 1px width are created


Expected Results:  
Tabs should have the usual width

Comment 1

[Andreas Jung]

Attachment: Screenshot

Comment 2

[Andreas Jung]

> It is possible to move these tabs, but not possible to detach them
It's also not possible to close them.

Adding the fadein attribute with DOM Inspector makes the tabs appear, but the problems mentioned above persist.

Comment 3

[tabmix.onemen]

look like this problem occurred because we don't pass the tab item to setTimeout

            if (aSkipAnimation ||
                this.tabContainer.getAttribute("overflow") == "true" ||
                !Services.prefs.getBoolPref("browser.tabs.animate")) {
              t.setAttribute("fadein", "true");
-             setTimeout(function (tabContainer) {
-               tabContainer._handleNewTab(t);
-             }, 0, this.tabContainer);
+             setTimeout(function (tabContainer, tab) {
+               tabContainer._handleNewTab(tab);
+             }, 0, this.tabContainer, t);
            } else {
-             setTimeout(function (tabContainer) {
-               if (t.pinned)
-                 tabContainer._handleNewTab(t);
-               else
-                 t.setAttribute("fadein", "true");
-             }, 0, this.tabContainer);
+             setTimeout(function (tabContainer, tab) {
+               if (tab.pinned)
+                 tabContainer._handleNewTab(tab);
+               else
+                 tab.setAttribute("fadein", "true");
+             }, 0, this.tabContainer, t);
            }

I can't post a patch

Comment 4

[Dão Gottwald [:dao]]

(In reply to comment #3)
> look like this problem occurred because we don't pass the tab item to
> setTimeout

Any idea why this would make a difference? Doesn't seem to make sense to me...

Comment 5

[Andreas Jung]

There seem to be two ways to end up with these broken tabs:
1) When a tab is opened:
   The tab is created, but the fadein attribute is never set. (described above)
2) When a tab is closed:
   The fadein attribute is removed, but the tab is never destroyed / 
   completely closed. (see also bug 380960 comment 100)

It seems the latter case can be provoked by closing several tabs in fast succession.

Comment 6

[tabmix.onemen]

(In reply to comment #4)
> (In reply to comment #3)
> > look like this problem occurred because we don't pass the tab item to
> > setTimeout
> 
> Any idea why this would make a difference? Doesn't seem to make sense to me...

when many tabs open fast, the after the timeout t in addtab can be another tab then the one that call the timeout

Comment 7

[Dão Gottwald [:dao]]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > look like this problem occurred because we don't pass the tab item to
> > > setTimeout
> > 
> > Any idea why this would make a difference? Doesn't seem to make sense to me...
> 
> when many tabs open fast, the after the timeout t in addtab can be another tab
> then the one that call the timeout

t is declared above that code within addTab. It's static within each addTab call and shouldn't change in closures. At least, this is how things /should/ work, as far as I know.

Comment 8

[Dão Gottwald [:dao]]

Attachment: workaround

Avoiding the closure is a good thing anyway, so I guess we should just try this. I can't tell right now if this will help, as I couldn't reproduce this bug.

Comment 9

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

(In reply to comment #3)
> look like this problem occurred because we don't pass the tab item to
> setTimeout

(In reply to comment #6)
> when many tabs open fast, the after the timeout t in addtab can be another tab
> then the one that call the timeout

Are these guesses, or actual observations from debugging? Can you reliably reproduce this issue? Does Dao's patch fix it?

The current code should be working fine from what I can see, so I don't like the idea of taking shots in the dark without trying to understand this better. If the closure is failing to produce the correct tab, then that would be a core bug that we need to fix.

Comment 10

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

This might actually just be bug 583757.

Comment 11

[Dão Gottwald [:dao]]

Yes, this should be working, but there's no reason to use a closure either.

Comment 12

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

Was this fixed by bug 583757? Bug 588981 seems to indicate otherwise...

Comment 14

[[not reading bugmail]]

I just had this happen again.  About:config is once again 1 px.

Comment 19

[Christian Ascheberg]

I can reliably reproduce this bug by quickly pressing CTRL+T (to open a new tab) and then CTRL+W (to close that new tab). For me this really happens all the time, I just need to to this so fast that the new tab is barely showing up at all.

Comment 21

[Christian Ascheberg]

(In reply to comment #19)

This is to reproduce a problem with tabs shrinking to 1px after closing them, as described in bug 597366. I am not sure if that is discussed in this bug at all, but the existing duplicate bugs (like bug 591650) made me think that. So is it, or is it not?

Comment 22

[Filip Zakrzewski]

Yes it is
bug 597366 is a duplicate

Comment 23

[Filip Zakrzewski]

Another part of weird problem.
Plus button (to open new tab) is impossible to click on if there are some ghost tabs

Comment 24

[ronin.achilles]

(In reply to comment #23)
> Another part of weird problem.
> Plus button (to open new tab) is impossible to click on if there are some ghost
> tabs

Confirmed... happens everytime with the Ghost tabs around.

Comment 25

[Johannes Pfrang [:johnp]]

I can reproduce it by fast pressing CTRL+T and CTRL+W too.
Normally have disabled the new tab button in tab bar, so I re-added it with TMP to reproduce it's behaviour, but after re-adding the new tab button all ghost tabs got their normal size but without any text and still uncloseable, but i also can't reproduce the ghost tabs any more !?

By the way bug 533924 comment 2 and 3 are about the same problem.
Hope this gets fixed soon.

Comment 27

[Andreas Gal :gal]

Why is this a JS bug?

Comment 28

[Dão Gottwald [:dao]]

(In reply to comment #27)
> Why is this a JS bug?

There's nothing wrong in the frontend code as far as I can see. Avoiding the closures mentioned in comment 3 supposedly makes the bug go away.

Comment 29

[Boris Zbarsky [:bzbarsky]]

In the failure mode, is t undefined, then?  I agree that shouldn't happen, if so...

Comment 30

[Dão Gottwald [:dao]]

(In reply to comment #29)
> In the failure mode, is t undefined, then?

I guess so. Unfortunately I don't know how to reproduce this bug.

Comment 31

[David Mandelin [:dmandelin]]

1. Can anyone reproduce this now?

2. If yes to #1, could you bisect this using Tracemonkey nightlies?

3. Also if yes to #1, please repost STR. I tried pressing Ctrl+T,Ctrl+W quickly, but nothing special happened. I'm not sure if I was supposed to get the browser in a certain state first, and I'm not sure exactly what's supposed to happen.

Comment 33

[Filip Zakrzewski]

@David Mandelin - for my fast pressing Ctrl+T and Ctrl+W creates "ghost tabs"
only addon i have is :
adblock plus 1.3a (always up-to-date)
addon compatibility reporter (always up-to-date)

Comment 34

[Christian Ascheberg]

(In reply to comment #31)
> 1. Can anyone reproduce this now?

I am using Windows Vista and (on the same PC) Win 7 as a VM, and I can still reproduce it on both using latest-trunk.

> 2. If yes to #1, could you bisect this using Tracemonkey nightlies?

I just installed latest-tracemonkey on my Win 7 VM and I can still reproduce it.

> 3. Also if yes to #1, please repost STR. I tried pressing Ctrl+T,Ctrl+W
> quickly, but nothing special happened. I'm not sure if I was supposed to get
> the browser in a certain state first, and I'm not sure exactly what's supposed
> to happen.

STR: still by pressing Ctrl+T -> Ctrl+W (do not even release Ctrl and press T W nearly at the same time, a few times)
clean profile, first startup, works in every situation for me

What happens, for example: It looks as if there is only one tab open (close other visible tabs first), but when you exit Firefox, the "Save and Quit" dialog comes up. Starting Firefox again then restores more than one tab. The "ghost tab" itself might be hard to notice.

Is there a build containing the workaround?

Comment 35

[Andreas Jung]

Yes, this bug is still reproducible.

STR didn't change, to open or close tabs as fast as possible is the probably easiest way.
Sometimes this bug is reproducible by clicking on flash ads, trying to close a tab that didn't finish opening (i.e. is still expanding), or adding/closing tabs while the browser is busy (i.e. high CPU usage), or it just happens randomly, but it is luck to reproduce it this way.

The problem with these STR is that it seems that it is not always possible to reproduce the bug with them. If that's the case continue browsing normally and try it again later.

Since this bug was reported by me 2 months ago, the screenshot isn't completely valid anymore, because these ghost tabs don't appear in the "List all tabs" menu anymore.
They only appear on the tab bar as vertical dark lines and sometimes, though very rarely, in TabCandy.

The ghost tab can be moved around, but cannot be closed and cannot be accessed (Except it's the rare case that one of them appears in TabCandy: In this case you can click on it in TabCandy to access it).

If there are ghost tabs and one regular tab open, Firefox will complain on exit, that more than one tab is open and when you choose "Save and Quit", the regular tab and the ghost tabs will be restored on next start as regular tabs.

Comment 36

[David Mandelin [:dmandelin]]

OK, I think I get it now.

STR: (On Win7) Press Ctrl+T,Ctrl+W rapidly and repeatedly.

Expected result: If pressing fast enough, pretty much nothing should happen.

Actual result: If you look closely at the "+" tab at the right, you will see it moving to the right about a pixel on each Ctrl+T and then back left on Ctrl+W. If you repeat the sequence for a while, when you are done you will notice that the space between the last tab and the "+" is slightly bigger than the space between the other adjacent pairs of tabs. The extra space is the "ghost tab". It doesn't show up in the tabs list, but it can be moved within the tab strip like a regular tab.

----

Now, if someone wouldn't mind pinpointing the first TM nightly build on which this occurs, that would be really helpful.

Comment 37

[Christian Ascheberg]

(In reply to comment #36)
> Now, if someone wouldn't mind pinpointing the first TM nightly build on which
> this occurs, that would be really helpful.

I downloaded firefox-4.0b*pre.en-US.win32.zip

2010-08-06-04-mozilla-central / 2010-08-07-04-tracemonkey
- could not reproduce

2010-08-07-04-mozilla-central / 2010-08-08-04-tracemonkey
- easy to reproduce (those have closing tab animation)

Comment 38

[David Mandelin [:dmandelin]]

(In reply to comment #37)
> (In reply to comment #36)
> > Now, if someone wouldn't mind pinpointing the first TM nightly build on which
> > this occurs, that would be really helpful.
> 
> I downloaded firefox-4.0b*pre.en-US.win32.zip
> 
> 2010-08-06-04-mozilla-central / 2010-08-07-04-tracemonkey
> - could not reproduce
> 
> 2010-08-07-04-mozilla-central / 2010-08-08-04-tracemonkey
> - easy to reproduce (those have closing tab animation)

Thanks! It's clear that the regressing change hit m-c before TM, so it is probably not JS (although conceivable some new code exposed an old bug), and closing tab animation looks like the most closely related change to me, too.

Comment 39

[Dão Gottwald [:dao]]

(In reply to comment #36)
> OK, I think I get it now.
> 
> STR: (On Win7) Press Ctrl+T,Ctrl+W rapidly and repeatedly.

I can reproduce it with these steps. The problem here is that we're not getting the transitionend event for the shrinking tab when the transition started before the opening transition finished. Not sure if that's expected. I could work around it.

I'm not sure this covers all aspects of this bug either; it doesn't seem like people always followed the above steps when they got the 1px wide tabs.

Comment 40

[Dão Gottwald [:dao]]

Attachment: another fix or workaround

Per comment 39, this is solely for the steps in comment 36.

Comment 41

[Dão Gottwald [:dao]]

(In reply to comment #39)
> (In reply to comment #36)
> > OK, I think I get it now.
> > 
> > STR: (On Win7) Press Ctrl+T,Ctrl+W rapidly and repeatedly.
> 
> I can reproduce it with these steps. The problem here is that we're not getting
> the transitionend event for the shrinking tab when the transition started
> before the opening transition finished. Not sure if that's expected.

dbaron, can you shed some light on this?

Comment 42

[Luke S]

(In reply to comment #39)
> I'm not sure this covers all aspects of this bug either; it doesn't seem like
> people always followed the above steps when they got the 1px wide tabs.

Correct: it seems to be able to happen whenever the closing tab animation occurs, though the chance is low.

The only thing about the ctrl-T,ctrl-W method is a way to speed up triggering the closing tab animation.

It's true that the animation might ALSO be more likely to trigger the bug when executed rapidly like that. However, that's certainly not the only circumstances under which it can trigger the bug.

Comment 43

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

Comment on attachment 464732 [details] [diff] [review]
workaround

This might have been relevant when bug 583757 existed, but I think it was mostly just confusing things, and the main issue is the one fixed by the new workaround (and caused by bug 380960).

Comment 44

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #41)
> (In reply to comment #39)
> > (In reply to comment #36)
> > > OK, I think I get it now.
> > > 
> > > STR: (On Win7) Press Ctrl+T,Ctrl+W rapidly and repeatedly.
> > 
> > I can reproduce it with these steps. The problem here is that we're not getting
> > the transitionend event for the shrinking tab when the transition started
> > before the opening transition finished. Not sure if that's expected.
> 
> dbaron, can you shed some light on this?

transitionend isn't fired when the transition is interrupted by another style change.  That's what the spec says to do.

We should probably add an additional event for transitionchanged (or cancelled, or something).  (I think WebKit has implemented more events than those in the spec.)

Comment 45

[Dão Gottwald [:dao]]

(In reply to comment #44)
> transitionend isn't fired when the transition is interrupted by another style
> change.  That's what the spec says to do.

It's not an arbitrary style change, though, but a new transition, for which we don't get the event either. Still expected?

Comment 46

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

You should get an end event for the new transition assuming there is a new transition.

But if the first transition hasn't even started moving yet and you set the style to exactly where it was, there won't be a second transition at all.

Comment 47

[Dão Gottwald [:dao]]

Comment on attachment 484177 [details] [diff] [review]
another fix or workaround

It surprises me that this can be triggered this easily, but like David said it seems that in the failure cases, the fade-in transition effectively hasn't started yet, so the max-width is still 1px and thus can't make a transition back down.

Comment 48

[Dão Gottwald [:dao]]

Attachment: patch

Comment 49

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

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

Add a "/* transition hasn't started yet */" comment?

I think a transitionchange/cancelled event would be a cleaner solution - followup bug(s)?

You can actually reproduce this programmatically using:
var tab = gBrowser.addTab();
executeSoon(function () {
  gBrowser.removeTab(tab, {animate:true});
});

which means this should be testable... Though I don't think there's a good way to know when to check whether the tab was actually removed (i.e. transitionend fired), which I suppose is kind of annoying.

Comment 50

[Dão Gottwald [:dao]]

(In reply to comment #49)
> I think a transitionchange/cancelled event would be a cleaner solution -
> followup bug(s)?

I'm not sure how that would help. The problem is not that we're cancelling the fade-in transition but that the fade-out transition has nothing to do...

Comment 51

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

We are cancelling it, by quickly undoing the change that would otherwise trigger it. The backend should be able to detect that and notify us.

Comment 52

[Dão Gottwald [:dao]]

(In reply to comment #51)
> We are cancelling it, by quickly undoing the change that would otherwise
> trigger it.

There's a difference between "cancelled before it could move" and "cancelled somewhere in the middle". Only the former is problematic, but an event exclusively for that case seems a bit strange to me.

Comment 53

[Dão Gottwald [:dao]]

Attachment: test added

Comment 54

[Dão Gottwald [:dao]]

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

I have seen a few 1px-wide tabs not using the STR from comment 36 a week or so ago but not since then. Optimistically marking fixed. I'll hold off landing bug 604735, as that would make it harder to observe the bug.

Comment 55

[Dão Gottwald [:dao]]

(In reply to comment #54)
> http://hg.mozilla.org/mozilla-central/rev/b6c9c4833613
> 
> I have seen a few 1px-wide tabs not using the STR from comment 36 a week or so
> ago but not since then. Optimistically marking fixed. I'll hold off landing bug
> 604735, as that would make it harder to observe the bug.

It just happened again, see bug 608589.