GNOME Bugzilla – Bug 709453
background: More memory management fixes
Last modified: 2013-10-07 10:56:48 UTC
Quickly clicking on 'Cancel' after opening the wallpaper chooser results in a bunch of criticals and is likely a regression from commit b9e3603ba4d6b494c0659cbc0196ef9f81fa6a60. GLib-GObject-CRITICAL **: g_object_unref: assertion 'G_IS_OBJECT (object)' failed GLib-GObject-CRITICAL **: g_object_unref: assertion 'G_IS_OBJECT (object)' failed GLib-GObject-CRITICAL **: g_object_unref: assertion 'G_IS_OBJECT (object)' failed GLib-GObject-CRITICAL **: g_object_unref: assertion 'G_IS_OBJECT (object)' failed
Created attachment 256508 [details] [review] background: Fix reference counting with item loading Commit b9e3603ba4d6b494c0659cbc0196ef9f81fa6a60 added an unref to fix a memory leak, but it went to a wrong place -- in XML loader, all items are stored in a hash table and unreffed when it's destroyed. This commit reverts b9e3603 and adds a source code comment explaining memory management there. The following commit fixes the real leak.
Created attachment 256509 [details] [review] background: Fix a memory leak with background items When adding items to GtkListStore there was an extra g_object_ref: the liststore references values itself and there's no need to reference it by the caller.
Review of attachment 256508 [details] [review]: ::: panels/background/cc-background-xml.c @@ +313,3 @@ g_object_set (G_OBJECT (item), "flags", flags, NULL); g_hash_table_insert (xml->priv->wp_hash, id, item); + /* Don't free ID or item, we've added them to the hash table */ I would rather add the g_object_ref while inserting into the hash table. This function is already a bit convulated. I find it much safer to give each data structure its own reference than trying to optimize away one call to g_object_unref. FWIW, there are similar uses of g_object_set_data instead of g_object_set_data_full in BgPicturesSource which make me uneasy. Too easy to get these "optimizations" wrong.
Review of attachment 256509 [details] [review]: Sure.
Please apply to both gnome-3-8 and master.
(In reply to comment #3) > Review of attachment 256508 [details] [review]: > > ::: panels/background/cc-background-xml.c > @@ +313,3 @@ > g_object_set (G_OBJECT (item), "flags", flags, NULL); > g_hash_table_insert (xml->priv->wp_hash, id, item); > + /* Don't free ID or item, we've added them to the hash table */ > > I would rather add the g_object_ref while inserting into the hash table. I think the person who wrote the original code wasn't trying to avoid a g_object_ref, but making a copy of 'id'. It probably makes sense to handle 'id' and 'item' in the same way, reffing one and not duping the other makes it even more confusing. I guess an option could be: - g_hash_table_insert (xml->priv->wp_hash, id, item); - /* Don't free ID or item, we've added them to the hash table */ + g_hash_table_insert (xml->priv->wp_hash, + g_strdup (id), + g_object_ref (item)); And then unref / free both at the end.
Created attachment 256536 [details] [review] background: Fix reference counting in background XML loader Commit b9e3603ba4d6b494c0659cbc0196ef9f81fa6a60 added an unref to fix a memory leak, but it went to a wrong place -- in XML loader, all items are stored in a hash table that takes ownership of them, and destroyed when the hash table goes away. This commit reverts b9e3603 and rearranges code a bit to make the memory management strategy clearer. The following commit fixes the real leak.
(In reply to comment #6) > (In reply to comment #3) > > Review of attachment 256508 [details] [review] [details]: > > > > ::: panels/background/cc-background-xml.c > > @@ +313,3 @@ > > g_object_set (G_OBJECT (item), "flags", flags, NULL); > > g_hash_table_insert (xml->priv->wp_hash, id, item); > > + /* Don't free ID or item, we've added them to the hash table */ > > > > I would rather add the g_object_ref while inserting into the hash table. > > I think the person who wrote the original code wasn't trying to avoid a > g_object_ref, but making a copy of 'id'. It probably makes sense to handle 'id' > and 'item' in the same way, reffing one and not duping the other makes it even > more confusing. > > I guess an option could be: > > - g_hash_table_insert (xml->priv->wp_hash, id, item); > - /* Don't free ID or item, we've added them to the hash table */ > + g_hash_table_insert (xml->priv->wp_hash, > + g_strdup (id), > + g_object_ref (item)); > > > And then unref / free both at the end. Yes, I like this. The benefits of making the code safer against memory errors over avoiding a g_strdup / g_free is much more. Unless the extra g_strdup / g_free is a performance bottleneck, which I don't think is the case here.
Created attachment 256539 [details] [review] background: Fix reference counting in background XML loader Commit b9e3603ba4d6b494c0659cbc0196ef9f81fa6a60 added an unref to fix a memory leak, but it went to a wrong place -- in XML loader, all items are stored in a hash table that takes ownership of them, and destroyed when the hash table goes away. This commit fixes the reference counting in the XML loader and adds explicit g_object_ref / g_strdup when inserting values to the hash table to make the memory manamgement more obvious. The following commit fixes the real leak.
Review of attachment 256539 [details] [review]: Looks good.
Thanks rishi! Attachment 256539 [details] pushed as 3491c66 - background: Fix reference counting in background XML loader Attachment 256509 [details] pushed as 4ecb35c - background: Fix a memory leak with wallpaper loading