365477 - Tab bar can flicker between overflow/no overflow

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[dolphinling]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a2pre) Gecko/20061230 Minefield/3.0a2pre ID:2006123004 [cairo]

The tab bar in firefox can flicker between overflowing and not overflowing. Reproducible, but seems to be a pretty rare occurrence. Closing a tab fixes it, and reopening a tab does not restart it.

I can consistently get it to flicker by having a window that's 962 pixels wide (content area, as reported by DOMI on about:blank's HTML element) and opening 19 tabs at once (using linky) while only having one currently open.

I'm on linux, using gnome, using the default firefox theme.

browser.tabs.closeButtons is set to 3
browser.tabs.tabClipWidth is set to 1023
browser.tabs.tabMinWidth is set to 45

Comment 1

[Tuukka Tolvanen (sp3000)]

I've seen this with the above prefs at defaults fwiw, recent linux trunk

Comment 2

[Martijn Wargers (dead)]

When did this regress?
I suspect this could be a regression from bug 363933, somehow.
I just filed a similar bug like this, bug 365592.

Comment 3

[Mats Palmgren (inactive)]

It's indeed a regression from bug 363933, which caused a few more events
(correctly) than we had before. The tab scrolling mechanism uses these
events, but they also flush layout events in the event handler which
together with the fact that they fail to check the orientation of the
event causes an oscillating effect in some cases.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/content/widgets/scrollbox.xml&rev=1.22&root=/cvsroot&mark=179#166
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/xul/base/src/nsScrollBoxObject.cpp&rev=1.25&root=/cvsroot&mark=385-386#381

Comment 4

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 5

[Boris Zbarsky [:bzbarsky]]

I don't know this code nearly as well as dbaron and roc do.  I can try to do the review, but I'm still catching up on being gone for 4 weeks, so I'm not likely to get to it until at least a week and a half from now...

Comment 6

[Boris Zbarsky [:bzbarsky]]

I really do think asking roc to do this review is the way to go.

Comment 7

[(not reading, please use [e-mail] instead)]

see also bug #366525, which has some steps to reproduce.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    if (frame != oldFrame) {
+      // The event handler destroyed us.
+      return rv;
+    }

This is not safe, a new frame could be recreated at the same address.

How about we make FireScrollPortEvent fire just one DOM event each time it's called. If we need both a horizontal and a vertical DOM event, re-post the nsAsyncScrollPortEvent, update mVerticalOverflow, then fire the DOM event. The next time FireScrollPortEvent runs, if nothing's changed it won't need to fire a vertical DOM event so it will fire the horizontal DOM event instead. This way, we don't need to worry about the frame or anything else being destroyed during the DOM event.

Why did you move the call to PostOverflowEvents in nsXULScrollFrame?

Comment 9

[Boris Zbarsky [:bzbarsky]]

You could also use an nsWeakFrame on the stack to detect frame destruction

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We could, but I think of nsWeakFrame as a last-resort approach

Comment 11

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

> Why did you move the call to PostOverflowEvents in nsXULScrollFrame?

nsXULScrollFrame::Layout() calls LayoutScrollArea() one or more times -
it's unnecessary to call PostOverflowEvents() more than once though.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 252153 [details] [diff] [review]
Patch rev. 2

I really like this new approach :-)

Rename PostOverflowEvents to PostOverflowEvent.

Instead of three occurrences of
+    message = mVerticalOverflow ? NS_SCROLLPORT_OVERFLOW :
+                                  NS_SCROLLPORT_UNDERFLOW;
just change message to "PRBool isOverflow" and then compute the message number when you construct the event.

Comment 13

[Mats Palmgren (inactive)]

(In reply to comment #12)
> just change message to "PRBool isOverflow"

Ok, but now looking how that turned out I see that we don't really need it.
How do you feel about:

  nsScrollPortEvent event(PR_TRUE,
                          (orient == nsScrollPortEvent::horizontal ?
                           mHorizontalOverflow : mVerticalOverflow) ?
                            NS_SCROLLPORT_OVERFLOW : NS_SCROLLPORT_UNDERFLOW,
                          nsnull);

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Fine with me

Comment 15

[Mats Palmgren (inactive)]

Attachment: Updated according to comment 12/13

Checked in to trunk at 2007-01-27 14:50 PST.

-> FIXED

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Mats, I'm seeing a lot of page reflows happening under the call to FlushPendingNotifications added here. I think we do an iniital reflow of the document when it's empty, which posts an overflow event, and then by the time we come to process that event we've created some frames for the page content which then get reflowed.

This isn't necessarily a problem but it's an unexpected side effect ...