node-gtk leaks boxed object created through <Type>.new*() (as opposed to new <Type>())

[peat-psuwit]

I'm not sure if this is the best way to reproduce/detect it, but please bear with me.

1. Install gdb, valgrind and gstreamer's debugging symbol (for Ubuntu 20.04, it's libgstreamer1.0-0-dbg).
2. Prepare 2 terminal windows. The first one is for node and the second one is for gdb.
3. On terminal 1, launch node like this: valgrind --vgdb=yes node --expose-gc.
4. Initialize node-gtk and Gstreamer, as usual. Valgrind may print something; it's unrelated to this issue.

const gi = require('node-gtk')
const Gst = gi.require('Gst', '1.0')

Gst.init(null);

5. On terminal 2, run gdb. Then, connect to valgrind using:

target remote | vgdb

6. Set the breakpoint on gst_buffer_new() this way, so that the return value gets print automatically:

break gst_buffer_new
command
  finish
end
continue

Unfortunately, we cannot set it to also automatically continue after finishing. So we have to do it manually.
7. On terminal 1, run the following code:

let b;
b = new Gst.Buffer(); // Won't leak.
b = Gst.Buffer.new(); // Will leak.
b = null;
b; // Apparently clears JIT cache
global.gc(); // Make sure that things that should be freed will be freed

While running this, GDB should break 2 times, with a line for each value returned. Take note of these addresses.
8. After global.gc() finishes, press Ctrl+C on terminal 2 to bring up the prompt. Then, run:

monitor v.info location <address 1>

There will be a long output, but the most important part is the first line. It should be Address <address 1> is 0 bytes inside a block of size 280 free'd, which indicate that this address is freed.
9. Do the same with address 2 (except Ctrl+C). Now, the first line will change to Address <address 2> is 0 bytes inside a block of size 280 alloc'd. This means the address is not freed.
10. Just in case something else is actually holding to the second GstBuffer, run:

monitor who_points_at <address 2>

The only output should be Searching for pointers to <address 2>. This means nothing can refer to it now; this GstBuffer is now leaked.

GstBuffer is a GstMiniObject, which in itself is a boxed type. I've looked into BoxedConstructor() inside boxed.cc; it seems to only either make a copy to itself or assume that the memory is managed elsewhere. The condition (mustCopy) differs in different code paths that lead to it, which I'm unable to follow. So, I'm not sure where to start.

[romgrk]

Got it. I've been swamped by work at my new job so I'm unfortunately out of time to look into this more in details or to debug this. Did you progress any further here? Anything I can do to help that isn't too much involved?

[peat-psuwit]

I've made some initial implementation in my repo:
https://github.com/peat-psuwit/node-gtk/commits/transfer-fully-in_-_anti-leak

I've made WrapperFromBoxed accept a new enum MemoryOwnership, whose choices are COPY (equals to mustCopy = true), LEASED (equals to mustCopy = false), and TRANSFERED. Then, through GIArgumentToV8() and GValueToV8(), I make sure that this value is selected appropriately throughout the codebase, which happens to change some copying behavior too. In particular, the return values are now copied, which has an implication that I have to put an exception for Typelib as g_irepository_require() is called from JS.

Interesting note is that it seems like Gjs always make a copy of the return value and then free the interim copy afterwards. I guess it works, but seems a bit wasteful.

[romgrk]

Any updates since your last comment? If we can avoid copies on return it would be better. I have a bit more time to take a look these days.

[peat-psuwit]

No code update from the last time.

For the return value's copy, I forgot to mention that it's copied only when the return value's transfer is not TRANSFER_FULL (previously it was never copied). This has to be done because we have to make sure that our JS wrapper stays valid beyond the lifetime of the received value. Otherwise, this case will have a problem:

let p: Gst.Promise = an_operation();
// https://gstreamer.freedesktop.org/documentation/gstreamer/gstpromise.html?gi-language=c#gst_promise_get_reply
// Returns ( [transfer: none][nullable]) – The reply set on promise
let s: Gst.Structure = p.getReply();
p = null; global.gc(); // s is gone with the GstPromise.
s.isEqual(s); // Use-after-free!

Unless we can find a way to detect this kind of thing and neuter the s wrapper, which seems less likely given how boxed object works.

Also, maybe I should rename the type to BoxedMemOwnership (or something), as this value really affects the boxed objects only. Alternatively, maybe the GObject support part should take MemoryOwnership into account. I'm still not sure what's the best approach for this.

[romgrk]

GObjects memory management is supposed to be fully handled by refcounting plus V8 GC's weakref callbacks (aka deallocation callbacks), so it shouldn't require it.

[peat-psuwit]

Well, in this case it's a boxed object (I have to put it the title...)