Experimental memory leak fix 🎉

This commit experiments with refactoring package `core/intern` to use
the newly added `runtime.AddCleanup` and `weak.Pointer` APIs from Go
1.24.

This commit has been tested against issues #106 and #126 to ensure
stability. It is still not recommended to use this in production
until further testing has been done, however.

Author: diamondburned

gir/cmd/gir-generate/gendata/gendata.go

@@ -404,11 +404,10 @@ func GLibVariantIter(nsgen *girgen.NamespaceGenerator) error {
 			}
 
 			_variant := (*Variant)(gextras.NewStructNative(unsafe.Pointer(_cret)))
-			runtime.SetFinalizer(
+			runtime.AddCleanup(
 				gextras.StructIntern(unsafe.Pointer(_variant)),
-				func(intern *struct{ C unsafe.Pointer }) {
-					C.g_variant_unref((*C.GVariant)(intern.C))
-				},
+				C.g_variant_unref,
+				(*C.GVariant)(unsafe.Pointer(_cret)),
 			)
 			return _variant, nil
 		}
@@ -428,11 +427,10 @@ func GLibVariantIter(nsgen *girgen.NamespaceGenerator) error {
 				}
 
 				variant := (*Variant)(gextras.NewStructNative(unsafe.Pointer(item)))
