From bb793a1e430c5307e93769f9289733d9f521c88a Mon Sep 17 00:00:00 2001 From: Ratchanan Srirattanamet Date: Mon, 21 Jul 2025 04:30:04 +0700 Subject: [PATCH 1/2] [WIP] feat(gobject): wrap GObject with a GC-able C++ wrapper TODO: test on Node 23+. Node-GTK itself requires update. Has PR. Instead of storing pointer to GObject directly in JS Object, store a C++ GC-able wrapper class. This enables us to track JS memory embedded inside GObject, avoiding reference loop. --- src/boxed.cc | 2 +- src/boxed.h | 2 +- src/function.cc | 8 +++ src/gi.cc | 12 ++--- src/gi.h | 2 +- src/gobject.cc | 116 +++++++++++++++++++++++++++++++++------- src/value.cc | 14 +++-- tests/module__system.js | 7 ++- 8 files changed, 128 insertions(+), 35 deletions(-) diff --git a/src/boxed.cc b/src/boxed.cc index 018b32e6..a2a7bef6 100644 --- a/src/boxed.cc +++ b/src/boxed.cc @@ -438,7 +438,7 @@ Local WrapperFromBoxed(GIBaseInfo *info, void *data, ResourceOwnership ow return instance.ToLocalChecked(); } -void* PointerFromWrapper(Local value) { +void* BoxedFromWrapper(Local value) { Local object = TO_OBJECT (value); g_assert(object->InternalFieldCount() > 0); void *boxed = object->GetAlignedPointerFromInternalField(0); diff --git a/src/boxed.h b/src/boxed.h index 9bbad52a..548caff4 100644 --- a/src/boxed.h +++ b/src/boxed.h @@ -32,6 +32,6 @@ class Boxed { Local MakeBoxedClass (GIBaseInfo *info); Local GetBoxedTemplate (GIBaseInfo *info, GType gtype); Local WrapperFromBoxed (GIBaseInfo *info, void *data, ResourceOwnership ownership = kNone); -void * PointerFromWrapper (Local); +void * BoxedFromWrapper (Local); }; diff --git a/src/function.cc b/src/function.cc index f650f52e..5ff99770 100644 --- a/src/function.cc +++ b/src/function.cc @@ -553,6 +553,14 @@ Local FunctionCall ( return jsReturnValue; } +static void* PointerFromWrapper(Local value) { + /* FIXME: find a better way to do this. */ + if (ValueIsInstanceOfGType(value, G_TYPE_OBJECT)) { + return GObjectFromWrapper(value); + } else { + return BoxedFromWrapper(value); + } +} /** * Creates the JS return value from the C arguments list diff --git a/src/gi.cc b/src/gi.cc index 28162569..92f89e00 100644 --- a/src/gi.cc +++ b/src/gi.cc @@ -121,7 +121,7 @@ NAN_METHOD(Bootstrap) { } NAN_METHOD(GetConstantValue) { - GIBaseInfo *gi_info = (GIBaseInfo *) GNodeJS::PointerFromWrapper (info[0]); + GIBaseInfo *gi_info = (GIBaseInfo *) GNodeJS::BoxedFromWrapper (info[0]); GITypeInfo *type_info = g_constant_info_get_type(gi_info); if (type_info == NULL) { @@ -213,8 +213,8 @@ NAN_METHOD(StructFieldSetter) { Local fieldInfo = info[1].As(); Local value = info[2]; - void *boxed = GNodeJS::PointerFromWrapper(boxedWrapper); - GIFieldInfo *field = (GIFieldInfo *) GNodeJS::PointerFromWrapper(fieldInfo); + void *boxed = GNodeJS::BoxedFromWrapper(boxedWrapper); + GIFieldInfo *field = (GIFieldInfo *) GNodeJS::BoxedFromWrapper(fieldInfo); GITypeInfo *field_type = g_field_info_get_type(field); g_assert(boxed); @@ -267,10 +267,10 @@ NAN_METHOD(StructFieldGetter) { return; } - auto boxed = GNodeJS::PointerFromWrapper(jsBoxed); + auto boxed = GNodeJS::BoxedFromWrapper(jsBoxed); // ref it because the unwrapped ref belongs to the JS wrapper BaseInfo fieldInfo = - g_base_info_ref((GIFieldInfo *) GNodeJS::PointerFromWrapper(jsFieldInfo)); + g_base_info_ref((GIFieldInfo *) GNodeJS::BoxedFromWrapper(jsFieldInfo)); if (boxed == NULL) { Nan::ThrowError("StructFieldGetter: instance is NULL"); @@ -320,7 +320,7 @@ NAN_METHOD(GetBaseClass) { } NAN_METHOD(GetTypeSize) { - GITypeInfo *gi_info = (GITypeInfo *) GNodeJS::PointerFromWrapper (info[0]); + GITypeInfo *gi_info = (GITypeInfo *) GNodeJS::BoxedFromWrapper (info[0]); auto size = GNodeJS::GetTypeSize (gi_info); info.GetReturnValue().Set(Nan::New(size)); } diff --git a/src/gi.h b/src/gi.h index ad4abb7b..16d391f9 100644 --- a/src/gi.h +++ b/src/gi.h @@ -55,7 +55,7 @@ class BaseInfo { BaseInfo (Local value) { Local object = value.As(); _info = g_base_info_ref( - (GIBaseInfo *) GNodeJS::PointerFromWrapper(object)); + (GIBaseInfo *) GNodeJS::BoxedFromWrapper(object)); }; ~BaseInfo () { diff --git a/src/gobject.cc b/src/gobject.cc index 9f6c3bec..19a20672 100644 --- a/src/gobject.cc +++ b/src/gobject.cc @@ -1,6 +1,13 @@ #include +#include "cppgc/allocation.h" +#include "cppgc/garbage-collected.h" +#include "cppgc/name-provider.h" +#include "v8-cppgc.h" +#include "v8-version.h" +#include "node_version.h" + #include "boxed.h" #include "callback.h" #include "closure.h" @@ -28,8 +35,60 @@ using Nan::WeakCallbackType; #define OFFSET_NOT_FOUND 0xffff +namespace { + +#if V8_MAJOR_VERSION >= 14 +using V8Wrappable = v8::Object::Wrappable; +#else +class V8Wrappable : public cppgc::GarbageCollected, + public cppgc::NameProvider { +public: + virtual void Trace(cppgc::Visitor* visitor) const = 0; +}; +#endif + +} + namespace GNodeJS { +static void ToggleNotify(gpointer user_data, GObject *gobject, gboolean toggle_down); + +class GObjectWrapper : public V8Wrappable { +public: + GObjectWrapper(GObject * gobject, Local object) { + Persistent *persistent = new Persistent(object); + g_object_set_qdata (gobject, GNodeJS::object_quark(), persistent); + + // Because we can't sink floating ref and add toggle ref at the same time, + // first sink the floating ref, add the toggle ref, and then release the + // ref we've just sunken. At the end, we must carry only the toggle ref. + g_object_ref_sink (gobject); + g_object_add_toggle_ref (gobject, ToggleNotify, NULL); + g_object_unref (gobject); + + this->gobject = gobject; + } + + ~GObjectWrapper() { + g_object_remove_toggle_ref (gobject, &ToggleNotify, NULL); + } + + const char * GetHumanReadableName() const override { + return g_type_name_from_instance( + reinterpret_cast(gobject)); + } + + void Trace(cppgc::Visitor* visitor) const override { + // TODO + } + + GObject * getGObject() { + return gobject; + } +private: + GObject * gobject; +}; + // Our base template for all GObjects static Nan::Persistent baseTemplate; @@ -112,20 +171,20 @@ static void ToggleNotify(gpointer user_data, GObject *gobject, gboolean toggle_d } } -static void AssociateGObject(Local object, GObject *gobject, GType gtype) { - object->SetAlignedPointerInInternalField (0, gobject); +static void AssociateGObject(Local object, GObject *gobject, GType gtype) +{ + v8::Isolate *isolate = object->GetIsolate(); + GObjectWrapper * cppWrapper = cppgc::MakeGarbageCollected( + isolate->GetCppHeap()->GetAllocationHandle(), gobject, object); - SET_OBJECT_GTYPE(object, gtype); - - auto *persistent = new Nan::Persistent(object); - g_object_set_qdata (gobject, GNodeJS::object_quark(), persistent); +#if NODE_VERSION_AT_LEAST(23,0,0) + v8::Object::Wrap( + isolate, object, cppWrapper); +#else + node::SetCppgcReference(isolate, object, cppWrapper); +#endif - // Because we can't sink floating ref and add toggle ref at the same time, - // first sink the floating ref, add the toggle ref, and then release the - // ref we've just sunken. At the end, we must carry only the toggle ref. - g_object_ref_sink (gobject); - g_object_add_toggle_ref (gobject, ToggleNotify, NULL); - g_object_unref (gobject); + SET_OBJECT_GTYPE(object, gtype); } static void GObjectConstructor(const FunctionCallbackInfo &info) { @@ -187,8 +246,6 @@ static void GObjectDestroyed(const Nan::WeakCallbackInfo &data) { /* We're destroying the wrapper object, so make sure to clear out * the qdata that points back to us. */ g_object_set_qdata (gobject, GNodeJS::object_quark(), NULL); - - g_object_remove_toggle_ref (gobject, &ToggleNotify, NULL); } static void GObjectClassDestroyed(const Nan::WeakCallbackInfo &info) { @@ -512,9 +569,8 @@ NAN_METHOD(GObjectToString) { const char* typeName = g_type_name(type); char *className = *Nan::Utf8String(self->GetConstructorName()); - void *address = self->GetAlignedPointerFromInternalField(0); - char *str = g_strdup_printf("[%s:%s %#zx]", typeName, className, (size_t)address); + char *str = g_strdup_printf("[%s:%s %#zx]", typeName, className, (size_t)g_object); info.GetReturnValue().Set(UTF8(str)); g_free(str); @@ -548,7 +604,17 @@ static MaybeLocal NewClassTemplate (GType gtype) { auto tpl = New (GObjectConstructor, New((void *) gtype)); tpl->SetClassName (UTF8(class_name)); - tpl->InstanceTemplate()->SetInternalFieldCount(1); + +// XXX: do we need to do anything with v8::Object::Wrap() ? +#if !NODE_VERSION_AT_LEAST(23,0,0) + v8::Isolate* isolate = v8::Isolate::GetCurrent(); + v8::WrapperDescriptor descriptor = + isolate->GetCppHeap()->wrapper_descriptor(); + uint16_t required_size = std::max(descriptor.wrappable_instance_index, + descriptor.wrappable_type_index); + tpl->InstanceTemplate()->SetInternalFieldCount(required_size + 1); +#endif + Nan::SetPrototypeTemplate( tpl, "__gtype__", v8::BigInt::NewFromUnsigned(v8::Isolate::GetCurrent(), gtype)); @@ -677,9 +743,19 @@ GObject * GObjectFromWrapper(Local value) { Local object = TO_OBJECT (value); - void *ptr = object->GetAlignedPointerFromInternalField (0); - GObject *gobject = G_OBJECT (ptr); - return gobject; +#if NODE_VERSION_AT_LEAST(23,0,0) + GObjectWrapper * cppWrapper = static_cast( + v8::Object::Unwrap( + object->GetIsolate(), object)); +#else + v8::WrapperDescriptor descriptor = + object->GetIsolate()->GetCppHeap()->wrapper_descriptor(); + GObjectWrapper * cppWrapper = static_cast( + object->GetAlignedPointerFromInternalField( + descriptor.wrappable_instance_index)); +#endif + + return cppWrapper->getGObject(); } MaybeLocal GetGObjectProperty(GObject * gobject, const char *prop_name) { diff --git a/src/value.cc b/src/value.cc index c2925caa..977eec3d 100644 --- a/src/value.cc +++ b/src/value.cc @@ -691,7 +691,7 @@ bool V8ToGIArgumentInterface(GIBaseInfo *gi_info, GIArgument *arg, Local case GI_INFO_TYPE_BOXED: case GI_INFO_TYPE_STRUCT: case GI_INFO_TYPE_UNION: - arg->v_pointer = PointerFromWrapper(value); + arg->v_pointer = BoxedFromWrapper(value); break; case GI_INFO_TYPE_FLAGS: @@ -740,7 +740,7 @@ bool V8ToGIArgument(GITypeInfo *type_info, GIArgument *arg, Local value, switch (type_tag) { case GI_TYPE_TAG_VOID: if (g_type_info_is_pointer(type_info)) { - arg->v_pointer = PointerFromWrapper(value); + arg->v_pointer = BoxedFromWrapper(value); break; } arg->v_pointer = NULL; @@ -848,7 +848,7 @@ bool V8ToGIArgument(GITypeInfo *type_info, GIArgument *arg, Local value, case GI_TYPE_TAG_ERROR: { - arg->v_pointer = (GError *) PointerFromWrapper(value); + arg->v_pointer = (GError *) BoxedFromWrapper(value); } break; @@ -1488,9 +1488,9 @@ bool V8ToGValue(GValue *gvalue, Local value, ResourceOwnership ownership) return false; } if (ownership == kCopy) - g_value_set_boxed (gvalue, PointerFromWrapper(value)); + g_value_set_boxed (gvalue, BoxedFromWrapper(value)); else - g_value_set_static_boxed (gvalue, PointerFromWrapper(value)); + g_value_set_static_boxed (gvalue, BoxedFromWrapper(value)); } else if (G_VALUE_HOLDS_PARAM (gvalue)) { if (!ValueIsInstanceOfGType(value, G_VALUE_TYPE (gvalue))) { Throw::CannotConvertGType("GParamSpec", G_VALUE_TYPE (gvalue)); @@ -1581,9 +1581,13 @@ bool ValueHasInternalField(Local value) { Local object = TO_OBJECT (value); +// FIXME: find something that works for v8::Object::Wrap() (short of actually +// unwrapping it). +#if !NODE_VERSION_AT_LEAST(23,0,0) // Wait, this is not a GObject! if (object->InternalFieldCount() == 0) return false; +#endif return true; } diff --git a/tests/module__system.js b/tests/module__system.js index 3bfc7b2f..38a94fb8 100644 --- a/tests/module__system.js +++ b/tests/module__system.js @@ -6,6 +6,7 @@ const gi = require('../lib/') const Gtk = gi.require('Gtk', '3.0') const common = require('./__common__.js') +const process = require('node:process') const system = gi.System Gtk.init() @@ -22,9 +23,13 @@ common.describe('system', () => { }) common.it('.internalFieldCount()', () => { + const NODE_MAJOR_VERSION = parseInt(process.versions.node.split('.')[0]); + const expectedCount = NODE_MAJOR_VERSION < 23 ? 2 : 0; + const btn = new Gtk.Button() const result = system.internalFieldCount(btn) - common.assert(result === 1, 'internalFieldCount() result isnt valid: ' + result) + common.assert(result === expectedCount, + 'internalFieldCount() result isnt valid: ' + result) }) common.it('.addressOf()', () => { From ec4c879cebcbde62da55daa29b879958833523ee Mon Sep 17 00:00:00 2001 From: Ratchanan Srirattanamet Date: Tue, 22 Jul 2025 00:48:29 +0700 Subject: [PATCH 2/2] feat(gobject, closure): record created Closure to be traced by cppgc Replace a Persistent in Closure with a TracedReference. Then, keep track of created Closure in GObjectWrapper. When GObjectWrapper is traced by cppgc, trace those reference to tell cppgc they're still alive. By not creating a Persistent, we prevent a reference loop where the Persistent holds function, holds JS wrapper, holds GObject, holds Persistent. When TracedReference, evetually the whole loop will not be traced by cppgc, allowing the whole loop to be dropped. This approach is inspired by PyGObject. --- src/closure.cc | 27 ++++++++++++++++++++------ src/closure.h | 8 +++++--- src/gobject.cc | 51 ++++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 73 insertions(+), 13 deletions(-) diff --git a/src/closure.cc b/src/closure.cc index 5412b5cb..c39e2887 100644 --- a/src/closure.cc +++ b/src/closure.cc @@ -15,13 +15,13 @@ using v8::Isolate; using v8::Local; using v8::Object; using v8::Value; -using Nan::Persistent; namespace GNodeJS { GClosure *Closure::New (Local function, GICallableInfo* info, guint signalId) { Closure *closure = (Closure *) g_closure_new_simple (sizeof (*closure), GUINT_TO_POINTER(signalId)); - closure->persistent.Reset(function); + closure->isolate = function->GetIsolate(); + closure->functionRef.Reset(closure->isolate, function); if (info) { closure->info = g_base_info_ref(info); } else { @@ -34,11 +34,12 @@ GClosure *Closure::New (Local function, GICallableInfo* info, guint si } void Closure::Execute(GICallableInfo *info, guint signal_id, - const Nan::Persistent &persFn, + v8::Isolate *isolate, + const v8::TracedReference &fnRef, GValue *g_return_value, guint n_param_values, const GValue *param_values) { Nan::HandleScope scope; - auto func = Nan::New(persFn); + auto func = fnRef.Get(isolate); GSignalQuery signal_query = { 0, }; @@ -139,11 +140,25 @@ void Closure::Marshal(GClosure *base, if (env->IsSameThread()) { /* Case 1: same thread */ - Closure::Execute(closure->info, signal_id, closure->persistent, g_return_value, n_param_values, param_values); + Closure::Execute( + closure->info, + signal_id, + closure->isolate, + closure->functionRef, + g_return_value, + n_param_values, + param_values); } else { /* Case 2: different thread */ env->Call([&]() { - Closure::Execute(closure->info, signal_id, closure->persistent, g_return_value, n_param_values, param_values); + Closure::Execute( + closure->info, + signal_id, + closure->isolate, + closure->functionRef, + g_return_value, + n_param_values, + param_values); }); } } diff --git a/src/closure.h b/src/closure.h index 792f8c9d..7c2a050f 100644 --- a/src/closure.h +++ b/src/closure.h @@ -16,11 +16,12 @@ namespace GNodeJS { struct Closure { GClosure base; - Nan::Persistent persistent; + v8::Isolate *isolate; + v8::TracedReference functionRef; GICallableInfo* info; ~Closure() { - persistent.Reset(); + functionRef.Reset(); if (info) g_base_info_unref (info); } @@ -29,7 +30,8 @@ struct Closure { guint signalId); static void Execute(GICallableInfo *info, guint signal_id, - const Nan::Persistent &persFn, + v8::Isolate *isolate, + const v8::TracedReference &persFn, GValue *g_return_value, guint n_param_values, const GValue *param_values); diff --git a/src/gobject.cc b/src/gobject.cc index 19a20672..61dcdaf8 100644 --- a/src/gobject.cc +++ b/src/gobject.cc @@ -1,5 +1,7 @@ #include +#include +#include #include "cppgc/allocation.h" #include "cppgc/garbage-collected.h" @@ -70,6 +72,11 @@ class GObjectWrapper : public V8Wrappable { } ~GObjectWrapper() { + for (auto closure : watchedClosures) { + g_closure_remove_invalidate_notifier( + closure, this, &closureInvalidated); + } + g_object_remove_toggle_ref (gobject, &ToggleNotify, NULL); } @@ -79,14 +86,40 @@ class GObjectWrapper : public V8Wrappable { } void Trace(cppgc::Visitor* visitor) const override { - // TODO + std::lock_guard l(mutex); + + for (auto closure : watchedClosures) { + visitor->Trace(reinterpret_cast(closure)->functionRef); + } } GObject * getGObject() { return gobject; } + + void watchClosure(GClosure * closure) { + std::lock_guard l(mutex); + + watchedClosures.push_back(closure); + g_closure_add_invalidate_notifier(closure, this, &closureInvalidated); + } + + static void closureInvalidated(gpointer data, GClosure * closure) { + GObjectWrapper * self = static_cast(data); + std::lock_guard l(self->mutex); + + // https://stackoverflow.com/a/3385251 + self->watchedClosures.erase(std::remove( + self->watchedClosures.begin(), + self->watchedClosures.end(), + closure + ), self->watchedClosures.end()); + } private: GObject * gobject; + + mutable std::mutex mutex; + std::vector watchedClosures; }; // Our base template for all GObjects @@ -399,10 +432,15 @@ static void DestroyVFuncs(GType gtype) { g_type_set_qdata (gtype, GNodeJS::vfuncs_quark(), NULL); } +// FIXME: reorganize so that we don't have this out-of-place declaration. +// We probably want to declare GObjectWrapper class in gobject.h now. +GObjectWrapper * GObjectCppWrapperFromWrapper(Local value); + NAN_METHOD(SignalConnect) { bool after = false; - GObject *gobject = GObjectFromWrapper (info.This ()); + GObjectWrapper *cppWrapper = GObjectCppWrapperFromWrapper(info.This ()); + GObject *gobject = cppWrapper->getGObject(); if (!gobject) { Nan::ThrowTypeError("Object is not a GObject"); @@ -448,6 +486,7 @@ NAN_METHOD(SignalConnect) { } gclosure = Closure::New (callback, signal_info, signalId); + cppWrapper->watchClosure(gclosure); handler_id = g_signal_connect_closure (gobject, signalName, gclosure, after); info.GetReturnValue().Set((double)handler_id); @@ -737,7 +776,7 @@ Local WrapperFromGObject(GObject *gobject) { return obj; } -GObject * GObjectFromWrapper(Local value) { +GObjectWrapper * GObjectCppWrapperFromWrapper(Local value) { if (!ValueHasInternalField(value)) return nullptr; @@ -755,7 +794,11 @@ GObject * GObjectFromWrapper(Local value) { descriptor.wrappable_instance_index)); #endif - return cppWrapper->getGObject(); + return cppWrapper; +} + +GObject * GObjectFromWrapper(Local value) { + return GObjectCppWrapperFromWrapper(value)->getGObject(); } MaybeLocal GetGObjectProperty(GObject * gobject, const char *prop_name) {