GNOME Bugzilla – Bug 789955
Memory leak in st_label_init
Last modified: 2017-11-09 03:37:27 UTC
Reviewing Massif output from one Gnome Shell user [1] seems to show a repeating leak/bloat in st_label_init (st-label.c:268) - label->priv->label = g_object_new (CLUTTER_TYPE_TEXT, "ellipsize", PANGO_ELLIPSIZE_END, NULL); And the source code appears to agree that st_label_dispose forgets to free it. Just strange I can't seem reproduce the same leak with Valgrind myself... [1] https://launchpadlibrarian.net/344249116/massif.out.1764.msprint
The ClutterText is a child of StLabel, so it should be destroyed/freed simply by chaining up. See bug 783483.
Yes, I suspect commit ad2cb22 might be wrong... label->priv->label = g_object_new (CLUTTER_TYPE_TEXT, "ellipsize", PANGO_ELLIPSIZE_END, NULL); refcount == 1 because "The reference count is initialized to one by g_object_new" [https://developer.gnome.org/gobject/stable/gobject-memory.html] clutter_actor_add_child (CLUTTER_ACTOR (label), priv->label); refcount == 2 because "This function will acquire a reference on child" [https://developer.gnome.org/clutter/stable/ClutterActor.html#clutter-actor-add-child] So the parent removing its reference later won't be enough. It sounds like commit ad2cb22 might need reverting.
... and whatever the original problem was that Cosimo was trying to solve might need solving differently.
Perhaps both are true: * commit ad2cb22 fixed a problem by avoiding premature clutter_actor_destroy; and * commit ad2cb22 created a leak. Surely g_object_unref or g_clear_object in st_label_dispose would be safer, and solve both problems...?
(In reply to Daniel van Vugt from comment #2) > Yes, I suspect commit ad2cb22 might be wrong... > > label->priv->label = g_object_new (CLUTTER_TYPE_TEXT, > "ellipsize", PANGO_ELLIPSIZE_END, > NULL); > > refcount == 1 because "The reference count is initialized to one by > g_object_new" > [https://developer.gnome.org/gobject/stable/gobject-memory.html] Except that ClutterActor inherits from GInitiallyUnowned, so the first ref is floating. > clutter_actor_add_child (CLUTTER_ACTOR (label), priv->label); > > refcount == 2 because "This function will acquire a reference on child" > [https://developer.gnome.org/clutter/stable/ClutterActor.html#clutter-actor- > add-child] No, refcount should still be 1, because clutter_actor_add_child() calls g_object_ref_sink() on a floating reference: https://git.gnome.org//browse/mutter/tree/clutter/clutter/clutter-actor.c#n12891 (In reply to Daniel van Vugt from comment #4) > Surely g_object_unref or g_clear_object in st_label_dispose would be safer, > and solve both problems...? I still don't see the problem - there should be a single reference to the ClutterText, and dispose() emits the ::destroy signal whose class handler will recursively destroy all children ... not to mention that clutter_actor_destroy() calls g_object_run_dispose(), which bypasses the refcount altogether.
Oh, good point. I did not notice that ClutterActor is GInitiallyUnowned. This is somewhat confusing given the documentation that simply says "The reference count is initialized to one by g_object_new". Because it's just not true in all cases, and yet is stated as fact. Thanks for the clarification.
(In reply to Daniel van Vugt from comment #6) > This is somewhat confusing given the documentation that simply says "The > reference count is initialized to one by g_object_new". Because it's just > not true in all cases, and yet is stated as fact. It is true - the initial reference of a GInitiallyUnowned is marked as floating, but its refcount is still 1. The bit that's a bit vague is the documentation of clutter_actor_add_child - "will acquire a reference" is easily misread as "increase the reference count".
Yeah, fair enough. I realized that soon after typing the comment. Still, from the perspective of someone reading the code they should be able to see g_object_new and expect an equal number of unrefs somewhere. Having that conditionally not applicable, while clever, is asking for user errors. The rules should be more clear-cut and simple to minimize programming errors. But I understand it's probably not something you designed, and not something that can be improved in retrospect.
(In reply to Daniel van Vugt from comment #8) > But I understand it's probably not something you designed Right, it dates back a long time. I can understand how floating references can be confusing at first (I know it took me a bit), but then I'm not convinced that GtkWidget *box, *w; box = gtk_box_new (GTK_ORIENTATION_HORIZONTAL, 0); w = gtk_label_new ("Foo"); gtk_container_add (GTK_CONTAINER (box), w); g_object_unref (w); would be less error-prone than GtkWidget *box = gtk_box_new (GTK_ORIENTATION_HORIZONTAL, 0); gtk_container_add (GTK_CONTAINER (box), gtk_label_new ("Foo"));