-				runtime.SetFinalizer(
+				runtime.AddCleanup(
 					gextras.StructIntern(unsafe.Pointer(variant)),
-					func(intern *struct{ C unsafe.Pointer }) {
-						C.g_variant_unref((*C.GVariant)(intern.C))
-					},
+					C.g_variant_unref,
+					(*C.GVariant)(unsafe.Pointer(item)),
 				)
 
 				return variant
@@ -563,11 +561,10 @@ func GioArrayUseBytes(nsgen *girgen.NamespaceGenerator) error {
 			)
 
 			_bytes := (*Bytes)(gextras.NewStructNative(unsafe.Pointer(v)))
-			runtime.SetFinalizer(
+			runtime.AddCleanup(
 				gextras.StructIntern(unsafe.Pointer(_bytes)),
-				func(intern *struct{ C unsafe.Pointer }) {
-					C.g_bytes_unref((*C.GBytes)(intern.C))
-				},
+				C.g_bytes_unref,
+				(*C.GBytes)(unsafe.Pointer(v)),
 			)
 
 			return _bytes

gir/girgen/generators/union.go

@@ -187,11 +187,12 @@ func (ug *UnionGenerator) Use(union *gir.Union) bool {
 		p.Linef("original := (*C.%s)(%s.underlying%s())", union.CType, ug.Recv(), ug.GoName)
 		p.Linef("copied := C.%s(original)", copyMethod.CIdentifier)
 		p.Linef("dst := (*%s)(gextras.NewStructNative(unsafe.Pointer(copied)))", ug.GoName)
-		p.Linef("runtime.SetFinalizer(")
+		p.Linef("runtime.AddCleanup(")
 		p.Linef("  gextras.StructIntern(unsafe.Pointer(dst)),")
-		p.Linef("  func(intern *struct{ C unsafe.Pointer }) {")
-		p.Linef(types.RecordPrintFree(ug.gen, &typRecord, "intern.C"))
-		p.Linef("},")
+		p.Linef("  func(ptr unsafe.Pointer) {")
+		p.Linef("    %s", types.RecordPrintFree(ug.gen, &typRecord, "ptr"))
+		p.Linef("  },")
+		p.Linef("  unsafe.Pointer(copied),")
 		p.Linef(")")
 		p.Linef("return dst")
 
@@ -264,12 +265,12 @@ func (ug *UnionGenerator) Use(union *gir.Union) bool {
 			ug.hdr.Import("unsafe")
 			ug.hdr.ImportCore("gextras")
 
-			p.Linef("runtime.SetFinalizer(")
-			// dst is ASSUMED TO BE A POINTER.
+			p.Linef("runtime.AddCleanup(")
 			p.Linef("  gextras.StructIntern(unsafe.Pointer(dst)),")
-			p.Linef("  func(intern *struct{ C unsafe.Pointer }) {")
-			p.Linef(types.RecordPrintFree(ug.gen, &typRecord, "intern.C"))
-			p.Linef("},")
+			p.Linef("  func(ptr unsafe.Pointer) {")
+			p.Linef("    %s", types.RecordPrintFree(ug.gen, &typRecord, "ptr"))
+			p.Linef("  },")
+			p.Linef("  unsafe.Pointer(cpy),")
 			p.Linef(")")
 		}
 

gir/girgen/types/typeconv/c-go.go

@@ -222,11 +222,11 @@ func (conv *Converter) cgoArrayConverter(value *ValueConverted) bool {
 			value.p.Linef("var len C.gsize")
 			// If we're fully getting the backing array, then we can just steal
 			// it (since we own it now), which is less copying.
-			value.p.Linef("p := C.g_byte_array_steal(&%s, &len)", value.In.Name)
-			value.p.Linef("%s = unsafe.Slice((*byte)(p), uint(len))", value.Out.Set)
-			value.p.Linef("runtime.SetFinalizer(&%s, func(v *[]byte) {", value.OutName)
-			value.p.Linef("  C.free(unsafe.Pointer(&(*v)[0]))")
-			value.p.Linef("})")
+			value.p.Linef("p := (*byte)(C.g_byte_array_steal(&%s, &len))", value.In.Name)
+			value.p.Linef("%s = unsafe.Slice(p, uint(len))", value.Out.Set)
+			value.p.Linef("runtime.AddCleanup(&%s, func(v *[]byte) {", value.OutName)
+			value.p.Linef("  C.free(unsafe.Pointer(p))")
+			value.p.Linef("}, p)")
 			value.p.Ascend()
 			return true
 		}
@@ -437,11 +437,7 @@ func (conv *Converter) cgoConverter(value *ValueConverted) bool {
 		// Set this to be freed if we have the ownership now.
 		if value.ShouldFree() {
 			value.header.Import("runtime")
-
-			// https://pkg.go.dev/github.com/diamondburned/gotk4/pkg/core/glib?utm_source=godoc#Value
-			value.p.Linef("runtime.SetFinalizer(%s, func(v *coreglib.Value) {", value.OutName)
-			value.p.Linef("  C.g_value_unset((*C.GValue)(unsafe.Pointer(v.Native())))")
-			value.p.Linef("})")
+			value.p.Linef("runtime.AddCleanup(%s, C.g_value_unset, (*C.GValue)(unsafe.Pointer(v.Native())))", value.OutName)
 		}
 		return true
 
@@ -502,11 +498,11 @@ func (conv *Converter) cgoConverter(value *ValueConverted) bool {
 			value.p.Linef("C.%s(%s)", ref, value.InNamePtr(1))
 		}
 
-		value.p.Linef("runtime.SetFinalizer(%s%s, func(v %s%s) {",
+		value.p.Linef("runtime.AddCleanup(%s%s, func(v %s%s) {",
 			value.OutInPtr(1), value.OutName, value.OutPtr(1), value.Out.Type)
 		value.p.Linef("C.%s((%s%s)(unsafe.Pointer(v.Native())))",
 			unref, value.InPtr(1), value.In.Type)
-		value.p.Linef("})")
+		value.p.Linef("}, %s%s)", value.OutInPtr(1), value.OutName)
 
 		return true
 	}
@@ -642,17 +638,12 @@ func (conv *Converter) cgoConverter(value *ValueConverted) bool {
 		// We can take ownership if the type can be reference-counted anyway.
 		if value.ShouldFree() || unref {
 			value.header.Import("runtime")
-			value.vtmpl(`
-				runtime.SetFinalizer(
-					gextras.StructIntern(unsafe.Pointer(<.OutInPtr 1><.OutName>)),
-					func(intern *struct{ C unsafe.Pointer }) {
-			`)
-			if value.fail {
-				value.Logln(logger.Debug, "SetFinalizer set fail")
-			}
-
-			value.p.Linef(types.RecordPrintFree(value.conv.fgen, value.Resolved.Extern, "intern.C"))
-			value.p.Linef("},")
+			value.p.Linef("runtime.AddCleanup(")
+			value.p.Linef("  gextras.StructIntern(unsafe.Pointer(%s%s)),", value.OutInPtr(1), value.OutName)
+			value.p.Linef("  func(ptr unsafe.Pointer) {")
+			value.p.Linef("    %s", types.RecordPrintFree(value.conv.fgen, value.Resolved.Extern, "ptr"))
+			value.p.Linef("  },")
+			value.p.Linef("  unsafe.Pointer(%s),", value.InNamePtr(1))
 			value.p.Linef(")")
 		}
 

gir/girgen/types/typeconv/go-c.go

@@ -653,18 +653,6 @@ func (conv *Converter) gocConverter(value *ValueConverted) bool {
 		value.vtmpl(
 			"<.Out.Set> = <.OutCast 1>(gextras.StructNative(unsafe.Pointer(<.InNamePtr 1>)))",
 		)
-
-		// If ShouldFree is true, then ideally, we'll be freeing the C copy of
-		// the value once we're done. However, since the C code is taking
-		// ownership, we can't do that, since the Finalizer won't know that and
-		// free the record. Instead, if we cannot free the data once we're done,
-		// then we detach the finalizer so Go can't.
-		if !value.ShouldFree() && types.RecordHasRef(v) == nil {
-			value.header.Import("runtime")
-			value.vtmpl(
-				"runtime.SetFinalizer(gextras.StructIntern(unsafe.Pointer(<.InNamePtr 1>)), nil)",
-			)
-		}
 		return true
 
 	case *gir.Callback:

gir/girgen/types/typeconv/valueconverted.go

@@ -816,7 +816,7 @@ func (value *ValueConverted) OutInNamePtr(want int) string {
 }
 
 // OutInPtr returns the left-hand side for the output name and type SPECIFICALLY
-// for inputting the output name elsewhere, like giving it to SetFinalizer.
+// for inputting the output name elsewhere, like giving it to AddCleanup.
 func (value *ValueConverted) OutInPtr(want int) string {
 	has := types.CountPtr(value.Out.Type)
 	return value._ptr(has, want)

pkg/core/closure/closure.go

@@ -33,3 +33,14 @@ func (r *Registry) Load(gclosure unsafe.Pointer) *FuncStack {
 func (r *Registry) Delete(gclosure unsafe.Pointer) {
 	r.reg.Delete(uintptr(gclosure))
 }
+
+// Len returns the number of closures in the registry.
+// Use this for debugging purposes only.
+func (r *Registry) Len() int {
+	var i int
+	r.reg.Range(func(_, _ interface{}) bool {
+		i++
+		return true
+	})
+	return i
+}

pkg/core/gdebug/objectinfo.go

@@ -0,0 +1,27 @@
+package gdebug
+
+// #cgo pkg-config: gobject-2.0
+// #include <glib-object.h>
+//
+// const gchar *gotk4_object_type_name(gpointer obj) {
+//   return G_OBJECT_TYPE_NAME(obj);
+// };
+import "C"
+import (
+	"fmt"
+	"log/slog"
+	"unsafe"
+)
+
+// ObjectInfo returns a slog.Attr with the GObject's pointer and type.
+func ObjectInfo(obj unsafe.Pointer) slog.Attr {
+	return slog.Group(
+		"gobject",
+		slog.String("ptr", fmt.Sprintf("%p", obj)),
+		slog.String("type", C.GoString(C.gotk4_object_type_name(C.gpointer(obj)))),
+		slog.Int("refs", objRefCount(obj)))
+}
+
+func objRefCount(obj unsafe.Pointer) int {
+	return int(C.g_atomic_int_get((*C.gint)(unsafe.Pointer(&(*C.GObject)(obj).ref_count))))
+}

pkg/core/glib/glib.go

@@ -14,13 +14,15 @@ import "C"
 import (
 	"errors"
 	"log"
+	"log/slog"
 	"reflect"
 	"runtime"
 	"sync"
 	"unsafe"
 
 	"github.com/diamondburned/gotk4/pkg/core/closure"
 	"github.com/diamondburned/gotk4/pkg/core/gbox"
+	"github.com/diamondburned/gotk4/pkg/core/gdebug"
 	"github.com/diamondburned/gotk4/pkg/core/intern"
 )
 
@@ -451,32 +453,6 @@ func SourceRemove(src SourceHandle) bool {
 	return gobool(C.g_source_remove(C.guint(src)))
 }
 
-// WipeAllClosures wipes all the Go closures associated with the given object
-// away. BE EXTREMELY CAREFUL WHEN USING THIS FUNCTION! If not careful, your
-// program WILL crash!
-//
-// There is only one specific use case for this function: if your object has
-// closures connected to it where these closures capture the object itself, then
-// it might create a cyclic dependency on the GC, preventing its finalizer from
-// ever running. This will cause the program to leak memory. As a temporary
-// hack, this function is introduced for cases where the programmer knows for
-// sure that the object will never be used again, and it is significant enough
-// of a leak that having a workaround is better than not.
-//
-// Deprecated: this function is dangerous and should not be used. Using this
-// function now causes a panic.
-func WipeAllClosures(objector Objector) {
-	panic("WipeAllClosures is deprecated and should not be used")
-}
-
-// Destroy destroys the Go reference to the given object. The object must not be
-// used ever again; it is the caller's responsibility to ensure that it will
-// never be used again. Resurrecting the object again is undefined behavior.
-func Destroy(objector Objector) {
-	v := BaseObject(objector)
-	v.destroy()
-}
-
 // SignalHandle is the ID of a signal handler.
 type SignalHandle uint
 
@@ -505,6 +481,7 @@ func ConnectGeneratedClosure(
 	userData := C.gpointer(gbox.Assign(data))
 
 	gclosure := C.g_cclosure_new(C.GCallback(tramp), userData, (*[0]byte)(C._gotk4_removeGeneratedClosure))
+	// C.g_closure_add_invalidate_notifier(gclosure, userData, (*[0]byte)(C._gotk4_removeGeneratedClosure))
 
 	// Hold the GClosure reference.
 	data.GClosure = uintptr(unsafe.Pointer(gclosure))
@@ -531,29 +508,28 @@ func ConnectGeneratedClosure(
 func ConnectedGeneratedClosure(closureData uintptr) *closure.FuncStack {
 	data, _ := gbox.Get(closureData).(*generatedClosureData)
 	if data == nil {
-		return nil
+		slog.Error(
+			"ConnectedGeneratedClosure: closure data not found in gbox",
+			"data", closureData)
+		panic("fatal error during callback")
 	}
 
 	// Get the function value associated with this callback closure.
 	box := intern.TryGet(unsafe.Pointer(data.GObject))
 	if box == nil {
-		log.Printf(
-			"gotk4: warning: object %s %v cannot be resurrected",
-			typeFromObject(unsafe.Pointer(data.GObject)),
-			unsafe.Pointer(data.GObject),
-		)
-		return nil
+		slog.Error(
+			"ConnectedGeneratedClosure: object cannot be resurrected for callback",
+			gdebug.ObjectInfo(unsafe.Pointer(data.GObject)))
+		panic("fatal error during callback")
 	}
 
 	fs := box.Closures().Load(unsafe.Pointer(data.GClosure))
 	if fs == nil {
-		log.Printf(
-			"gotk4: warning: object %s %v missing closure %v",
-			typeFromObject(unsafe.Pointer(data.GObject)),
-			unsafe.Pointer(data.GObject),
-			unsafe.Pointer(data.GClosure),
-		)
-		return nil
+		slog.Error(
+			"ConnectedGeneratedClosure: closure not found",
+			"n_closures", box.Closures().Len(),
+			gdebug.ObjectInfo(unsafe.Pointer(data.GObject)))
+		panic("fatal error during callback")
 	}
 
 	return fs
@@ -662,10 +638,6 @@ func newObject(ptr unsafe.Pointer, take bool) *Object {
 	}
 }
 
-func (v *Object) destroy() {
-	intern.Free(v.box)
-}
-
 // Cast casts v to the concrete Go type (e.g. *Object to *gtk.Entry).
 //
 //go:nosplit
@@ -1025,7 +997,7 @@ func marshalValue(p uintptr) (interface{}, error) {
 	}
 
 	v := &value{(*C.GValue)(unsafe.Pointer(c))}
-	runtime.SetFinalizer(v, (*value).unset)
+	runtime.AddCleanup(v, func(v *C.GValue) { C.g_value_unset(v) }, v.gvalue)
 
 	return &Value{v}, nil
 }
@@ -1058,7 +1030,7 @@ func AllocateValue() *Value {
 	//An allocated GValue is not guaranteed to hold a value that can be unset
 	//We need to double check before unsetting, to prevent:
 	//`g_value_unset: assertion 'G_IS_VALUE (value)' failed`
-	runtime.SetFinalizer(v.value, (*value).unset)
+	runtime.AddCleanup(v.value, func(v *C.GValue) { C.g_value_unset(v) }, v.gvalue)
 
 	return v
 }

pkg/core/glib/paramspec.go

@@ -209,9 +209,7 @@ func ParamSpecTake(ptr unsafe.Pointer, take bool) *ParamSpec {
 	if !take {
 		C.g_param_spec_ref(p.intern)
 	}
-	runtime.SetFinalizer(p.paramSpec, func(p *paramSpec) {
-		C.g_param_spec_unref(p.intern)
-	})
+	runtime.AddCleanup(p.paramSpec, func(p *C.GParamSpec) { C.g_param_spec_unref(p) }, p.intern)
 	return p
 }
 

pkg/core/intern/intern.c

@@ -1,14 +1,9 @@
 #include "intern.h"
 
-const gchar *gotk4_object_type_name(gpointer obj) {
-  return G_OBJECT_TYPE_NAME(obj);
-};
-
 gboolean gotk4_intern_remove_toggle_ref(gpointer obj) {
   // First, remove the toggle reference. This forces the object to be freed,
   // calling any necessary finalizers.
-  g_object_remove_toggle_ref(G_OBJECT(obj), (GToggleNotify)goToggleNotify,
-                             NULL);
+  g_object_remove_toggle_ref(G_OBJECT(obj), (GToggleNotify)goToggleNotify, NULL);
 
   // Only once the object is freed, we can remove it from the weak reference
   // registry, since now the finalizers will not be called anymore.