Memory leak when using events from subclass

[gnunn1]

Below is a simple reproducer for a problem I'm having in Terminix (gnunn1/tilix#589) where objects don't seem to be released properly when using eventHandlers. I'm not sure if this is a GtkD problem, an issue with D's GC not handling chained dependencies or a coding practice I shouldn't be doing.

When you run the reproducer, it has an Add and Delete button. Clicking the Add Button creates a new Box with an Entry, clicking the Delete button removes and calls the D GC to collect (just to force things for testing purposes). The CustomVox and CustomEntry which are created have destructors that simply write a line out to stdout.

If you run the code and press Add then Delete and repeat several times you will see that the destructors for CustomBox and CustomEntry are not being executed. However, if you comment out the line:

entry.addOnChanged(&onChanged);

and repeat the test you will see that the destructors are being called properly.

My expectation is that because the CustomBox is not being referenced in D anywhere that it should be GC'ed, however something with the event handler between the CustomBox and CustomEvent is stopping it from working. Is this incrementing the reference count and preventing the GC from removing the struct from root maybe?

import std.stdio;

import gio.Application : GioApplication = Application;
import gtk.Application;
import gtk.ApplicationWindow;
import gtk.Box;
import gtk.Button;
import gtk.EditableIF;
import gtk.Entry;
import gtk.Label;

class HelloWorld : ApplicationWindow
{

	Box bContent;
	CustomBox bEntry;

	this(Application application)import std.stdio;

import gio.Application : GioApplication = Application;
import gtk.Application;
import gtk.ApplicationWindow;
import gtk.Box;
import gtk.Button;
import gtk.EditableIF;
import gtk.Entry;
import gtk.Label;

class HelloWorld : ApplicationWindow
{

	Box bContent;
	CustomBox bEntry;

	this(Application application)
	{
		super(application);
		setTitle("GtkD");
		setBorderWidth(10);

		Box bButtons = new Box(Orientation.HORIZONTAL,6);
		Button btnAdd = new Button("Add");
		btnAdd.addOnClicked(delegate(Button b) {
			if (bEntry is null) {
				bEntry = new CustomBox();
				bEntry.showAll();
				bContent.add(bEntry);
			}
		});
		bButtons.add(btnAdd);
		
		Button btnDelete = new Button("Delete");
		btnDelete.addOnClicked(delegate(Button b) {
			if (bEntry !is null) {
				bContent.remove(bEntry);
				bEntry = null;

				import core.memory;
				GC.collect();
			}
		});
		bButtons.add(btnDelete);

		bContent = new Box(Orientation.VERTICAL, 6);
		bContent.add(bButtons);

		add(bContent);

		showAll();
	}
}

class CustomBox: Box {
private:
	CustomEntry entry;

	void onChanged(EditableIF editable) {
		writeln("Entry changed");
	}

public:
	this() {
		super(Orientation.HORIZONTAL,0);
		entry = new CustomEntry();
		// Comment out the addOnChanged and destructors start triggering properly
		entry.addOnChanged(&onChanged);
		add(entry);
	}

	~this() {
		writeln("Box destructor destroyed");
	}
}

class CustomEntry: Entry {

