Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix crash in FinalizationRegistry when the observed object is GC'd #371

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

saghul
Copy link
Contributor

@saghul saghul commented 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

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 saghul requested a review from bnoordhuis April 11, 2024 20:44
Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I think, but a question: doesn't held_value turn into JS_UNDEFINED when it's collected?

JS_IsLiveObject is functionally equivalent to JS_IsUndefined if that's the case, I'm just wondering if that's indeed how it works.

@saghul
Copy link
Contributor Author

saghul commented Apr 12, 2024

LGTM, I think, but a question: doesn't held_value turn into JS_UNDEFINED when it's collected?

Not quite. It's still an object, but it has no shape and in the next phase it will be freed.

@saghul saghul merged commit 38fa7d7 into master Apr 12, 2024
46 checks passed
@saghul saghul deleted the fix-finrec branch April 12, 2024 10:23
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"FinalizationRegistry" bug
2 participants