From a1bfe15be55a76298828d93afb6d9e16607d0b03 Mon Sep 17 00:00:00 2001 From: diamondburned Date: Fri, 9 Feb 2024 01:07:02 -0800 Subject: [PATCH] Fix intern.Box being freed before finalizers are called Related issue: #130 --- pkg/core/glib/connect.go | 2 +- pkg/core/intern/intern.c | 18 ++++++++++++++ pkg/core/intern/intern.go | 25 +++---------------- pkg/core/intern/intern.h | 6 +++++ pkg/core/intern/intern_export.go | 41 ++++++++++++++++++++++++++++++-- 5 files changed, 67 insertions(+), 25 deletions(-) create mode 100644 pkg/core/intern/intern.c create mode 100644 pkg/core/intern/intern.h diff --git a/pkg/core/glib/connect.go b/pkg/core/glib/connect.go index 9101cf2df..8aa46f635 100644 --- a/pkg/core/glib/connect.go +++ b/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)) diff --git a/pkg/core/intern/intern.c b/pkg/core/intern/intern.c new file mode 100644 index 000000000..6ec2e1d05 --- /dev/null +++ b/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; +} diff --git a/pkg/core/intern/intern.go b/pkg/core/intern/intern.go index 9bc1c30aa..2858e154e 100644 --- a/pkg/core/intern/intern.go +++ b/pkg/core/intern/intern.go @@ -2,14 +2,7 @@ package intern // #cgo pkg-config: gobject-2.0 -// #include -// -// 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 ( @@ -313,20 +306,6 @@ func finalizeBox(dummy *boxDummy) { toggleRefs.Println(objInfoRes, "finalizeBox: moving finalize to next GC cycle") } } 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() - - // 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. @@ -335,6 +314,8 @@ func finalizeBox(dummy *boxDummy) { (*[0]byte)(C.gotk4_intern_remove_toggle_ref), C.gpointer(dummy.gobject)) + shared.mu.Unlock() + if toggleRefs != nil { toggleRefs.Println(objInfoRes, "finalizeBox: remove_toggle_ref queued for next main loop iteration") diff --git a/pkg/core/intern/intern.h b/pkg/core/intern/intern.h new file mode 100644 index 000000000..c7d53edb1 --- /dev/null +++ b/pkg/core/intern/intern.h @@ -0,0 +1,6 @@ +#include + +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); diff --git a/pkg/core/intern/intern_export.go b/pkg/core/intern/intern_export.go index fae3f16e2..b56a8f59a 100644 --- a/pkg/core/intern/intern_export.go +++ b/pkg/core/intern/intern_export.go @@ -1,10 +1,13 @@ package intern // #cgo pkg-config: gobject-2.0 -// #include +// #include "intern.h" import "C" -import "unsafe" +import ( + "log" + "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 +35,37 @@ 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) { + 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: finishRemovingToggleRef called on unknown GObject %s", + objInfo(gobject)) + return + } + + if strong { + // Panic here, else we're memory leaking. + log.Panicf( + "gotk4: critical: finishRemovingToggleRef cannot be called on strong GObject %s (object unexpectedly resurrected?)", + objInfo(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) +}