Memory leak with widgets that have event handlers

[gnunn1]

I had a Tilix user report a pretty serious memory leak with the Tilix sidebar here: gnunn1/tilix#1558

After debugging it I've determined that the issue happens when a widget has an event handler associated with it. This seems to prevent the D GC from reclaiming the object and freeing it. If there is no event handlers in the widget everything works fine.

I've created a simple reproducer for you so you don't have to deal with the large Tilix code base to track this down. The reproducer is located here:

https://github.com/gnunn1/gctest

To reproduce the issue, compile the application using the following command:

dub build --config=trace

Notice in the destructor of the ListBoxRow I have added a line to log when the D destructor is called here:

https://github.com/gnunn1/gctest/blob/master/source/gctest.d#L129

After compiling the program run it in a terminal so you can see the output. The application shows a window with a headerbar and a listbox with some rows in it. In the headerbar are two buttons: "Recreate Rows" and "GC". If you click the "Recreate Rows" button it will remove the existing rows and replace them with a new set. Clicking the "GC" button invokes D's GC.

Press the "Recreate Rows" once and then press the "GC" button a couple of times, notice there is no output from the destructor.

Next, comment out the event handler for the button in the GCListBoxRow at line 129 (https://github.com/gnunn1/gctest/blob/master/source/gctest.d#L129) and re-compile it and then run it again. Repeat the same exercise as above and you will see that the destructor is now being invoked.

If you need any additional information just let mw know.

[MikeWey]

It looks like the GC.addRoot in the DClosure that is keeping the delegate alive for as long as the CLosure is alive, is causing the issue.

[gnunn1]

Thanks, I can see that now. As a workaround if I explicitly call the destroy method that will cause it to be GC'ed.

One thing I was wondering about, rather then doing GC.addRoot, would it be possible for ObjectG to manage references to the DClosure itself and thus tie it's lifecycle to the object? For example, I was thinking something along these lines pseudo code wise:

class ObjectG {
  private DClosure closures[];

  ...
}

And then in Signals.connect:

public static gulong connect(T)(ObjectG instance, string detailedSignal, T callback, ConnectFlags connectFlags = cast(ConnectFlags)0)
	if ( isCallable!T && !is(T == GCallback) )
	{
		bool after = (connectFlags & ConnectFlags.AFTER) != false;
		bool swap = (connectFlags & ConnectFlags.SWAPPED) != false;
                DClosure closure = new DClosure(callback, swap);
                instance.closures ~=closure;

		return Signals.connectClosure(instance, detailedSignal, closure, after);
	}

Obviously signals disconnect would need to be updated to remove the closure from the object instance as well.

In this way the lifecycle of the closures are tied to the object instance instead of separate instance entities. I'm not knowledgeable about this as you but does that make any kind of sense?

[MikeWey]

Commit c1ed569 does exactly that, it fixes the memory issue for the small test case.
Does it work for Tilix?

[gnunn1]

I've tried the change, while tilix initially runs unfortunately I am getting segfaults in other event handlers. Specifically if I run Tilix and then show the Preferences dialog it triggers a FocusOut event which is handled by the following delegate:

https://github.com/gnunn1/tilix/blob/master/source/gx/tilix/appwindow.d#L1732

The backtracebelow from gdb shows it failing on the check for gsSettings. If I insert some debug code into the delgate I can see the delegate itself seems to be there but the reference to gsSettings is bad. A check for null on it passes but then then fails at line 1733 where it tries to load the boolean value.

gsSettings is a member of the AppWindow class and it shouldn't be getting GC'ed since the AppWindow instance is holding a reference to it.

Thread 1 "tilix" received signal SIGSEGV, Segmentation fault.
0x0000000000000000 in ?? ()
(gdb) bt
#0  0x0000000000000000 in  ()
#1  0x000055555592aff4 in _D2gx5tilix9appwindow9AppWindow6__ctorMFC3gtk11ApplicationQnbZ12__dgliteral7MFC3gdk5EventQgCQBz6WidgetQhZb
    (e=0x7ffff76243a0, widget=0x7ffff7584400) at /home/gnunn/Development/dlang/tilix/source/gx/tilix/appwindow.d:1733
#2  0x0000555555b6a41a in _D7gobject8DClosureQj__T17d_closure_marshalTDFC3gdk5EventQgC3gtk6WidgetQhZbZQCaUPSQDc1c5types8GClosurePSQDyQwQw6GValuekQrPvQcZv
    (closure=0x555556739210, return_value=0x7fffffffda10, n_param_values=2, param_values=0x7ffff75dca40, invocation_hint=0x7fffffffd9f0 "J", marshal_data=0x0) at DClosure.d:114
#3  0x00007ffff6c263d5 in g_closure_invoke () at /usr/lib/libgobject-2.0.so.0
#4  0x00007ffff6c12c78 in  () at /usr/lib/libgobject-2.0.so.0
#5  0x00007ffff6c165ed in g_signal_emit_valist () at /usr/lib/libgobject-2.0.so.0
#6  0x00007ffff6c17a80 in g_signal_emit () at /usr/lib/libgobject-2.0.so.0
#7  0x00007ffff59431f5 in  () at /usr/lib/libgtk-3.so.0
#8  0x00007ffff57fd0d8 in gtk_main_do_event () at /usr/lib/libgtk-3.so.0
#9  0x00007ffff54faa06 in  () at /usr/lib/libgdk-3.so.0
#10 0x00007ffff552c755 in  () at /usr/lib/libgdk-3.so.0
#11 0x00007ffff6b3f3cf in g_main_context_dispatch () at /usr/lib/libglib-2.0.so.0
#12 0x00007ffff6b40f89 in  () at /usr/lib/libglib-2.0.so.0
#13 0x00007ffff6b40fce in g_main_context_iteration () at /usr/lib/libglib-2.0.so.0
#14 0x00007ffff6ce57ee in g_application_run () at /usr/lib/libgio-2.0.so.0
#15 0x0000555555a92f96 in _D3gio11ApplicationQn3runMFAAyaZi (this=0x7ffff7583200, argv=...) at Application.d:931
#16 0x000055555593f6b5 in D main (args=...) at /home/gnunn/Development/dlang/tilix/source/app.d:152

Also, when I compile Tilix as a debug build, I insert a menu entry that calls the D garbage collector:

core.memory.GC.collect();

This generates the following backtrace in gdb:

Thread 1 "tilix" received signal SIGSEGV, Segmentation fault.
0x000055555592af48 in _D2gx5tilix9appwindow9AppWindow6__ctorMFC3gtk11ApplicationQnbZ__T12__dgliteral6TCQBo6WidgetQhZQBdMFPS3gdk1c5types12GdkRectangleQBvZv (rect=0x7fffffffd510, Widget=0x7ffff7584400) at /home/gnunn/Development/dlang/tilix/source/gx/tilix/appwindow.d:1724
1724	                    isBGImage.destroy();
(gdb) bt
#0  0x000055555592af48 in _D2gx5tilix9appwindow9AppWindow6__ctorMFC3gtk11ApplicationQnbZ__T12__dgliteral6TCQBo6WidgetQhZQBdMFPS3gdk1c5types12GdkRectangleQBvZv (rect=0x7fffffffd510, Widget=0x7ffff7584400) at /home/gnunn/Development/dlang/tilix/source/gx/tilix/appwindow.d:1724
#1  0x0000555555b79e3d in _D7gobject8DClosureQj__T17d_closure_marshalTDFPS3gdk1c5types12GdkRectangleC3gtk6WidgetQhZvZQCpUPSQDrQBwQBx8GClosurePSQElQCqQCr6GValuekQtPvQcZv
    (closure=0x5555567389e0, return_value=0x0, n_param_values=2, param_values=0x7ffff757d0c0, invocation_hint=0x7fffffffd1a0 "*", marshal_data=0x0)
    at DClosure.d:114
#2  0x00007ffff6c263d5 in g_closure_invoke () at /usr/lib/libgobject-2.0.so.0
#3  0x00007ffff6c12c78 in  () at /usr/lib/libgobject-2.0.so.0
#4  0x00007ffff6c1701e in g_signal_emit_valist () at /usr/lib/libgobject-2.0.so.0
#5  0x00007ffff6c17a80 in g_signal_emit () at /usr/lib/libgobject-2.0.so.0
#6  0x00007ffff594d740 in gtk_widget_size_allocate_with_baseline () at /usr/lib/libgtk-3.so.0
#7  0x00007ffff595e6a7 in  () at /usr/lib/libgtk-3.so.0
#8  0x00007ffff6c1716e in g_signal_emit_valist () at /usr/lib/libgobject-2.0.so.0
#9  0x00007ffff6c17a80 in g_signal_emit () at /usr/lib/libgobject-2.0.so.0
#10 0x00007ffff57289d1 in  () at /usr/lib/libgtk-3.so.0
#11 0x00007ffff6c263d5 in g_closure_invoke () at /usr/lib/libgobject-2.0.so.0
#12 0x00007ffff6c13195 in  () at /usr/lib/libgobject-2.0.so.0
#13 0x00007ffff6c1701e in g_signal_emit_valist () at /usr/lib/libgobject-2.0.so.0
#14 0x00007ffff6c17a80 in g_signal_emit () at /usr/lib/libgobject-2.0.so.0
#15 0x00007ffff550431b in  () at /usr/lib/libgdk-3.so.0
#16 0x00007ffff54eeb2b in  () at /usr/lib/libgdk-3.so.0
#17 0x00007ffff6b3eb63 in  () at /usr/lib/libglib-2.0.so.0
#18 0x00007ffff6b3f271 in g_main_context_dispatch () at /usr/lib/libglib-2.0.so.0
#19 0x00007ffff6b40f89 in  () at /usr/lib/libglib-2.0.so.0
#20 0x00007ffff6b40fce in g_main_context_iteration () at /usr/lib/libglib-2.0.so.0
#21 0x00007ffff6ce57ee in g_application_run () at /usr/lib/libgio-2.0.so.0
#22 0x0000555555a92f96 in _D3gio11ApplicationQn3runMFAAyaZi (this=0x7ffff7583200, argv=...) at Application.d:931
#23 0x000055555593f6b5 in D main (args=...) at /home/gnunn/Development/dlang/tilix/source/app.d:152

Similar to the onFocusOut handler, it looks like it is segfaulting a referencing a variable that is scoped outside the delegate.

[gnunn1]

I've updated the gctest repo to reproduce the problem, simply trying to reference lblText in the button click handler causes the same issue. If you press the GC button a couple of times and then click the button in one of the rows you will see it segfault.

https://github.com/gnunn1/gctest/blob/master/source/gctest.d#L121

[MikeWey]

I've added addRange and removeRange to the DClosure so the D GC will scan the struct that was allocated by GLib, this fixes the segfaults but the original issue returns.

addRange is a slight improvement over the previous way that addRoot was used since we now the GC will only scan the DClosure when it's referenced from D.

For the original issue:
GtkD hold a C reference on the object and adds it as a GC root when the refcount is grater or equal to 2.
When the test adds the new rows the refcount of the old rows go's down to 1, removing it as a GC root and the GC should than be free to collect it.
Only the ListBoxRow still holds a reference to the added Button, the refCount for the button is now 2 and it's still a GC root.
The Button also holds the DClosure that is used for the onClicked signal, the DClosure holds the delegate used, and the delegate references the ListBoxRow keeping it alive.

[gnunn1]

I'm not sure I'm quite following, why is the button ref count 2?

Is it because the ListBoxRow holds a reference to it as it's parent or because the closure does? If it's a case of the ListBoxRow holding a reference to the button, why isn't there the same issue when no signals are involved? If it's a case of the closure holding a reference to it, why is the closure incrementing the reference of the button, does it need to or should it be using something like a weak reference?

Also, looking at the source code at the line below, why do we need to remove root from the GClosure, I'm not seeing where it gets added to root? It doesn't look like there is a corresponding ObjectG for it where the root would be added and doing a search of the code the only addRoot I can see is in ObjectG though I may be missing something.

https://github.com/gtkd-developers/GtkD/blob/master/generated/gtkd/gobject/DClosure.d#L101

DClosure does wrap GClosure but it doesn't descend from ObjectG as far as I can tell and thus I'm not seeing how it gets added to root.

[MikeWey]

For the button, both the listboxrow and gtkd are holding a reference. The issue only heapens with signals because the bottom has a DClosure which hold a delegate which references the listbox row. And since the button is a GC root when its reference count is grater then 1, we have a cycle with one GC root in it.

Without the signal the gc would collect the listboxrow, bringing the ref count for the button down to 1, removing it as a root and then it can be collected.

We need to either store a reference to a child widget in its parent and not add it as a GC root. Or we need some way to break the cycle when the ref count of the container go's down to 1.

The remove root in the destructor should have been removeRange.

[gnunn1]

I'm still a little confused (not unusual!), I'm not seeing why adding the event (signal) makes a difference to it being GC'ed. I may not be following what you are saying above, but I'm not seeing how the reference count on the button being held by the listboxrow is a problem since that should be the same regardless of whether we have the event or not. I'm also not seeing the issue as being the delegate holding a reference to the lisboxrow, the GC should be able to collect a graph of objects so once the listboxrow goes out of scope everything associated with it should be collected.

Instead I think the issue is the addRange in DClosure preventing the GC and I'm not sure why this is needed. It looks like you are preventing the memory associated with gGlosure from being GC'ed by using addRange however the base Closure class already has a reference to the pointer. I guess that's not enough to prevent the memory from being GC'ed and if that's the case is there a way to simply maintain a reference to the block of memory as an instance variable instead of using addRange?

The whole idea I was trying to get at in my original comment is to not use addRoot/addRange for DClosure but rather tie it to the ObjectG lifecycle via instance variables. I was thinking this would allow the GC to naturally collect the graph of objects once the ObjectG expires via it's own ref counting and the removeRoot that your are managing.

[gnunn1]

And looking at the code some more for DClosure, at this line here a GClosure is allocated:

https://github.com/gtkd-developers/GtkD/blob/master/generated/gtkd/gobject/DClosure.d#L85

Is the variable here shadowing the gClosure field in the base Closure class?

https://github.com/gtkd-developers/GtkD/blob/master/generated/gtkd/gobject/Closure.d#L84