GNOME Bugzilla – Bug 521513
Firefox crash when using file picker
Last modified: 2008-03-14 11:03:56 UTC
When using the file picker dialog in Firefox 2.0.0.12 the program ends with a SEGV in glib code. This does not happen with glib 2.14.x. Currently using glib-2.15.6 on FreeBSD 8-CURRENT. Steps to reproduce: "File | Open File... | Cancel"
Created attachment 106955 [details] Backtrace
Also happens with gedit, same repro. steps.
The problem is actually for widespread, and occurs in the fam gio backend. These two traces show what is happening. When the file chooser is closed, the FAM subscription on fstab is canceled. Then the monitor is unreffed (lines 1130 and 1131 in gunixmounts.c). However, the GIO watch on the FAM connection file descriptor is not removed, so as soon as an event comes in to FAM, the callback is triggered in an unreffed object, and the crash occurs. It seems to me that the GIO watch needs to be removed when the object is unreffed. (Note: these traces were taken from gnome-terminal which suffers the same problem when the Edit Profile dialog is destroyed.)
+ Trace 191986
And this results in an instant crash. Maybe there needs to be a _fam_sub_shutdown() function in fam-helper.c which removes the GIO watch...?
Created attachment 106996 [details] [review] Remove GIO source when FAM monitor is unreffed
I've attached a patch which I believe to be correct. It does fix the crashes, and I have not observed any other negative effects.
Yep, the patch makes 2.15.6 not crash, but 2.16.0 seems broken in more horrible ways. 0>pwo@one ~> gedit [ open file picker ] (gedit:5061): GLib-GObject-CRITICAL **: g_value_dup_boxed: assertion `G_VALUE_HOLDS_BOXED (value)' failed (gedit:5061): GLib-GObject-CRITICAL **: g_value_dup_boxed: assertion `G_VALUE_HOLDS_BOXED (value)' failed (gedit:5061): GLib-GIO-CRITICAL **: g_themed_icon_constructed: assertion `themed->names != NULL && themed->names[0] != NULL' failed (gedit:5061): GLib-GIO-CRITICAL **: g_themed_icon_constructed: assertion `themed->names != NULL && themed->names[0] != NULL' failed (gedit:5061): GLib-CRITICAL **: g_strv_length: assertion `str_array != NULL' failed ... Multiple segmentation faults occurred; can't display error dialog
Note: my patch is not perfect. Even after adding locks in _fam_sub_shutdown(), the FAM file and directory monitors still compete over who will shutdown the FAM connection first. There is most likely a better way to solve this, but at least my patch quells the crash. As for glib-2.16.0, that appears to be broken in some other ways.
Two different backtraces of 2.16.0 without Marcus patch. This one the picker shows up and paints fully and then gedit crashes, or crashes when cancel is pressed in the picker: (gdb) bt
+ Trace 192021
Thread 6318608 (LWP 100140)
More investigation. The problem stems from the fact that the libgiofam.so module is unloaded when the file chooser is destroyed. Since the g_source is left active, bad things happen. Take a look. When the file chooser is opened: Breakpoint 3, dlopen (name=0xc95170 "/usr/local/lib/gio/modules/libgiofam.so", mode=1) at /usr/src/libexec/rtld-elf/rtld.c:1880 1880 wlock_release(rtld_bind_lock, lockstate); (gdb) print obj $16 = (Obj_Entry *) 0x8075c6200 (gdb) cont Continuing. So the FAM module is loaded at 0x8075c6200. Now, as soon as the chooser is destroyed by clicking on either button: Breakpoint 4, dlclose (handle=0x8075c6200) at /usr/src/libexec/rtld-elf/rtld.c:1742 1742 { (gdb) bt
+ Trace 192120
The module is unloaded. So while my patch sucks, unless the module is not unloaded, I don't see any other way of accommodating this scenario. Other ideas are certainly welcome.
One option would certainly be to make the module resident. Alex, what is your opinion ?
I have come up with a new, better patch. I don't know why I didn't think of this before. I simply call the _fam_sub_shutdown() function from the module unload function. This avoids the races, and also fixes the crash. I have attached the new patch.
Created attachment 107174 [details] [review] Make sure the g_source is removed when the module is unloaded.
The initial patch is wrong, as it doesn't work if you have multiple open monitors. However, the last patch seems very good. Applying it.