Skip to content

"FinalizationRegistry" bug #367

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
boiledPeanuts123 opened this issue Apr 11, 2024 · 7 comments · Fixed by #371
Closed

"FinalizationRegistry" bug #367

boiledPeanuts123 opened this issue Apr 11, 2024 · 7 comments · Fixed by #371

Comments

@boiledPeanuts123
Copy link

boiledPeanuts123 commented Apr 11, 2024

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
Copy link
Contributor

saghul commented Apr 11, 2024

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
Copy link
Author

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
Copy link
Contributor

saghul commented Apr 11, 2024

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
Copy link
Contributor

saghul commented Apr 11, 2024

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 added a commit that referenced this issue Apr 11, 2024
In the pathological case shown in
#367 both the object and the
registry will be destroyed as part of the GC phase of JS_FreeRuntime.
When the GC sweep happens it's possible we are holding on to a corpse so
avoid calling the registry callback in that case.

This is similar to how Weak{Map,Set} deal with iterators being freed as
part of a cycle.

Fixes: #367
@saghul
Copy link
Contributor

saghul commented Apr 11, 2024

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

@boiledPeanuts123
Copy link
Author

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:bellard/quickjs#218 (comment), which is not fixed in the quickjs-ng version

@saghul
Copy link
Contributor

saghul commented Apr 11, 2024

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

saghul added a commit that referenced this issue Apr 12, 2024
In the pathological case shown in
#367 both the object and the
registry will be destroyed as part of the GC phase of JS_FreeRuntime.
When the GC sweep happens it's possible we are holding on to a corpse so
avoid calling the registry callback in that case.

This is similar to how Weak{Map,Set} deal with iterators being freed as
part of a cycle.

Fixes: #367
bluesky950520 pushed a commit to bluesky950520/quickjs that referenced this issue Mar 14, 2025
In the pathological case shown in
quickjs-ng/quickjs#367 both the object and the
registry will be destroyed as part of the GC phase of JS_FreeRuntime.
When the GC sweep happens it's possible we are holding on to a corpse so
avoid calling the registry callback in that case.

This is similar to how Weak{Map,Set} deal with iterators being freed as
part of a cycle.

Fixes: quickjs-ng/quickjs#367
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants