GNOME Bugzilla – Bug 674050
Free image->priv->icon_helper in gtk_image_finalize instead of gtk_image_destroy
Last modified: 2012-04-20 13:01:43 UTC
Created attachment 211991 [details] Test program by Martin Pitt that demos the crash Hi, we had some crashes in pygi application like apport-gtk and software-center that appear to be caused by gtk freeing image->priv->icon_helper earlier than pygi is expecting this. Attached is a demo program that triggers the crash and a patch that moves the freeing from gtk_image_destroy() to gtk_image_finalize(). Feedback very welcome as I'm not a expert on GObject lifecycles :) But it does fix the crash in software-center and in the demo app.
Created attachment 211992 [details] [review] move g_clear_object (&image->priv->icon_helper); to gtk_image_finalize This moves the freeing of the icon_helper from the destory to the finalize function to avoid the crash with pygi.
Note that the test case is calling destroy on this to simulate e.g. a top level window destroy by closing the window and a gtk event being delivered at roughtly the same time.
launchpad bug: https://bugs.launchpad.net/ubuntu/+source/apport/+bug/938090 stracktrace example: https://launchpadlibrarian.net/93653357/Stacktrace.txt "#0 _gtk_icon_helper_get_storage_type (self=0x0) at /build/buildd/gtk+3.0-3.3.14/./gtk/gtkiconhelper.c:482 No locals.
+ Trace 230060
..."
Review of attachment 211992 [details] [review]: Looks good to commit, with just one little comment below: ::: gtk/gtkimage.c @@ +1482,3 @@ gtk_image_clear (GtkImage *image) { + g_return_if_fail (GTK_IS_IMAGE (image)); Please leave this out of the patch, I don't think it's needed.
I committed Michael's patch with Cosimos' suggestion and added an appropriate changelog: http://git.gnome.org/browse/gtk+/commit/?id=19e55d620fc0793ec688377bb11b77bdd2c9e9cb
This looks wrong to me: internal objects should be unreffed in destroy/dispose not in finalize, to allow breaking cyclic references. If we have crashes because pygi or applications try to access destroyed objects, then the bug is in pygi or the application. For instance in Martin's test case it is the application that should take care of cancelling the idles before destroying the objects. Incidentally I also partially disagree with Cosimo's comment: while it is true that the g_return_if_fail should not be part of this patch, I still think it should be added since all gtk entry points should have the sanity checks on their params
(In reply to comment #6) > This looks wrong to me: internal objects should be unreffed in destroy/dispose > not in finalize, to allow breaking cyclic references. AFAICS there's no risk of cyclic references here, since GtkIconHelper doesn't take any reference to widgets. Since it's still possible to call e.g. gtk_image_set_icon_name() or similar methods after gtk_widget_destroy() is called, even if it's "wrong", I still think it's better to avoid crash if we can like in this case. > Incidentally I also partially disagree with Cosimo's comment: while it is true > that the g_return_if_fail should not be part of this patch, I still think it > should be added since all gtk entry points should have the sanity checks on > their params Yeah, my point was more about the fact that if anything it should be a separate patch; it's also not a regression caused by my code, since gtk_image_clear() wasn't checking input parameters before the IconHelper migration.
*** Bug 668563 has been marked as a duplicate of this bug. ***