[Feature] Refactor Gtk callback setters and types

[diamondburned]

Is your feature request related to a problem? Please describe.

Kind of. While working on a pull request to add TreeViewSearchEqualFunc, I've noticed 3 (three) uglies:

• All callback types right now take in a ...interface{} (variadic empty interface) argument. This is superfluous; Go closures can reference values outside.

• There is no clean up of callbacks (GDestroyNotify is not used).

• Each Set.*Func method and/or type seems to require its own map with a mutex and serial incrementing ID, which is redundant.

Describe the solution you'd like

I propose we address this problem in 3 simple steps:

1. Remove all ...interface{} arguments from the callback types. This allows us to store the Go callback function value cleanly without requiring a struct to wrap the interfaces.

2. Move the callback containers off of the package and into an internal package like so.

3. Use the created abstraction to clean up callbacks using the GDestroyNotify callback:

// extern void callbackDelete(gpointer ptr);
import "C"

//export callbackDelete
func callbackDelete(ptr C.gpointer) {
	callback.Delete(uintptr(ptr))
}

// An example usage (note that a Go bug requires doing the weird casting.)
C.hdy_combo_row_bind_model(c.native(), v1, nil, nil, C.gpointer(uintptr(0)), (*[0]byte)(C.callbackDelete))

Describe alternatives you've considered

Since this change will break existing code, I have considered to just go along with adding some instead of all the solutions above. Despite that, I still think the current code is worth refactoring and breaking.

Additional Information

My generated libhandy binding implements something similar to this.

[diamondburned]

I can probably work on a PR for this if the idea sounds good.

[diamondburned]

It seems like the memory leak bug that I struggled to fix for more than a year actually has to do with this issue itself. The irony is that it took me a few weeks to even realize that this is the actual reason.

I'll probably get this done within this (and next) week.

[diamondburned]

While working on the upcoming pull request, I have noticed a problem that I think might be affecting the entire library rather than just what I am patching up in the library.

Prior to my discovery, the type gdk.PixbufLoader's constructor would take a reference and later unreference the returned object. This is incorrect; the code should have assumed a reference and not take another one. The newly constructed object returned from g_object_new is defined to have 1 floating reference, not 0 references. It is worth noting that using a RefSink did not work. If the fact about a single floating reference is true, then RefSink should've left the reference count alone and remove the floating flag. However, it seems to take its own reference, as doing RefSink leaks memory. This might be because gdk.PixbufLoader does not have a floating reference, for some reason, but this is irrelevant for this object, as we're not taking another reference ourselves.

The incorrect referencing has led me to the second issue of Gtk complaining about unreferencing an invalid object. Although this may seem like Gtk is complaining about PixbufLoader, it was actually that Go should've taken a reference from the returned Pixbuf. As it is, any object that we return through these functions should have a reference taken by Go, with the only difference being whether or not we actually need to take the reference. This is because the new object returned may be kept alive for longer than the lifespan of the object it is returned from. In the case of a transfer-full, it could be assumed that we have already taken a reference, therefore we do not need to do it again. With transfer-none (what gdk_pixbuf_loader_get_pixbuf returns), then we must take a reference of our own.

Because of this, in the upcoming pull request, I will be clarifying the glib.Take function as well as adding a new function called glib.AssumeOwnership which does what glib.Take does except for taking a reference from the object:

// Take wraps a unsafe.Pointer as a glib.Object, taking ownership of it.
// This function is exported for visibility in other gotk3 packages and
// is not meant to be used by applications.
//
// To be clear, this should mostly be used when Gtk says "transfer none". Refer
// to AssumeOwnership for more details.
func Take(ptr unsafe.Pointer) *Object {
	obj := newObject(ToGObject(ptr))
	obj.RefSink()
	runtime.SetFinalizer(obj, (*Object).Unref)
	return obj
}

// AssumeOwnership is similar to Take, except the function does not take a
// reference. This is usually used for newly constructed objects that for some
// reason does not have an initial floating reference.
//
// To be clear, this should often be used when Gtk says "transfer full", as it
// means the ownership is transferred to the caller, so we can assume that much.
// This is in contrary to Take, which is used when Gtk says "transfer none", as
// we're now referencing to an object that might possibly be kept, so we should
// mark as such.
func AssumeOwnership(ptr unsafe.Pointer) *Object {
	obj := newObject(ToGObject(ptr))
	runtime.SetFinalizer(obj, (*Object).Unref)
	return obj
}

The new implementation of Take has one minor difference: it now calls RefSink instead of checking for IsFloating. This is because RefSink will not take a reference if the object is floating (i.e. newly constructed) and "sink" the object, therefore assuming that the initial reference is now Go's. Otherwise, it will take a reference as usual.

As a side note, this discovery may be related to PR #507.

[diamondburned]

This PR also helped me find out another issue: an object that is being signal-connected to can be referenced from inside the signal callback, sometimes creating a circular reference in some cases. In these cases, neither the object nor the callback will ever be freed, as nothing would call the destructors needed to unregister the callback in the internal registry.

Specifically with gdk.PixbufLoader, the documentation listed Close() to not unreference the object. Take this simple code:

l, _ := gdk.PixbufLoaderNew()
l.Connect("area-prepared", func() {
    p, _ := l.GetPixbuf()
    image.SetFromPixbuf(p)
})

io.Copy(l, file)
l.Close()

This code has a memory leak: the area-prepared callback is referencing l, so l is not finalized until the callback is gone. Because l is not finalized, it will never be unreferenced, so its connected signals will never be cleaned up, therefore the callback will also never be cleaned up.

There are a few ways to solve this problem. One way is to spend a lot of effort correcting the callback behaviors. For example, area-prepared is only ever called once per object, so we could remove the callback immediately after it's called. This requires a lot of edge cases and state-keeping, however, so it's not the most ideal way.

The other way is a lot more obvious: don't reference the object it is being connected to. The fix to the above code (in diff format) would then be:

-l.Connect("area-prepared", func() {
+l.Connect("area-prepared", func(l *gdk.PixbufLoader) {
     p, _ := l.GetPixbuf()

This, combined with the new changes commented above, completely fixed issues with the PixbufLoader leaking.

After working out such a simple fix to the above problem, I am now considering the choice of forcing Connect and ConnectAfter to enforce the callback to have at least the argument of the object type it is connecting to. Any memory leak is intentional, so having such an explicit error will better warn developers not to make the same mistake.

---

As a side note, I'm unsure how well this will pan out if said object needs to be in a structure, perhaps in a situation like this:

type ChatBox struct {
    gtk.TextView
    Loader *gdk.PixbufLoader

    State State
}

func (box *ChatBox) Method() {
    box.Loader.Connect("size-allocate", func(loader *gdk.PixbufLoader) {
        // Here, we're dereferencing box to get the state, which might keep box
        // alive along with the PixbufLoader, causing a circular reference.
        loader.SetSize(box.State.Width, box.State.Height)
    })
}