Last Comment Bug 353686 - Firefox crashes when gtk_key_theme (gconf) changes [@ nsLookAndFeel::InitColors]
: Firefox crashes when gtk_key_theme (gconf) changes [@ nsLookAndFeel::InitColors]
Status: RESOLVED FIXED
: crash, fixed1.8.0.8, fixed1.8.1
Product: Core
Classification: Components
Component: Widget: Gtk (show other bugs)
: Trunk
: x86 Linux
: -- critical (vote)
: mozilla1.9alpha1
Assigned To: Adam Guthrie
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-21 11:42 PDT by Matthias Clasen
Modified: 2011-06-09 14:58 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
stacktrace (9.07 KB, text/plain)
2006-09-21 18:50 PDT, Matthias Clasen
no flags Details
Remove callback for gtk-key-theme-change (1.36 KB, patch)
2006-09-21 22:10 PDT, Adam Guthrie
dbaron: review+
dbaron: superreview+
dveditz: approval1.8.0.8+
mconnor: approval1.8.1+
Details | Diff | Review

Description Matthias Clasen 2006-09-21 11:42:52 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.6) Gecko/20060828 Fedora/1.5.0.6-8 Firefox/1.5.0.6 pango-text
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.6) Gecko/20060828 Fedora/1.5.0.6-8 Firefox/1.5.0.6 pango-text

Changing the value of /desktop/gnome/interface/gtk_key_theme (possible values
are Default and Emacs), firefox exits.

I haven't investigated why the crash occurs, but it seems dubious to connect
the same callback for key theme changes as for theme changes, since key theme
changes don't require any kind of screen refresh.

Just removing that callback for key theme changes should fix this.

Reproducible: Always

Steps to Reproduce:
1. start firefox
2. open gconf-editor and change /desktop/gnome/interface/gtk_key_theme


Actual Results:  
firefox crashes

Expected Results:  
firefox does not crash
Comment 1 Adam Guthrie 2006-09-21 13:08:48 PDT
So, I'm wondering if this happens in a mozilla.org build. I don't see anywhere that we explicitly watch that key.

Also, can you get a stack for the crash? You'll need to get the firefox-debug package so you'll have symbols, then start with `firefox -g -d gdb'.
Comment 2 Matthias Clasen 2006-09-21 18:50:31 PDT
Created attachment 239600 [details]
stacktrace
Comment 3 Adam Guthrie 2006-09-21 20:08:52 PDT
OK, so gtk_rc_get_style_by_paths() is probably returning NULL, although I don't know why since we're getting the style for a tooltip...

We should probably doing something more similar to this example: http://mail.gnome.org/archives/gtk-devel-list/2001-June/msg00449.html; e.g. actually creating the widget rather than explicitly passing hard-coded values to that function.
Comment 4 Matthias Clasen 2006-09-21 20:13:02 PDT
I maintain that simply not doing anything like that for key theme changes is the easiest fix for the crash.
Comment 5 Adam Guthrie 2006-09-21 20:28:18 PDT
That could be done fairly easily by commenting out this line: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/gtk2/nsWindow.cpp&rev=1.185&mark=2866-2868#2861

I'm not sure what the implications of doing this would be, though. dbaron?
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-09-21 20:37:30 PDT
Removing that is fine.  I don't think I knew what that was -- not sure where I got it from.

Still, it's not clear why it would crash.  Why don't we crash on theme changes?
Comment 7 Adam Guthrie 2006-09-21 21:12:43 PDT
I really don't know. Maybe a bug in GTK? Matthias, can you get symbols for GTK, break in gtk_rc_get_style_by_paths and see what's going on?
Comment 8 Matthias Clasen 2006-09-21 21:34:35 PDT
Thats really quite pointless. 
There is no reason to do this callback on a key theme change
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-09-21 21:48:10 PDT
Do you also crash on a theme change?
Comment 10 Matthias Clasen 2006-09-21 21:53:47 PDT
No, on theme change, firefox goes into a looong (like >1min) near-death
experience, but recovers eventually...
Comment 11 Adam Guthrie 2006-09-21 22:10:00 PDT
Created attachment 239612 [details] [diff] [review]
Remove callback for gtk-key-theme-change

OK. This removes the callback for gtk-key-theme-change(s). 

FWIW, Boris Zbarsky fixed the issue of the massive hanging when changing the GTK theme in bug 352096, but that patch is currently only on trunk.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-09-21 22:12:00 PDT
Comment on attachment 239612 [details] [diff] [review]
Remove callback for gtk-key-theme-change

r+sr=dbaron.  Thanks for fixing this.
Comment 13 Adam Guthrie 2006-09-22 10:26:59 PDT
Checked in by timeless on 2006-09-22 00:24. ->FIXED
Comment 14 timeless 2006-09-25 02:48:01 PDT
Comment on attachment 239612 [details] [diff] [review]
Remove callback for gtk-key-theme-change

mozilla/widget/src/gtk2/nsWindow.cpp 	1.186
Comment 15 Christopher Aillon (sabbatical, not receiving bugmail) 2006-09-26 17:51:41 PDT
Comment on attachment 239612 [details] [diff] [review]
Remove callback for gtk-key-theme-change

Nominating. Either way, I really really want to add this crash fix to our RPMs if it doesn't make the branches.
Comment 16 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-09-26 20:12:54 PDT
This is safe.  I think we should take it for 1.8.1.
Comment 17 Mike Connor [:mconnor] 2006-09-27 10:37:49 PDT
Comment on attachment 239612 [details] [diff] [review]
Remove callback for gtk-key-theme-change

a=mconnor on behalf of drivers for 1.8 branch checkin
Comment 18 Daniel Veditz [:dveditz] 2006-09-29 11:39:21 PDT
Comment on attachment 239612 [details] [diff] [review]
Remove callback for gtk-key-theme-change

approved for 1.8.0 branch, a=dveditz for drivers
Comment 19 Adam Guthrie 2006-09-29 12:06:15 PDT
(Eek, I guess it's my responsibility to get this checked in now.)
Comment 20 Adam Guthrie 2006-09-29 14:35:16 PDT
Comment on attachment 239612 [details] [diff] [review]
Remove callback for gtk-key-theme-change

This got approved on 2006-09-27, but didn't get checked in. dbaron recommended I re-request approval.

This is a simple GTK-only patch that Red Hat wants to get in for Firefox 2.
Comment 21 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-09-29 17:52:01 PDT
Checked in to MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH.  (Forgot to change the name of the approver in the checkin comment for the latter; oops.)

Note You need to log in before you can comment on or make changes to this bug.