353686 - Firefox crashes when gtk_key_theme (gconf) changes [@ nsLookAndFeel::InitColors]

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Matthias Clasen]

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]

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]

Attachment: stacktrace

Comment 3

[Adam Guthrie]

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]

I maintain that simply not doing anything like that for key theme changes is the easiest fix for the crash.

Comment 5

[Adam Guthrie]

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-4, no longer working on Mozilla)]

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]

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]

Thats really quite pointless. 
There is no reason to do this callback on a key theme change

Comment 9

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

Do you also crash on a theme change?

Comment 10

[Matthias Clasen]

No, on theme change, firefox goes into a looong (like >1min) near-death
experience, but recovers eventually...

Comment 11

[Adam Guthrie]

Attachment: 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-4, no longer working on Mozilla)]

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]

Checked in by timeless on 2006-09-22 00:24. ->FIXED

Comment 14

[timeless]

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)]

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-4, no longer working on Mozilla)]

This is safe.  I think we should take it for 1.8.1.

Comment 17

[Mike Connor [:mconnor]]

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]]

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]

(Eek, I guess it's my responsibility to get this checked in now.)

Comment 20

[Adam Guthrie]

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-4, no longer working on Mozilla)]

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.)