From 9c5c441744bfb123f6ef555c8326a4bdf25cd5d9 Mon Sep 17 00:00:00 2001
From: Ben Noordhuis <info@bnoordhuis.nl>
Date: Thu, 7 Nov 2024 09:12:34 +0100
Subject: [PATCH] Fix FinalizationRegistry refcounting bug (#656)

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: https://github.com/quickjs-ng/quickjs/issues/648
---
 quickjs.c       | 10 ++++------
 tests/bug648.js | 13 +++++++++++++
 2 files changed, 17 insertions(+), 6 deletions(-)
 create mode 100644 tests/bug648.js

diff --git a/quickjs.c b/quickjs.c
index a104aaf..a886ee2 100644
--- a/quickjs.c
+++ b/quickjs.c
@@ -2107,6 +2107,8 @@ void JS_FreeRuntime(JSRuntime *rt)
     }
 #endif
 
+    assert(list_empty(&rt->gc_obj_list));
+
     /* free the classes */
     for(i = 0; i < rt->class_count; i++) {
         JSClass *cl = &rt->class_array[i];
@@ -2239,9 +2241,6 @@ void JS_FreeRuntime(JSRuntime *rt)
     }
 #endif
 
-    // FinalizationRegistry finalizers have run, no objects should remain
-    assert(list_empty(&rt->gc_obj_list));
-
     {
         JSMallocState *ms = &rt->malloc_state;
         rt->mf.js_free(ms->opaque, rt);
@@ -54985,9 +54984,7 @@ static const JSClassShortDef js_finrec_class_def[] = {
 
 static JSValue js_finrec_job(JSContext *ctx, int argc, JSValue *argv)
 {
-    JSValue ret = JS_Call(ctx, argv[0], JS_UNDEFINED, 1, &argv[1]);
-    JS_FreeValue(ctx, argv[1]);
-    return ret;
+    return JS_Call(ctx, argv[0], JS_UNDEFINED, 1, &argv[1]);
 }
 
 void JS_AddIntrinsicWeakRef(JSContext *ctx)
@@ -55078,6 +55075,7 @@ static void reset_weak_ref(JSRuntime *rt, JSWeakRefRecord **first_weak_ref)
                 args[1] = fre->held_val;
                 JS_EnqueueJob(frd->ctx, js_finrec_job, 2, args);
             }
+            JS_FreeValueRT(rt, fre->held_val);
             JS_FreeValueRT(rt, fre->token);
             js_free_rt(rt, fre);
             break;
diff --git a/tests/bug648.js b/tests/bug648.js
new file mode 100644
index 0000000..cb0a7d3
--- /dev/null
+++ b/tests/bug648.js
@@ -0,0 +1,13 @@
+/*---
+negative:
+  phase: runtime
+  type: Error
+---*/
+let finrec = new FinalizationRegistry(v => {})
+let object = {object:"object"}
+finrec.register(object, {held:"held"}, {token:"token"})
+object = undefined
+// abrupt termination should not leak |held|
+// unfortunately only shows up in qjs, not run-test262,
+// but still good to have a regression test
+throw Error("ok")