quickjs-ng / quickjs

QuickJS, the Next Generation: a mighty JavaScript engine
MIT License
691 stars 66 forks source link

"FinalizationRegistry" bug #367

Closed boiledPeanuts123 closed 2 months ago

boiledPeanuts123 commented 2 months ago
import * as os from "os";
import * as std from "std";

globalThis.obj = { data: 'example' };

globalThis.finalizationRegistry = new FinalizationRegistry((heldValue) => {

  console.log('Object has been garbage collected:', heldValue);
});

finalizationRegistry.register(obj, 'some value');

Running the above code will crash。

saghul commented 2 months ago

Can you paste the backtrace? I suspect it as to do with the object being global so it will be collected in the final stage when the context is being destroyed...

boiledPeanuts123 commented 2 months ago

Can you paste the backtrace? I suspect it as to do with the object being global so it will be collected in the final stage when the context is being destroyed...

ok!

backtrace:

1 find_own_property quickjs.c 5167 0x55e10f6ef04f 2 JS_GetGlobalVar quickjs.c 9627 0x55e10f6ef04f 3 JS_CallInternal quickjs.c 15229 0x55e10f7008bf 4 JS_Call quickjs.c 17073 0x55e10f709907 5 reset_weak_ref quickjs.c 52145 0x55e10f77357c 6 free_object quickjs.c 5401 0x55e10f6e08c1 7 free_gc_object quickjs.c 5426 0x55e10f6e09e9 8 gc_free_cycles quickjs.c 5759 0x55e10f6e15ef 9 JS_RunGC quickjs.c 5789 0x55e10f6e1713 10 JS_FreeRuntime quickjs.c 1926 0x55e10f6d72ae 11 main qjs.c 484 0x55e10f6c729b

saghul commented 2 months ago

Alright, I dug a bit and I see the problem:

The reference cycle is not broken and thus the objects live after JS_FreeContext has been called. Arguably that is wrong. Then, when JS_FreeRuntime runs a GC pass is made and when freeing the objects we have pointers to invalid memory, as the context was freed.

I tried to run GC towards the end of the JS_FreeContext phase, to no avail, gotta think about it some more.

Thanks for the report!

saghul commented 2 months ago

Hum, not quite. Thanks to some "autoinit" logic the call to JS_FreeContext in qjs.c doesn't do anything other than decrement the refcount. It's actually destroyed as part of JS_RunGC inside JS_FreeRuntime.

I suppose that my original assesment still stands though: eventually the context is going to be freed and when it's the turn to free the global object we cannot call the function because the context is gone.

saghul commented 2 months ago

Scratch that. The problem was that the held object was freed before the finrec, because it was part of a cycle. Fixed in https://github.com/quickjs-ng/quickjs/pull/371

boiledPeanuts123 commented 2 months ago

Scratch that. The problem was that the held object was freed before the finrec, because it was part of a cycle. Fixed in #371

There's another GC issue, also present in the mentioned link below:https://github.com/bellard/quickjs/issues/218#issue-2080600884, which is not fixed in the quickjs-ng version

saghul commented 2 months ago

I'll take a look at porting the patch, thanks!