GNOME Bugzilla – Bug 764692
Minor memory leaks in champlain-map-source-factory and champlain-view
Last modified: 2016-04-10 10:37:59 UTC
Memory leaks and fixes: 1) g_object_get() creates copies which are never destroyed. https://developer.gnome.org/gobject/unstable/gobject-The-Base-Object-Type.html#g-object-get 2) g_hash_table_remove() destroys the key in the hash table(if found) but not the one sent as parameter for comparison https://developer.gnome.org/glib/2.34/glib-Hash-Tables.html#g-hash-table-remove
Created attachment 325495 [details] [review] champlain-map-source-factory: Fix memory leak in champlain_map_source_new_generic()
Created attachment 325496 [details] [review] champlain-view: Fix memory leak in tile_map_set
(In reply to Marius Stanciu from comment #1) > Created attachment 325495 [details] [review] [review] > champlain-map-source-factory: Fix memory leak in > champlain_map_source_new_generic() The variables that you freed are supplied to champlain_network_tile_source_new_full() which then creates a new source with g_object_new(). The g_object_new() uses this variables so freeing them might create problem.
(In reply to Nayan Deshmukh from comment #3) > (In reply to Marius Stanciu from comment #1) > > Created attachment 325495 [details] [review] [review] [review] > > champlain-map-source-factory: Fix memory leak in > > champlain_map_source_new_generic() > > The variables that you freed are supplied to > champlain_network_tile_source_new_full() which then creates a new source > with g_object_new(). The g_object_new() uses this variables so freeing them > might create problem. I'm not entirely sure about this, but from what I tested, g_object_new makes copies of const ghar* value parameters. That means the original gchar array remains unfreed.
Marius, good catch with the champlain_map_source_new_generic(), this really looks like a leak and your patch looks correct to me. I'm just wondering if it wouldn't be better to call the getters from ChamplainMapSourceDesc instead of g_object_get() - the getters don't do any reffing so the unrefs and frees wouldn't be necessary. Also something similar should be done for champlain_map_source_new_memphis(). By the way, it's not the g_object_get() which makes the reference - for instance with ChamplainMapSourceDesc it causes that the corresponding property gets retrieved by champlain_map_source_desc_get_property() and then it's the call of e.g. g_value_set_string() which duplicates the string. Regarding the second patch with the hash table, this doesn't look right (unless I miss something). The hash table is created using priv->tile_map = g_hash_table_new_full (g_int64_hash, g_int64_equal, slice_free_gint64, NULL); which means that when the key is removed it's always freed with slice_free_gint64() and no extra free is needed.
(In reply to Jiri Techet from comment #5) > Marius, good catch with the champlain_map_source_new_generic(), this really > looks like a leak and your patch looks correct to me. I'm just wondering if > it wouldn't be better to call the getters from ChamplainMapSourceDesc > instead of g_object_get() - the getters don't do any reffing so the unrefs > and frees wouldn't be necessary. Also something similar should be done for > champlain_map_source_new_memphis(). > > By the way, it's not the g_object_get() which makes the reference - for > instance with ChamplainMapSourceDesc it causes that the corresponding > property gets retrieved by champlain_map_source_desc_get_property() and then > it's the call of e.g. g_value_set_string() which duplicates the string. > The g_object_get() and all the memory management that comes after sure looks unnecessary in this case. I'll attach a patch using the class getters. > Regarding the second patch with the hash table, this doesn't look right > (unless I miss something). The hash table is created using > > priv->tile_map = g_hash_table_new_full (g_int64_hash, g_int64_equal, > slice_free_gint64, NULL); > > which means that when the key is removed it's always freed with > slice_free_gint64() and no extra free is needed. I've tested it both manually and with valgrind, and it definitely is a major leak(hundred of unfreed variables occur when panning the map) Here, I've cropped up a small code snippet to illustrate exactly what's going on as it is easy to miss. (see attached screenshot) The exact same thing is happening in tile_map_set()
Created attachment 325639 [details] Code snippet describing memory leak in tile_map_set
Review of attachment 325496 [details] [review]: Sorry, I misunderstood what you said - I didn't notice the remove case. Merged now, thanks.
Created attachment 325642 [details] [review] champlain-map-source-factory: Fix memory leak in map source constructors
Thanks, committed.