Fix crash after regressions from #126

Fixes #130

Author: diamondburned

pkg/core/glib/connect.go

@@ -66,7 +66,7 @@ func closureNew(v *Object, f interface{}) *C.GClosure {
 	closures := closure.RegistryType.Get(v.box)
 	closures.Register(unsafe.Pointer(gclosure), fs)
 
-	// C.g_object_watch_closure(v.native(), gclosure)
+	C.g_object_watch_closure(v.native(), gclosure)
 	C.g_closure_set_meta_marshal(gclosure, C.gpointer(v.Native()), (*[0]byte)(C._gotk4_goMarshal))
 	C.g_closure_add_finalize_notifier(gclosure, C.gpointer(v.Native()), (*[0]byte)(C._gotk4_removeClosure))
 

pkg/core/intern/intern.c

@@ -0,0 +1,18 @@
+#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);
+
+  // Only once the object is freed, we can remove it from the weak reference
+  // registry, since now the finalizers will not be called anymore.
+  goFinishRemovingToggleRef(obj);
+
+  return FALSE;
+}

pkg/core/intern/intern.go

@@ -2,14 +2,7 @@
 package intern
 
 // #cgo pkg-config: gobject-2.0
-// #include <glib-object.h>
-//
-// extern void goToggleNotify(gpointer, GObject*, gboolean);
-// static const gchar* gotk4_object_type_name(gpointer obj) { return G_OBJECT_TYPE_NAME(obj); };
-//
-// gboolean gotk4_intern_remove_toggle_ref(gpointer obj) {
-//   g_object_remove_toggle_ref(G_OBJECT(obj), (GToggleNotify)goToggleNotify, NULL);
-// }
+// #include "intern.h"
 import "C"
 
 import (
@@ -89,6 +82,7 @@ type Box struct {
 	gobject unsafe.Pointer
 	dummy   *boxDummy
 	data    [maxTypesAllowed]unsafe.Pointer
+	done    bool
 }
 
 type boxDummy struct {
@@ -178,7 +172,14 @@ var shared = struct {
 //go:nosplit
 func TryGet(gobject unsafe.Pointer) *Box {
 	shared.mu.RLock()
+
 	box, _ := gets(gobject)
+	if box != nil && box.done {
+		log.Panicf(
+			"gotk4: critical: %s TryGet called on a finalized object",
+			objInfo(gobject))
+	}
+
 	shared.mu.RUnlock()
 	return box
 }
@@ -285,64 +286,57 @@ func finalizeBox(dummy *boxDummy) {
 	}
 
 	shared.mu.Lock()
+	defer shared.mu.Unlock()
 
 	box, strong := gets(dummy.gobject)
 	if box == nil {
 		log.Print("gotk4: intern: finalizer got unknown gobject ", dummy.gobject, ", ignoring")
-		shared.mu.Unlock()
 		return
 	}
 
-	var objInfoRes string
-	if toggleRefs != nil {
-		objInfoRes = objInfo(dummy.gobject)
-		toggleRefs.Println(objInfoRes, "finalizeBox: acquiring lock...")
-	}
+	// Always delegate the finalization to the next cycle.
+	// This won't be the case once goFinishRemovingToggleRef is called.
+	runtime.SetFinalizer(dummy, finalizeBox)
 
-	if strong {
-		// If the closures are strong-referenced, then they might still be
-		// referenced from the C side, and those closures might access this
+	if box.done || strong {
+		// If box.done:
+		// The finalizer is again called before goFinishRemovingToggleRef gets
+		// the chance to run. Set the finalizer again and move on.
+		//
+		// If strong: the closures are strong-referenced, then they might still
+		// be referenced from the C side, and those closures might access this
 		// object. Don't free.
 
-		// Delegate finalizing to the next cycle.
-		runtime.SetFinalizer(dummy, finalizeBox)
-
-		shared.mu.Unlock()
-
 		if toggleRefs != nil {
-			toggleRefs.Println(objInfoRes, "finalizeBox: moving finalize to next GC cycle")
+			if box.done {
+				toggleRefs.Println(
+					objInfo(dummy.gobject),
+					"finalizeBox: finalizeBox called on already finalized object")
+			} else {
+				toggleRefs.Println(
+					objInfo(dummy.gobject),
+					"finalizeBox: moving finalize to next GC cycle since object is still strong")
+			}
 		}
-	} else {
-		// If the closures are weak-referenced, then the object reference hasn't
-		// been toggled yet. Since the object is going away and we're still
-		// weakly referenced, we can wipe the closures away.
-		delete(shared.weak, dummy.gobject)
 
-		shared.mu.Unlock()
+		return
+	}
 
-		// Unreference the object. This will potentially free the object as
-		// well. The closures are definitely gone at this point.
-		// C.g_object_remove_toggle_ref(
-		// 	(*C.GObject)(unsafe.Pointer(dummy.gobject)),
-		// 	(*[0]byte)(C.goToggleNotify), nil,
-		// )
-
-		// Do this in the main loop instead. This is because finalizers are
-		// called in a finalizer thread, and our remove_toggle_ref might be
-		// destroying other main loop objects.
-		C.g_main_context_invoke(
-			nil, // nil means the default main context
-			(*[0]byte)(C.gotk4_intern_remove_toggle_ref),
-			C.gpointer(dummy.gobject))
+	// Mark the box as being removed "done" so that we don't double-free it.
+	box.done = true
 
-		if toggleRefs != nil {
-			toggleRefs.Println(objInfoRes,
-				"finalizeBox: remove_toggle_ref queued for next main loop iteration")
-		}
+	// Do this in the main loop instead. This is because finalizers are
+	// called in a finalizer thread, and our remove_toggle_ref might be
+	// destroying other main loop objects.
+	C.g_main_context_invoke(
+		nil, // nil means the default main context
+		(*[0]byte)(C.gotk4_intern_remove_toggle_ref),
+		C.gpointer(dummy.gobject))
 
-		if objectProfile != nil {
-			objectProfile.Remove(dummy.gobject)
-		}
+	if toggleRefs != nil {
+		toggleRefs.Println(
+			objInfo(dummy.gobject),
+			"finalizeBox: remove_toggle_ref queued for next main loop iteration")
 	}
 }
 

pkg/core/intern/intern.h

@@ -0,0 +1,6 @@
+#include <glib-object.h>
+
+extern void goToggleNotify(gpointer, GObject *, gboolean);
+extern void goFinishRemovingToggleRef(gpointer);
+const gchar *gotk4_object_type_name(gpointer obj);
+gboolean gotk4_intern_remove_toggle_ref(gpointer obj);

pkg/core/intern/intern_export.go

@@ -1,10 +1,14 @@
 package intern
 
 // #cgo pkg-config: gobject-2.0
-// #include <glib-object.h>
+// #include "intern.h"
 import "C"
 
-import "unsafe"
+import (
+	"log"
+	"runtime"
+	"unsafe"
+)
 
 // goToggleNotify is called by GLib on each toggle notification. It doesn't
 // actually free anything and relies on Box's finalizer to free both the box and
@@ -32,3 +36,58 @@ func goToggleNotify(_ C.gpointer, obj *C.GObject, isLastInt C.gboolean) {
 		toggleRefs.Println(objInfo(unsafe.Pointer(obj)), "goToggleNotify: is last =", isLast)
 	}
 }
+
+// finishRemovingToggleRef is called after the toggle reference removal routine
+// is dispatched in the main loop. It removes the GObject from the strong and
+// weak global maps and unsets the finalizer.
+//
+//go:nosplit
+//export goFinishRemovingToggleRef
+func goFinishRemovingToggleRef(gobject unsafe.Pointer) {
+	if toggleRefs != nil {
+		toggleRefs.Printf("goFinishRemovingToggleRef: called on %p", gobject)
+	}
+
+	shared.mu.Lock()
+	defer shared.mu.Unlock()
+
+	box, strong := gets(gobject)
+	if box == nil {
+		// Extremely weird error. This should never happen.
+		log.Printf(
+			"gotk4: critical: %p: finishRemovingToggleRef called on unknown object",
+			gobject)
+		return
+	}
+
+	if strong {
+		// Panic here, else we're memory leaking.
+		log.Panicf(
+			"gotk4: critical: %p: finishRemovingToggleRef cannot be called on strongly-referenced object (unexpectedly resurrected?)",
+			gobject)
+	}
+
+	if !box.done {
+		log.Panicf(
+			"gotk4: critical: %p: finishRemovingToggleRef cannot be called with finalizer still set",
+			gobject)
+	}
+
+	// If the closures are weak-referenced, then the object reference hasn't
+	// been toggled yet. Since the object is going away and we're still
+	// weakly referenced, we can wipe the closures away.
+	//
+	// Finally clear the object data off the registry.
+	delete(shared.weak, gobject)
+
+	// Clear the finalizer.
+	runtime.SetFinalizer(box.dummy, nil)
+
+	// Keep the box alive until the end of the function just in case the
+	// finalizer is called again.
+	runtime.KeepAlive(box.dummy)
+
+	if objectProfile != nil {
+		objectProfile.Remove(gobject)
+	}
+}