GNOME Bugzilla – Bug 307288
Leak in nautilus-icon-factory
Last modified: 2005-09-05 20:39:46 UTC
Valgrind reports this: ==8732== 5236 (52 direct, 5184 indirect) bytes in 1 blocks are definitely lost in loss record 17625 of 20645 ==8732== at 0x1B909B71: calloc (vg_replace_malloc.c:175) ==8732== by 0x1C4E8FB7: g_malloc0 (gmem.c:154) ==8732== by 0x1C4A40D0: g_type_create_instance (gtype.c:1576) ==8732== by 0x1C48A20F: g_object_constructor (gobject.c:1052) ==8732== by 0x1C48A861: g_object_newv (gobject.c:949) ==8732== by 0x1C48B3EA: g_object_new_valist (gobject.c:1033) ==8732== by 0x1C48B509: g_object_new (gobject.c:830) ==8732== by 0x1C01C2EB: gdk_pixbuf_new_from_data (gdk-pixbuf-data.c:65) ==8732== by 0x1C01A832: gdk_pixbuf_new (gdk-pixbuf.c:297) ==8732== by 0x1C01FFD7: gdk_pixbuf_scale_simple (gdk-pixbuf-scale.c:252) ==8732== by 0x1B96CA9F: create_normal_cache_icon (nautilus-icon-factory.c:1099) ==8732== by 0x1B96CCB0: nautilus_icon_factory_get_pixbuf_for_icon_internal (nautilus-icon-factory.c:1329) ==8732== by 0x1B96D022: nautilus_icon_factory_get_pixbuf_for_file_internal (nautilus-icon-factory.c:1545) ==8732== by 0x80A942A: fm_list_model_get_value (fm-list-model.c:231) ==8732== by 0x1BE7C310: gtk_tree_model_get_value (gtktreemodel.c:1077) ==8732== by 0x1BEA5C12: gtk_tree_view_column_cell_set_cell_data (gtktreeviewcolumn.c:2548) ==8732== by 0x1BE8FF88: validate_row (gtktreeview.c:4589) ==8732== by 0x1BE92EAD: validate_visible_area (gtktreeview.c:4866) ==8732== by 0x1BE9382E: do_presize_handler (gtktreeview.c:5203) ==8732== by 0x1BE93878: presize_handler_callback (gtktreeview.c:5214) ==8732== by 0x1C4E4FFF: g_idle_dispatch (gmain.c:3812) ==8732== by 0x1C4E2E5D: g_main_context_dispatch (gmain.c:1933) ==8732== by 0x1C4E5C2D: g_main_context_iterate (gmain.c:2564) ==8732== by 0x1C4E612E: g_main_loop_run (gmain.c:2768) ==8732== by 0x1BDD0464: gtk_main (gtkmain.c:974) ==8732== by 0x8070E89: main (nautilus-main.c:434) It looks like the code in get_icon_from_cache() takes two refs on the icon? if (cached_icon == NULL) { /* Not in the table, so load the image. */ /* g_print ("cache miss for %s:%s:%s:%d\n", icon, modifier?modifier:"", embedded_text?"<tl>":"", nominal_size); */ cached_icon = create_normal_cache_icon (icon, modifier, nominal_size, force_nominal); /* Try to fallback without modifier */ if (cached_icon == NULL && modifier != NULL) { cached_icon = create_normal_cache_icon (icon, NULL, nominal_size, force_nominal); } if (cached_icon == NULL) { cached_icon = factory->fallback_icon; cache_icon_ref (cached_icon); } /* Create the key and icon for the hash table. */ key = g_new (CacheKey, 1); key->name = g_strdup (icon); key->modifier = g_strdup (modifier); key->nominal_size = nominal_size; key->force_nominal = force_nominal; g_hash_table_insert (hash_table, key, cached_icon); } /* Hand back a ref to the caller. */ cache_icon_ref (cached_icon); /* Since this item was used, keep it in the cache longer. */ mark_recently_used (&cached_icon->recently_used_node); /* Come back later and sweep the cache. */ nautilus_icon_factory_schedule_sweep (factory); return cached_icon; }
Yeah, the additional factory->fallback_icon ref looks very odd. Do you feel good enough to write a patch for fixing this?
Created attachment 50483 [details] [review] patch for the leak I removed the one inside the if (cached_icon == NULL) clause since the other one looked very intentional with the comment and all...
The attachment 50483 [details] [review] also contains bits from bug 149714/attachment 48946 [details] [review]. The rest looks fine, though. The single added reference will be removed in nautilus_icon_factory_get_pixbuf_for_icon_internal again if wants_default is FALSE.
Created attachment 50625 [details] [review] updated patch after the other stuff was commited This should contain just this one change.
2005-08-23 Martin Wehner <martin.wehner@gmail.com> * libnautilus-private/nautilus-icon-factory.c: (get_icon_from_cache): Don't ref cached_icon twice. Fixes #307288. Patch from Kjartan Maraas <kmaraas@gnome.org>
This change was wrong. The icon should be ref:ed twice, just as the other icons in that part of the function is (create + ref). With this patch in nautilus reliably crashes for me in one directory. Also, bug 314718 is caused by this.
Why the additional ref? For the hash table?
Ouch. Please ignore comment 7. Of course it is right to ref it once more, because the caller will unref the icon again and it should stay in the hash table. I wonder why we leaked memory at all if the original behavior was correct, since nautilus_icon_factory_finalize correctly made the fallback icon refcount drop to zero.
Please revert this patch before 2.12.0. Dubious memleaks are preferable over heisenbug icon cache crashers.
Alex already has reverted the patch, as commented in bug 314718. Please close the bug.