Skip to content

Commit

Permalink
Fix FinalizationRegistry refcounting bug (#656)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
bnoordhuis authored Nov 7, 2024
1 parent aedd829 commit 9c5c441
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 6 deletions.
10 changes: 4 additions & 6 deletions quickjs.c
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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;
Expand Down
13 changes: 13 additions & 0 deletions tests/bug648.js
Original file line number Diff line number Diff line change
@@ -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")

0 comments on commit 9c5c441

Please sign in to comment.