From 4219107dd2e65cee7535fcd941ce718da2a1396d Mon Sep 17 00:00:00 2001 From: diamondburned Date: Fri, 9 Feb 2024 01:07:02 -0800 Subject: [PATCH] Fix crash after regressions from #126 Fixes #130 --- pkg/core/glib/connect.go | 2 +- pkg/core/intern/intern.c | 18 ++++++ pkg/core/intern/intern.go | 94 +++++++++++++++----------------- pkg/core/intern/intern.h | 6 ++ pkg/core/intern/intern_export.go | 63 ++++++++++++++++++++- 5 files changed, 130 insertions(+), 53 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..ff0119f26 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 ( @@ -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") } } 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..90ac26c0b 100644 --- a/pkg/core/intern/intern_export.go +++ b/pkg/core/intern/intern_export.go @@ -1,10 +1,14 @@ package intern // #cgo pkg-config: gobject-2.0 -// #include +// #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) + } +}