Skip to content

Commit

Permalink
Run FinalizationRegistry callback in the job queue
Browse files Browse the repository at this point in the history
The spec says HostMakeJobCallback has to be used on the callback: https://tc39.es/ecma262/multipage/managing-memory.html#sec-finalization-registry-cleanup-callback

That makes the following (arguably contrived) example run forever until
memory is exhausted.

```js
let count = 0;
function main() {
    console.log(`main! ${++count}`);
    const registry = new FinalizationRegistry(() => {
        globalThis.foo = main();
    });
    registry.register([]);
    registry.register([]);
    return registry;
}
main();

console.log(count);
```

That is unlike V8, which runs 0 times. This can be explained by the
difference in GC implementations and since FinRec makes GC observable,
here we are!

Fixes: #432
  • Loading branch information
saghul committed Sep 9, 2024
1 parent c740aa0 commit 61c8fe6
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 10 deletions.
23 changes: 17 additions & 6 deletions quickjs.c
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,8 @@ struct JSRuntime {
BOOL in_out_of_memory : 8;
/* and likewise if inside Error.prepareStackTrace() */
BOOL in_prepare_stack_trace : 8;
/* true if inside JS_FreeRuntime */
BOOL in_free : 8;

struct JSStackFrame *current_stack_frame;

Expand Down Expand Up @@ -1812,6 +1814,8 @@ int JS_EnqueueJob(JSContext *ctx, JSJobFunc *job_func,
JSJobEntry *e;
int i;

assert(!rt->in_free);

e = js_malloc(ctx, sizeof(*e) + argc * sizeof(JSValue));
if (!e)
return -1;
Expand Down Expand Up @@ -1932,6 +1936,7 @@ void JS_FreeRuntime(JSRuntime *rt)
struct list_head *el, *el1;
int i;

rt->in_free = TRUE;
JS_FreeValueRT(rt, rt->current_exception);

list_for_each_safe(el, el1, &rt->job_list) {
Expand Down Expand Up @@ -53228,6 +53233,13 @@ static const JSClassShortDef js_finrec_class_def[] = {
{ JS_ATOM_FinalizationRegistry, js_finrec_finalizer, js_finrec_mark }, /* JS_CLASS_FINALIZATION_REGISTRY */
};

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;
}

void JS_AddIntrinsicWeakRef(JSContext *ctx)
{
JSRuntime *rt = ctx->rt;
Expand Down Expand Up @@ -53310,12 +53322,11 @@ static void reset_weak_ref(JSRuntime *rt, JSWeakRefRecord **first_weak_ref)
/**
* During the GC sweep phase the held object might be collected first.
*/
if (!JS_IsObject(fre->held_val) || JS_IsLiveObject(frd->ctx->rt, fre->held_val)) {
JSValue func = js_dup(frd->cb);
JSValue ret = JS_Call(frd->ctx, func, JS_UNDEFINED, 1, &fre->held_val);
JS_FreeValueRT(rt, func);
JS_FreeValueRT(rt, ret);
JS_FreeValueRT(rt, fre->held_val);
if (!rt->in_free && (!JS_IsObject(fre->held_val) || JS_IsLiveObject(rt, fre->held_val))) {
JSValue args[2];
args[0] = frd->cb;
args[1] = fre->held_val;
JS_EnqueueJob(frd->ctx, js_finrec_job, 2, args);
}
JS_FreeValueRT(rt, fre->token);
js_free_rt(rt, fre);
Expand Down
13 changes: 9 additions & 4 deletions tests/test_builtin.js
Original file line number Diff line number Diff line change
Expand Up @@ -998,14 +998,19 @@ function test_finalization_registry()
let expected = {};
let actual;
let finrec = new FinalizationRegistry(v => { actual = v });
finrec.register({}, expected); // {} goes out of scope immediately
assert(actual, expected);
finrec.register({}, expected);
queueMicrotask(() => {
assert(actual, expected);
});
}
{
let expected = 42;
let actual;
let finrec = new FinalizationRegistry(v => { actual = v });
finrec.register({}, 42); // {} goes out of scope immediately
assert(actual, 42);
finrec.register({}, expected);
queueMicrotask(() => {
assert(actual, expected);
});
}
}

Expand Down

0 comments on commit 61c8fe6

Please sign in to comment.