FinalizationRegistry resource leak?

[bnoordhuis]

Opening so I don't forget.

There seems to be a leak caused by FinalizationRegistry. Apply this diff:

diff --git a/tests/test_builtin.js b/tests/test_builtin.js
index 4c29503..57c77ab 100644
--- a/tests/test_builtin.js
+++ b/tests/test_builtin.js
@@ -1047,6 +1047,7 @@ test_generator();
 test_proxy_iter();
 test_proxy_is_array();
 test_finalization_registry();
+throw "boom";
 test_exception_source_pos();
 test_function_source_pos();
 test_exception_prepare_stack();

And then:

$ build/qjs tests/test_builtin.js
boom
qjs: /home/bnoordhuis/src/quickjs/quickjs.c:2109: JS_FreeRuntime: Assertion `list_empty(&rt->gc_obj_list)' failed.
Aborted (core dumped)

The assert does not trigger when you throw before the call to test_finalization_registry.

[bnoordhuis]

More minimal example:

let finrec = new FinalizationRegistry(v => {})
finrec.register({}, {}) // primitive value works okay
throw "boom"

[bnoordhuis]

I still trigger that assert from time to time... reopening.

[bnoordhuis]

Note how this now works:

let finrec = new FinalizationRegistry(v => {})
let object = {}
finrec.register(object, {})
throw "boom"

But this still asserts:

let finrec = new FinalizationRegistry(v => {})
finrec.register({}, {})
throw "boom"

As does this:

let finrec = new FinalizationRegistry(v => {})
let object = {}
finrec.register(object, {})
object = undefined
throw "boom"

js_finrec_finalizer only runs runs in the first case.

[bnoordhuis]

Oh, and: setting finrec = undefined also avoids the assert. - no wait, still asserts

[saghul]

If you enable leak dumping, what do you see? (Not at the computer now)

[bnoordhuis]

Right, I didn't mention that but it's the heldValue (second argument) that leaks.

[bnoordhuis]

I tracked it down to a regression beginning with commit 61c8fe6.