Skip to content

Fix FinalizationRegistry refcounting bug#656

Merged
bnoordhuis merged 2 commits intoquickjs-ng:masterfrom
bnoordhuis:fix648-for-realz
Nov 7, 2024
Merged

Fix FinalizationRegistry refcounting bug#656
bnoordhuis merged 2 commits intoquickjs-ng:masterfrom
bnoordhuis:fix648-for-realz

Conversation

@bnoordhuis
Copy link
Copy Markdown
Contributor

Introduced in commit 61c8fe6 from last month that moved the callback into the job queue:

  1. It leaked fre->held_val when no job was enqueued

  2. It fumbled the reference count when enqueuing; JS_EnqueueJob already takes care of incrementing and decrementing it

Reverts commit 0a70623 from earlier today because that didn't turn out to be a complete fix.

Fixes: #648

Introduced in commit 61c8fe6 from last month that moved the callback
into the job queue:

1. It leaked `fre->held_val` when no job was enqueued

2. It fumbled the reference count when enqueuing; JS_EnqueueJob already
   takes care of incrementing and decrementing it

Reverts commit 0a70623 from earlier today because that didn't turn out
to be a complete fix.

Fixes: quickjs-ng#648
@bnoordhuis
Copy link
Copy Markdown
Contributor Author

bnoordhuis commented Nov 6, 2024

Grr, tests/test_builtin.js still asserts, even with this change. It definitely fixes a bug but it clearly doesn't fix all bugs.

No wait, it did fix it! I didn't apply it right to another branch.

@saghul
Copy link
Copy Markdown
Contributor

saghul commented Nov 7, 2024

Thanks Ben!

@bnoordhuis bnoordhuis merged commit 9c5c441 into quickjs-ng:master Nov 7, 2024
@bnoordhuis bnoordhuis deleted the fix648-for-realz branch November 7, 2024 08:12
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 this pull request may close these issues.

FinalizationRegistry resource leak?

2 participants