GNOME Bugzilla – Bug 113655
Panel crashes when I open panel properties
Last modified: 2004-12-22 21:47:04 UTC
Hello there, I just compiled Gnome 2.3.2 from garnome and the panel _always_ crashes when I right-click on it and then select the option properties in the popup menu. The courios thing is that it happens only with my current panels, the panel at the top and the panel at the bottom of te screen. When I add a third panel it works just fine. The following is the backtrace I got:
+ Trace 37175
I hope this helps. -William
Appears to be a unique stack trace, according to the simple-dup-finder. Setting version->2.3.x, marking priority->high & severity->critical (it's a crasher), and adding bugsquad keyword.
unique stack trace?? So, this is something related only to my system?
William: Elijah was only indicating that no-one else has reported this bug yet. That's a good thing :-) Thanks. Ray: you might look at this ?
Just compiled garnome build and I can reproduce. Seems like there are two faucets to this bug. 1) The code passes a value that can conceivably be NULL straight from gconf to gtk_entry_set_text () (which doesn't like NULL values), 2) In this case the value is NULL because the background gconf key and all its subkeys aren't being created for the top panel and bottom panel. I will investigate why 2) is happening, fix it, and submit a patch for both issues.
Okay, I've played with this a bit more and I have a theory about what's happening. When I reproduced this bug, it was on a test account that I have. When I logged into garnome for the first time, the panel locked up hard and so I just killed it (I probably should have tracked down why it locked up, but i was focusing on trying to reproduce this bug so I didn't). Then when the panel restarted, I was able to reproduce this bug. When I erased my config and started it again, I could no longer reproduce the bug. This leads me to believe that problem 2) from my previously mentioned list was actually caused by me killing the panel. In other words, I think I killed it before the default setup was completely copied over to the new panels' keys. Indeed, if I erase my config, startup gnome-panel in gdb and kill it before the default setup is completely copied over, then the bug occurs the next time I start gnome-panel. William, when you ran garnome for the first time did you encounter any panel lockups, system crashes, power outages, X server crashes or anything like that (before this crash occured)? Or did your panel load without any problems the first time you logged in and the only problem you've noticed is when you go to properties? Mark, I would like to guard against half copied over keys by introducing a new GConf key. Something like "reset_config". If it's set to "yes", then the panel will automatically reset to its default setup. When it's done resetting it will set "reset_config" to "no". This way, if the panel is in the process of copying over a default setup and gnome-panel is terminated prematurely (for whatever reason), the next time the panel is started it will copy over the default setup again and so half copied keys can be avoided. This will also make it easier for kiosk type environments to reset their settings each time a user logs in, I think. If this sounds fine to you, then I'll write a patch and attach it to this bug report. In the mean time, I'm going to attach a fix for 1) (as mentioned previously).
Created attachment 16847 [details] [review] Ensures that panel_profile_get_background_image() always returns a string.
instead could we check in panel_properties_dialog_setup_image_entry() if we got a NULL before doing a gtk_entry_set_text() ? Not really sure we should send an empty string. Anyways, I would leave that to Mark. Also panel_properties_dialog_update_name() seems a potential candidate for dump if "text" remains a NULL.
I think NULL is a perfectly valid return from panel_profile_get_background_image, but we should handle it safely in panel-properties-dialog.c with something like: --- panel-properties-dialog.c 9 Apr 2003 05:55:36 -0000 1.6 +++ panel-properties-dialog.c 26 May 2003 10:42:17 -0000 @@ -297,11 +297,12 @@ panel_properties_dialog_setup_image_entr dialog->image_entry = glade_xml_get_widget (gui, "image_entry"); image = panel_profile_get_background_image (dialog->toplevel); + if (image) { + entry = gnome_file_entry_gtk_entry (GNOME_FILE_ENTRY (dialog->image_entry)); + gtk_entry_set_text (GTK_ENTRY (entry), image); - entry = gnome_file_entry_gtk_entry (GNOME_FILE_ENTRY (dialog->image_entry)); - gtk_entry_set_text (GTK_ENTRY (entry), image); - - g_free (image); + g_free (image); + } g_signal_connect_swapped (dialog->image_entry, "changed", How does that sound ?
Heh :-) Mid-air collision. Yep, I image there are other places where we could get similar problems and need to patched in the same way. Thanks :-)
Created attachment 16849 [details] [review] Only calls gtk_entry_set_text() if text is non-NULL.
Mark, the above patch does it like you suggest. It also fixes Arvind's update_name() case and plugs a small memleak. Okay to commit? What's your opinion on adding the "reset_config" key?
Patch looks great. Please go ahead. About the reset_config key ... sorry I missed that the first time :/ Resetting like this should (and is) a part of GConf itself. If you do rm -rf ~/.gconf/apps/panel or recursively unset /apps/panel then it should be the same thing. I don't see what benefit it would be to make the panel do this itself ...
My theory has been that the crash was just a symptom of a bigger problem. Namely, that the panel can leave its gconf keys in an unexpected state if it terminates at the wrong time. The problem is that the panel copies some keys over from the default_setup key the first time gnome-panel is run (or if it is run after you do rm -rf ~/.gconf/apps/panel or recursively unset /apps/panel). If for some reason the panel is terminated while it is copying those keys over, then the next time the panel starts, gconf won't have all the keys that the panel expects (for instance in the case of this bug gconf didn't have the background key and its subkeys). The patch for 1) above isn't a complete fix to the problem, because some environments may set the schemas to have a default background image (or some other setting). In the event that a partial copy happens (from the panel terminating early for whatever reason) in such an environment, the above patch will stop the panel from crashing, but it won't retain the background image set in the schema (assuming the sysadmin setup the schema to have a preset panel background [or whatever]). My solution to this is to add a key to gconf that that the panel can set that says, "I am in the process of copying config data over", so if the panel terminates during this process, the next time it starts it will know it terminated and can continue to copy the config data over (e.g. add the background key or whatever). I purpose calling this key "reset_config". When the panel deems it necessary to copy the keys from default_setup, it will set "reset_config" to "yes". It will then copy the keys over, and when it's finished it will set "reset_config" to "no". If a gnome-panel starts and it finds that "reset_config" is "yes", then it will copy from default_setup (thus preventing the panel from having an incomplete set of gconf keys). Now it could be argued that "reset_config" isn't a good name and something like "is_copying_defaults" would be better. My opinion, though, is that having a key that has absolutely no use to users and is only useful for ensuring consistent program state seems to go against what gconf was designed for. So I figured extending the semantics of the key slightly to make it more externally useful was the way to go. By that I mean, if the key is called "reset_config", then a user can easily reset their configuration with gconf-editor or a sysadmin could just run gconftool --set at startup. Granted it doesn't bring a whole lot of advantage versus rm -Rf or gconftool --unset, but it seemed better than having a gconf key with no useful purpose to the user. Another option to protect against partial copies of keys would be to check if a default setup is being used, and if so, then check if each key (such as background/image) exists before trying to read from it. If a key doesn't exist then copy it from default_setup. Yet another option (and maybe a better option) is to extend GConf to have atomic transactions, where you could do something like: gconf_client_start_transaction (client); panel_gconf_copy_dir (client, src_dir, dest_dir); gconf_client_complete_transaction (client); I think if GConfChangeSet were atomic that would be even better. Of course, another option is to just say the odds of the panel terminating right when the defaults are being copied over is so slim that in the unlikely event it happens, the user can just delete ~/.gconf/apps/panel. So what do you think?
Ray, My panel loaded the first time without any problem. It is strange all that things you said aobout erasing the configuration, I erased ~/.gconf* and ~/.gnome* etc. to restart a whole fresh configuration and this bug is still present. Ray, Mark, should gtk_entry_set_text() validate NULL strings any way??
> I erased ~/.gconf* and ~/.gnome* etc. to restart a whole fresh > configuration and this bug is still present. Hmmm... Did you "killall gconfd-2" and then restart the panel (or reboot) after removing ~/.gconf*? What does: gconftool-2 -R /apps/panel/profiles/default/toplevels say? It could be there is another problem, yet.
> Ray, Mark, should gtk_entry_set_text() validate NULL strings any way?? It does unless G_DISABLE_CHECKS is defined when compiling glib. garnome defines it by default.
Ray: no, g_return_if_fail() is only meant to be used for debugging. It is similar to assert() as in you generally wouldn't ship an application with assert() turned on.
Mark: Yes, I know. I didn't mean to imply otherwise. To make it clear, gtk_entry_set_text() does not allow a NULL input, and any code that expects that it would is incorrect. Having said that, the result of giving a NULL input to gtk_entry_set_text() depends on whether glib was compiled with G_DISABLE_CHECKS defined or not. If G_DISABLE_CHECKS was not defined when glib was compiled, then gtk_entry_set_text() will validate the input by making sure it is non-NULL before using it. In the event that it is NULL and G_DISABLE_CHECKS is not defined then, it will post a warning to the console. If G_DISABLE_CHECKS is defined when glib is compiled then the input is not checked and a NULL value will cause the program to crash.
> What does: > > gconftool-2 -R /apps/panel/profiles/default/toplevels > > say? It could be there is another problem, yet. /apps/panel/profiles/default/toplevels/top_panel_screen0: y = 0 enable_animations = true enable_buttons = false hide_delay = 1500 animation_speed = fast unhide_delay = 0 size = 24 y_centered = false name = Top Panel expand = false x_centered = true auto_hide_size = 1 screen = 0 monitor = 0 x = 79 auto_hide = true enable_arrows = true orientation = top /apps/panel/profiles/default/toplevels/bottom_panel_screen0: y = 996 enable_animations = true enable_buttons = false hide_delay = 1500 animation_speed = fast unhide_delay = 0 size = 24 y_centered = false name = Bottom Panel expand = false x_centered = true auto_hide_size = 1 screen = 0 monitor = 0 x = 0 auto_hide = true enable_arrows = true orientation = bottom I did kill gconfd-2 before restarting but the bug keeps buging me :-) -William
Ray: Its not glib that is affected by the G_DISABLE_CHECKS define in this case. G_DISABLE_CHECKS is used in gmessages.h, so whether g_return_if_fail actually does anything or not in this case depends on whether *gtk+* is built with G_DISABLE_CHECKS ... Anyway, you've committed your patch, right ? So, I'll close the bugs. Much thanks to both of you (Ray and William) :-) 2003-05-26 Ray Strode <halfline@hawaii.rr.com> * panel-properties-dialog.c: (panel_properties_dialog_setup_image_entry): Don't send NULL to gtk_entry_set_text. (panel_properties_dialog_update_name): Don't send NULL to gtk_entry_set_text and plug a leak.
> Anyway, you've committed your patch, right ? Yes. > So, I'll close the bugs. Well, we've only fixed the crashing portion of the bug. I haven't found the broader issue of why /apps/panel/profiles/default/toplevels/* are missing background subdirs (iow, why the defaults aren't being copied over completely). I think maybe the bug should be reopened and the severity/priority should be dropped (because it's no longer a crasher), or maybe a new bug should be filed. As it turns out, my other ramblings about the panel being terminated prematurely wasn't the cause of the bug for William, so that issue can be shelved for now, I think. > Its not glib that is affected by the G_DISABLE_CHECKS define in > this case. G_DISABLE_CHECKS is used in gmessages.h, so whether > g_return_if_fail actually does anything or not in this case depends on > whether *gtk+* is built with G_DISABLE_CHECKS ... Yes, you're right. My bad.
Ray, I think it would be better to open a new bug with that problem explained more clearly. There's so much in this ubg already it would just confuse the issue. You've fixed the crash, which is actually a bug in itself.
Okay, I filed http://bugzilla.gnome.org/show_bug.cgi?id=113831