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

@bnoordhuis bnoordhuis commented Nov 6, 2024

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
@bnoordhuis
Fix FinalizationRegistry refcounting bug
53727e0 
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
squash! add ineffective regression test
e2db483
@bnoordhuis
Copy link
Copy Markdown
Contributor Author

bnoordhuis commented Nov 6, 2024 •
edited
Loading

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

2 participants

@bnoordhuis @saghul