Open peat-psuwit opened 3 years ago
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?
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.
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.
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.
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.
Well, in this case it's a boxed object (I have to put it the title...)
I'm not sure if this is the best way to reproduce/detect it, but please bear with me.
libgstreamer1.0-0-dbg
).valgrind --vgdb=yes node --expose-gc
.Gst.init(null);
target remote | vgdb
break gst_buffer_new command finish end continue
While running this, GDB should break 2 times, with a line for each value returned. Take note of these addresses.
global.gc()
finishes, press Ctrl+C on terminal 2 to bring up the prompt. Then, run: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.Address <address 2> is 0 bytes inside a block of size 280 alloc'd
. This means the address is not freed.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 aGstMiniObject
, which in itself is a boxed type. I've looked intoBoxedConstructor()
insideboxed.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.