Skip to content

Commit 9c5c441

Browse files
authored
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: #648
1 parent aedd829 commit 9c5c441

File tree

2 files changed

+17
-6
lines changed

2 files changed

+17
-6
lines changed

quickjs.c

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2107,6 +2107,8 @@ void JS_FreeRuntime(JSRuntime *rt)
21072107
}
21082108
#endif
21092109

2110+
assert(list_empty(&rt->gc_obj_list));
2111+
21102112
/* free the classes */
21112113
for(i = 0; i < rt->class_count; i++) {
21122114
JSClass *cl = &rt->class_array[i];
@@ -2239,9 +2241,6 @@ void JS_FreeRuntime(JSRuntime *rt)
22392241
}
22402242
#endif
22412243

2242-
// FinalizationRegistry finalizers have run, no objects should remain
2243-
assert(list_empty(&rt->gc_obj_list));
2244-
22452244
{
22462245
JSMallocState *ms = &rt->malloc_state;
22472246
rt->mf.js_free(ms->opaque, rt);
@@ -54985,9 +54984,7 @@ static const JSClassShortDef js_finrec_class_def[] = {
5498554984

5498654985
static JSValue js_finrec_job(JSContext *ctx, int argc, JSValue *argv)
5498754986
{
54988-
JSValue ret = JS_Call(ctx, argv[0], JS_UNDEFINED, 1, &argv[1]);
54989-
JS_FreeValue(ctx, argv[1]);
54990-
return ret;
54987+
return JS_Call(ctx, argv[0], JS_UNDEFINED, 1, &argv[1]);
5499154988
}
5499254989

5499354990
void JS_AddIntrinsicWeakRef(JSContext *ctx)
@@ -55078,6 +55075,7 @@ static void reset_weak_ref(JSRuntime *rt, JSWeakRefRecord **first_weak_ref)
5507855075
args[1] = fre->held_val;
5507955076
JS_EnqueueJob(frd->ctx, js_finrec_job, 2, args);
5508055077
}
55078+
JS_FreeValueRT(rt, fre->held_val);
5508155079
JS_FreeValueRT(rt, fre->token);
5508255080
js_free_rt(rt, fre);
5508355081
break;

tests/bug648.js

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
/*---
2+
negative:
3+
phase: runtime
4+
type: Error
5+
---*/
6+
let finrec = new FinalizationRegistry(v => {})
7+
let object = {object:"object"}
8+
finrec.register(object, {held:"held"}, {token:"token"})
9+
object = undefined
10+
// abrupt termination should not leak |held|
11+
// unfortunately only shows up in qjs, not run-test262,
12+
// but still good to have a regression test
13+
throw Error("ok")

0 commit comments

Comments
 (0)