node-gtk icon indicating copy to clipboard operation
node-gtk copied to clipboard

feat(gobject, closure): wrap GObject with GC-able C++ wrapper to fix closures reference loop

Open peat-psuwit opened this issue 5 months ago • 3 comments

Instead of storing pointer to GObject directly in JS Object, store a C++ GC-able wrapper class.

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 and utilize V8's Oilpan (also called cppgc), C++ garbage collector integrated with V8. This, however, means cutting support for NodeJS older than version 20.6. That said, NodeJS 18 has just became end-of-life (not counting commercial support), so maybe it's not that bad, compared to not leaking memory?


TODO:

  • [ ] Test with Node 23+, where mechanics of wrapping GC-able C++ inside V8 Object changes. Node 23+ itself requires other changes before Node-GTK compiles. See #374.
  • [ ] Re-organize the code to move out-of-place function declaration in gobject.cc (see FIXME in the code). Also consider moving GObjectWrapper class declaration to gobject.h.
  • [ ] Maybe also track & trace callbacks and vfuncs?

peat-psuwit avatar Jul 21 '25 18:07 peat-psuwit

Looks like #374 could be merged soon.

Test with Node 23+, where mechanics of wrapping GC-able C++ inside V8 Object changes

I haven't followed V8 development lately, do you have a link to the changelog/docs for that? Do you expect any big issue with compat between <23 and 23+?

romgrk avatar Jul 27 '25 20:07 romgrk

I haven't followed V8 development lately, do you have a link to the changelog/docs for that? Do you expect any big issue with compat between <23 and 23+?

I can't find doc, but the relevant commit is: https://github.com/nodejs/node/commit/0be79f4debef90737eb0e90e65e8e80381af04c7

NodeJS actually gives an API for wrapping part (and that part technically have compatibility layer). However I can't find an equivalent API for unwrapping from NodeJS and has to directly use V8 APIs.

I've already written code for that and put #if NODE_VERSION_AT_LEAST(23,0,0) around affected code (2 of them). It's just that I haven't tested if my code actually functions.

peat-psuwit avatar Jul 28 '25 08:07 peat-psuwit

#374 was merged and I'll release it as v1.0.0 soon. Afaiu, this is all internal and wouldn't break the external API so it can be released as a minor/patch once it's ready.

romgrk avatar Jul 28 '25 21:07 romgrk