Created attachment 157752 [details] [review]
[StThemeNode] Squash epic memory leak

StThemeNode holds a reference to its parent, but we never released
that reference.  This could cause us to hold onto whole chains
of theme nodes with rather dire memory usage implications.

Also move the other g_object_unref into _dispose.

Review of attachment 157752 [details] [review]:

The finalize/dispose split here doesn't seem necessary to me - ThemeNodes are simple data structures, and shouldn't have signals etc and shouldn't participate in refcount cycles. And we don't g_object_run_dispose() dispose the theme node in st_widget_dispose(), before unreffing it. But it is also harmless.

You missed one of the three things ref'ed in the constructor - node->theme. (Which has an inconsistency in two code paths as to whether it is ref'ed or not, so that needs fixing too)

(In reply to comment #2)
> Review of attachment 157752 [details] [review]:
> 
> The finalize/dispose split here doesn't seem necessary to me - ThemeNodes are
> simple data structures, and shouldn't have signals etc and shouldn't
> participate in refcount cycles. And we don't g_object_run_dispose() dispose the
> theme node in st_widget_dispose(), before unreffing it. But it is also
> harmless.

I'd prefer to keep the dispose; helps me be consistent for when it is needed.

> You missed one of the three things ref'ed in the constructor - node->theme.
> (Which has an inconsistency in two code paths as to whether it is ref'ed or
> not, so that needs fixing too)

It looks to me like it's always referenced.

Created attachment 157827 [details] [review]
[StThemeNode] Squash epic memory leak

StThemeNode holds a reference to its parent, but we never released
that reference.  This could cause us to hold onto whole chains
of theme nodes with rather dire memory usage implications.

Also move the other g_object_unref into _dispose.

Review of attachment 157827 [details] [review]:

Looks fine

(In reply to comment #5)
> Review of attachment 157827 [details] [review]:
> 
> Looks fine

No, this seems to cause lots of 
g_object_ref: assertion `G_IS_OBJECT (object)' failed
and
g_object_unref: assertion `G_IS_OBJECT (object)' failed

random elements to have no style applied and even a crash.

The previous version was fine.

Ok, i see the probable bug here:


diff --git a/src/st/st-theme-node.c b/src/st/st-theme-node.c
index 6d37d60..d742c6b 100644
--- a/src/st/st-theme-node.c
+++ b/src/st/st-theme-node.c
@@ -47,7 +47,7 @@ st_theme_node_dispose (GObject *gobject)
   if (node->theme)
     {
       g_object_unref (node->theme);
-      node->context = NULL;
+      node->theme = NULL;
     }
 
   if (node->parent_node)