	~this() {
		writeln("Entry destructor called");
	}
}

int main(string[] args)
{
    auto application = new Application("org.gtkd.demo.helloworld", GApplicationFlags.FLAGS_NONE);
    application.addOnActivate(delegate void(GioApplication app) { new HelloWorld(application); });
    return application.run(args);
}

[gnunn1]

One other thing, if you destroy the bEntry box in the delete button delegate after removing it things work as expected. I am doing this in terminix as well but forgot to include it in this reproducer.

Having said that, this reproducer still shows how something can prevent D's GC from cleaning up objects. I'm hoping that maybe it can shed some light on my issue.

[MikeWey]

I know whats going on.

On the GTK+ side the objects are reference counted an on the GtkD side we use the GC. And there is some code in GtkD that makes sure the D object isn't collected if the reference GtkD has isn't the only one.

When you create an object only GtkD has a reference to it, and the GC is free to collect it. But once you set an event handler, or add the widget to a container GTK+ also holds one or more references. And in that case the objects aren't collected so the handlers will keep working if the D code doesn't keep an reference to the object.

For now you can call the destroy function to manually destroy the object. And maybe at some point we need to provide an function to remove the event handlers.

[gnunn1]

Thanks @MikeWey, one follow-up question. In GTK the destroy is automatically propagated to the children however GTKD does not do this at it's level. I can see in the Widget.destroy all that is happening is the struct is being set to null, does this need to happen with all of child widgets for D's GC to clean them up?

Like I said in Terminix I am calling destroy on the enclosing class (which is a Stack) that I remove but I'm still having the issue of the destructor never being called. I'm wondering if I should try explicitly calling destroy on the Terminal widget and/or the rest of the widget heirarchy?

Having said that, it's quite possible I have a reference issue somewhere else that is eluding me. These memory issues are like whack-a-mole sometimes.

[MikeWey]

In theory if you destroy a container like Stack it will decrease the reference count of all it's children by one. And if at that point GtkD holds the last reference the GC is free to destroy the object.

[gnunn1]

Just to summarize the original problem, the vte has a finalizer that is cleaning up some file descriptors and @egmontkob noticed this wasn't happening in Terminix. I can see that I'm having an issue somewhere where either a D or GTK reference is being held onto which is preventing my high level Terminal Box, which adds the Terminal (VTE) widget, is not being GC'ed.

As a workaround and test, I thought I'd simply try to removing the VTE widget directly, destroying it and setting the reference to null. From a D perspective, I can see that the D destructor is called however the VTE finalizer in GTK is not called. I quickly realized what the problem was, calling Widget.destroy() sets the struct to null, so that when ObjectG destructor is called it no longer has a reference to the struct and can't unref it. Thus the finalizer never actually gets called.

The solution is to comment out the setStruct(null) in Widget.d:

		public void destroy()
		{
			gtk_widget_destroy(gtkWidget);
			//setStruct(null);
		}

Once this is done, the VTE finalizer is called as expected.

I'm not sure why the struct is being set to null there, maybe this setting it to null should be moved to the ObjectG destructor?

[MikeWey]

The setStruct(null) was there to stop the destructor to call functions on an invalid object.

But it seems that gtk_widget_destroy doesn't destroy the widget if there are additional references to the widget. ObjectG does have an destroy notify callback, but currently it doesn't unref the object.

I'll try and see if this is really whats causing the problems.

[csoriano1618]

Just passing by and saw this, just a clarification:
"But it seems that gtk_widget_destroy doesn't destroy the widget if there are additional references to the widget"
gtk_widget_destroy calls g_object_dispose, effectively destroying the widget and its associated memory, regardless of how many references are held
https://git.gnome.org/browse/gtk+/tree/gtk/gtkwidget.c#n4128

[MikeWey]

That looks to be the case, judging by the code.

Although the documentation states:

It's important to notice that gtk_widget_destroy() will only cause the @widget to be finalized if no additional references, acquired using g_object_ref(), are held on it.

So it might be destroyed but not finalized.

[csoriano1618]

oh right

[gnunn1]

@MikeWey I fixed the original issue that sparked this, the two issues that remain on your side are as follows:

1. Calling destroy prevents the widget finalizer from being called.

2. As a feature request, I think adding the ability to remove events is important and something that should be supported.

[gnunn1]

Btw, I'm not sure how big a deal point 1 is, I managed to work around the issue of calling destroy on VTE preventing finalization. However there are a few spots where I've had to explicitly call destroy on other gtk widgets. Not sure how prevalent finalization is in the gtk outside of VTE.

[csoriano1618]

Hey Gerald, if I understood correctly your question... gtk finalization is "mandatory", as any other GObject, so every gtk widget must free its resources in there.
And therefore finalization must happen and be called when a widget is no longer in use.

[MikeWey]

I've added a unref call in de destroy callback.
That should take care of the reference GtkD holds.

[gnunn1]

Thanks @MikeWey, much appreciated.

[gnunn1]

Closing, I added removing event handlers as a separate issue #167 and @MikeWey took care of the other issue.