After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 307288 - Leak in nautilus-icon-factory
Leak in nautilus-icon-factory
Status: RESOLVED FIXED
Product: nautilus
Classification: Core
Component: general
2.11.x
Other All
: Immediate blocker
: 2.12.x
Assigned To: Nautilus Maintainers
Nautilus Maintainers
Depends on:
Blocks:
 
 
Reported: 2005-06-11 14:01 UTC by Kjartan Maraas
Modified: 2005-09-05 20:39 UTC
See Also:
GNOME target: 2.12.x
GNOME version: 2.11/2.12


Attachments
patch for the leak (4.53 KB, patch)
2005-08-09 19:16 UTC, Kjartan Maraas
needs-work Details | Review
updated patch after the other stuff was commited (726 bytes, patch)
2005-08-12 18:17 UTC, Kjartan Maraas
committed Details | Review

Description Kjartan Maraas 2005-06-11 14:01:56 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;
}
Comment 1 Christian Neumair 2005-07-12 12:50:39 UTC
Yeah, the additional factory->fallback_icon ref looks very odd. Do you feel good
enough to write a patch for fixing this?
Comment 2 Kjartan Maraas 2005-08-09 19:16:33 UTC
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...
Comment 3 Christian Neumair 2005-08-09 19:50:06 UTC
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.
Comment 4 Kjartan Maraas 2005-08-12 18:17:11 UTC
Created attachment 50625 [details] [review]
updated patch after the other stuff was commited

This should contain just this one change.
Comment 5 Martin Wehner 2005-08-22 22:35:50 UTC
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>
Comment 6 Alexander Larsson 2005-08-29 11:56:55 UTC
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.
Comment 7 Christian Neumair 2005-08-29 12:30:09 UTC
Why the additional ref? For the hash table?
Comment 8 Christian Neumair 2005-08-29 13:55:00 UTC
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.
Comment 9 Christian Neumair 2005-09-03 23:26:53 UTC
Please revert this patch before 2.12.0. Dubious memleaks are preferable over
heisenbug icon cache crashers.
Comment 10 Jürg Billeter 2005-09-05 19:01:51 UTC
Alex already has reverted the patch, as commented in bug 314718. Please close
the bug.