GNOME Bugzilla – Bug 655366
missing GSettings schemas lead to obscure crashes
Last modified: 2011-10-20 09:39:10 UTC
this report has been filed here: https://bugs.launchpad.net/ubuntu/+source/gnome-terminal/+bug/816299 backtrace: ".
+ Trace 227907
Thread 1 (Thread 5134)
-> gsettings
The only way that this variable can be NULL is if GSettings has failed an assertion already due to an invalid key -- that would point to a bug in the user of GSettings. That said, we could deal with this error case more gracefully as well...
The key definitely exists in that schema (from gsettings-desktop-schemas) (otherwise there'd be tons of dups already). The .xsession-errors file available on lp that comment 0 failed to link to doesn't contain any gsettings criticals. You can probably just close this one as unreproducible and blame some freak ubuntu weirdness :)
*** Bug 655807 has been marked as a duplicate of this bug. ***
*** Bug 659832 has been marked as a duplicate of this bug. ***
*** Bug 659604 has been marked as a duplicate of this bug. ***
*** Bug 655427 has been marked as a duplicate of this bug. ***
Created attachment 198086 [details] [review] Avoid unreffing if variant is NULL Not sure why gsettings wasn't doing this already. This is a simple patch that checks for NULL before the g_variant_unref() in every g_settings_get_ call other than g_settings_get_value (which is where the variant value comes from in all the other calls, and can return NULL).
Created attachment 198087 [details] [review] Avoid unref if variant is NULL Here is a much simpler patch. Don't know why I didn't just do this in the first place. :)
Review of attachment 198087 [details] [review]: It's an error to call g_variant_unref() on a NULL GVariant. This is a very strong convention in GLib -- it applies also to GObject and pretty much everything else.
Review of attachment 198086 [details] [review]: This might be where the crash occurs, but the real bug is further up -- where the user created a GSettings object without the schema installed. This patch just treats the symptom.
commit 3106391694408877ebf6e8451146c5ac5d7bb017 Author: Ryan Lortie <desrt@desrt.ca> Date: Mon Oct 3 10:19:14 2011 -0400 Revert "GSettings: don't abort on missing schemas" This reverts commit c841c2ce3fda6f754c88ae2c9099f36dff2f0814. This approach has been an unmitigated disaster. We're getting all sorts of crashes due to functions that are returning NULL because they can't find the schema for the default value. The people who get these crashes are then confused about the root cause of the problem and waste a lot of time trying to figure it out. Until we find a better solution, we should go back to what we had before. https://bugzilla.gnome.org/show_bug.cgi?id=655366
Created attachment 198105 [details] [review] Update patch that uses g_return_if_fail instead Updated patch which only does g_return_if_fail inside g_variant_unref so we get criticals but avoid segfault in normal usage.
Review of attachment 198105 [details] [review]: commit to glib-2-30 please.
Should this not also go onto master?
I've heard a rumour that you're attempting to use the fact that your program no longer crashes completely (and merely g_critical()s all over the place) to argue that it is now correct. I disagree with that assertion. I consider this patch to be treatment for the bad symptoms caused by the fact that GSettings doesn't abort on missing schemas on glib-2-30. We no longer have that problem on master.
Rumors are just that. I'm suggesting that GValue is a public API and GSettings isn't the only thing to use it, and the correct behavior should be to critical rather than segfault if a NULL does get passed in. This patch fixes that to be the case.
*** Bug 661222 has been marked as a duplicate of this bug. ***
(In reply to comment #16) > I've heard a rumour that you're attempting to use the fact that your program no > longer crashes completely (and merely g_critical()s all over the place) to > argue that it is now correct. I disagree with that assertion. > > I consider this patch to be treatment for the bad symptoms caused by the fact > that GSettings doesn't abort on missing schemas on glib-2-30. We no longer > have that problem on master. I think Rodney is right. Now that we abort again on missing schemas, people won't be able to consider it correct and get hundreds of warnings: it will crash their programs. So you can safely fix g_variant_unref() not to crash when passed NULL. g_object_unref() already does that (actually, it does even more, since it calls G_IS_OBJECT(), which is probably a good idea).