Fork 1

Memory corruption after calling methods that return SList with transfer-ownership="none" #183

New issue

Closed

opened 2025-01-09 21:25:03 +01:00 by BwackNinja · 5 comments

BwackNinja commented

2025-01-09 21:25:03 +01:00

(Migrated from github.com)

I've been trying to figure out what's causing memory corruption when I'm rendering text, and I think I've found it.

The finalizer registered to the Cleaner in the SList constructor universally calls SListNode.free(new SListNode(address)); even when java-gi doesn't own the memory and shouldn't be freeing it. Examples include Layout.getLinesReadonly() in Pango.

Retaining a reference to the SList to prevent the finalizer from running does prevent the memory corruption.

I've been trying to figure out what's causing memory corruption when I'm rendering text, and I think I've found it. The finalizer registered to the `Cleaner` in the `SList` constructor universally calls `SListNode.free(new SListNode(address));` even when java-gi doesn't own the memory and shouldn't be freeing it. Examples include `Layout.getLinesReadonly()` in `Pango`. Retaining a reference to the `SList` to prevent the finalizer from running does prevent the memory corruption.

jwharm commented

2025-01-09 21:56:46 +01:00

(Migrated from github.com)

Thanks for the bug report. These issues a really annoying to debug. This helps a lot.

BwackNinja commented

2025-01-09 22:09:20 +01:00

(Migrated from github.com)

Jemalloc is really nice for finding memory leaks, but finding when and what has been preemptively been free'd is tough.

This is related, so I'm not sure if this needs to be a separate report, but there are other issues also involving returns with transfer-ownership="none". Buffer.peekMemory() and Buffer.getMeta() in GStreamer run MemoryCleaner.takeOwnership(_instance); despite also being marked transfer-ownership="none".

Jemalloc is really nice for finding memory leaks, but finding when and what has been preemptively been free'd is tough. This is related, so I'm not sure if this needs to be a separate report, but there are other issues also involving returns with transfer-ownership="none". `Buffer.peekMemory()` and `Buffer.getMeta()` in GStreamer run `MemoryCleaner.takeOwnership(_instance);` despite also being marked transfer-ownership="none".

jwharm commented

2025-01-11 11:55:51 +01:00

(Migrated from github.com)

Buffer.peekMemory() calls g_boxed_copy to create its own copy or add a reference, and then takes ownership of the copy. The MemoryCleaner will call g_boxed_free to free the copy (or decrease the reference). This ensures your Java variable will not be freed suddenly, while you were still using it.

Similarly, Buffer.getMeta() creates a copy of the memory of the returned Meta struct. That memory is freed by the MemoryCleaner afterwards. (This needs more work, because the copy is "shallow": Meta contains a MetaInfo child that isn't copied.)

`Buffer.peekMemory()` calls `g_boxed_copy` to create its own copy or add a reference, and then takes ownership of the copy. The MemoryCleaner will call `g_boxed_free` to free the copy (or decrease the reference). This ensures your Java variable will not be freed suddenly, while you were still using it. Similarly, `Buffer.getMeta()` creates a copy of the memory of the returned `Meta` struct. That memory is freed by the MemoryCleaner afterwards. (This needs more work, because the copy is "shallow": `Meta` contains a `MetaInfo` child that isn't copied.)

BwackNinja commented

2025-01-11 16:30:05 +01:00

(Migrated from github.com)

Ah. Thanks for explaining that.

I'm still seeing some other memory issues, at least a failure to free GdkTextures, but I'm trying to isolate that before I can confidently make a bug report. Thanks for the improvements! I'm almost back where I was with version 0.9.x without needing the explicit unref() calls.

Ah. Thanks for explaining that. I'm still seeing some other memory issues, at least a failure to free `GdkTexture`s, but I'm trying to isolate that before I can confidently make a bug report. Thanks for the improvements! I'm almost back where I was with version 0.9.x without needing the explicit `unref()` calls.

jwharm commented

2025-01-11 21:19:49 +01:00

(Migrated from github.com)

Awesome! May I ask what you are developing?