352096 - [FIX]Switching GNOME theme effectively hangs app

Status: VERIFIED FIXED

(Core Graveyard :: GFX: Gtk, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1) Start our app (Firefox, Seamonkey, Thunderbird, whatever; make sure it's a
   GTK2 build)
2) Open some windows (a dozen should be good)
3) Open gnome-control-center
4) Select the "themes" item
5) Switch the current theme

EXPECTED RESULTS:  Theme changes, all is well

ACTUAL RESULTS: Theme changes, app starts taking 100% CPU.  Been that way for 10+ minutes now.  Sysprof says 25% of time in libXft, the rest of the time in libgkgfx.  Sadly, this is a release (stripped and all) build, so that's all sysprof can tell me....

I'm told that if I wait long enough the app might start responding again.  Maybe.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I should note that if, God forbid, you don't have a current GNOME theme selected, simply opening the theme dialog will hang our app.  That's what happened to me.

Comment 2

[Steve England [:stevee]]

Dupe of / related to bug 318829?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Similar for sure.  Might be a dup; hard to say.  I'll try getting this profiled and fixed; at that point we can see whether bug 318829 is still around for that reporter.  I wouldn't be surprised if there are multiple hang scenarios here...

Comment 4

[Boris Zbarsky [:bzbarsky]]

So I waited for about 3 hours, with the app completely hung. Then I went to bed; in the morning things were ok... until I made the mistake of killing the gnome-theme-manager process, at which point the app went back to being hung.  I'll probably try to get non-computer stuff done for the rest of the day, I guess, and see whether I can safely profile this somehow in the evening....

Comment 5

[Boris Zbarsky [:bzbarsky]]

So the issue here is that with a single window and single tab open on a theme change we get 8 each of "notify::gtk-theme-name" and "notify::gtk-key-theme-name" all dispatched to 8 different widgets.  This leads to 32 calls into nsPresContext::ThemeChanged, since nsWindow::ThemeChanged calls ThemeChanged on all child windows too.  In practice, this means X calls to ClearStyleDataAndReflow on the chrome prescontext, X calls to ResizeReflow on the content prescontext, and 32-X calls to ClearStyleDataAndReflow on the content prescontext.  I didn't bother to find out what X is.  The upshot is that in my case I had 100+ documents open at the time of the theme change; reflowing each of them 30+ times just ... took a while.

Patch coming up that should make this saner.  Certainly does over here.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Handle GNOME theme changes lazily.

This is much faster.  If we care, we could even suppress reflow in background tabs, but that would take a little more work... and I'm not sure that's desirable.

Comment 7

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

Comment on attachment 237895 [details] [diff] [review]
Handle GNOME theme changes lazily.

mPending* could just be PRPackedBools.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> mPending* could just be PRPackedBools.

Er... good catch.  I'll do that.  Or rather make them bits in the bitfield we already have.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 10

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 11

[Adam Guthrie]

Verified FIXED using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060912 Minefield/3.0a1.

I had about 20-30 tabs opened in one window, switched the theme and Firefox recovered in < 1 minute.

Comment 12

[Hussam Al-Tayeb]

Is it possible to get this checked in for 1.8 branch for firefox2?

Comment 13

[Boris Zbarsky [:bzbarsky]]

If someone ports it, does a bunch of testing, and then convinces the branch drivers to take it into an effectively code-frozen branch, sure.  I really don't feel it's worth the trouble, myself.

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

Considering the major linux enterprise vendors will be shipping this, and this is not an uncommon task in the desktop, I think it should be worth at the very least consideration based on the fact that this hangs the entire browser.

Further investigation in the patch leads me to believe that this is scary only in the sense that "it touches the pres context", but a quick look at the patch tells me that at the worst, all this will break potentially is that theme updates don't get propogated at all (application still uses the old theme widgetry).  That potential regression seems a smidgen better than hanging the entire application.  But I can provide testing there, facilitated by the really easy backport.  Creating a test build now with a backported patch I'll push after this is done.

Then of course, there are the platforms that don't hang, e.g. Windows (or does it?  Not sure.)  In which case better testing really *is* needed for those platforms which I am unable to provide, but hopefully others can.

With the small-ish risk involved and the considerably larger gain, I'd like to try and get this considered more seriously for branches.  Are there known regressions from the trunk?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Chris, if you have the resources to deal with this, I have no problems with getting it in on branch per se.  Just no time to do it myself.  Please request blocking of 1.8.1 or 1.8.1.1 or whatever if you'd like this to block those.

Not sure what the Windows situation is, but it looks like it avoids most of the issues (at least if it only gets notified by the OS once), since it doesn't do the recursion thing GTK does.  So I doubt there were hangs on Windows.

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch backported to Firefox 1.5.0.x

Only major differences here are the threading stuff, which needed to use the nsIEventQueue stuff.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 240193 [details] [diff] [review]
Patch backported to Firefox 1.5.0.x

Looks reasonable.

Comment 18

[Adam Guthrie]

*** Bug 356545 has been marked as a duplicate of this bug. ***

Comment 19

[Alexander Sack]

any chance to see this on the branches at some point?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Huh.  Chris never requested approval?

Poke him to do it?  Or do it yourself (with an explanation to drivers of why this should go on branches)?  I'm happy to land the patch if it gets approved.

Comment 21

[Alexander Sack]

(In reply to comment #20)
> Huh.  Chris never requested approval?
> 

Chris, any comment? Any reasons that you did not request approval?

Comment 22

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

> Poke him to do it?  Or do it yourself (with an explanation to drivers of why
> this should go on branches)?  I'm happy to land the patch if it gets approved.

Do you mean just setting the flag and writing a comment?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Yes.