diff --git a/doc/api/ffi.md b/doc/api/ffi.md index 94dc9e42f03d..56d3329b6315 100644 --- a/doc/api/ffi.md +++ b/doc/api/ffi.md @@ -459,6 +459,10 @@ memory. Keeps the callback strongly referenced by JavaScript. +Throws `ERR_INVALID_ARG_VALUE` if the callback function has already been +garbage collected after a previous `library.unrefCallback(pointer)` call, since +a collected function cannot be referenced again. + ### `library.unrefCallback(pointer)` * `pointer` {bigint} @@ -469,6 +473,9 @@ If the callback function is later garbage collected, subsequent native invocations become a no-op. Non-void return values are zero-initialized before returning to native code. +Throws `ERR_INVALID_ARG_VALUE` if the callback function has already been +garbage collected. + ## Calling native functions Argument conversion depends on the declared FFI type. diff --git a/src/node_ffi.cc b/src/node_ffi.cc index b05d09270126..4424ad6afbc6 100644 --- a/src/node_ffi.cc +++ b/src/node_ffi.cc @@ -1125,6 +1125,15 @@ void DynamicLibrary::RefCallback(const FunctionCallbackInfo& args) { return; } + // The callback function may already have been collected after a previous + // unrefCallback() call. The persistent handle is empty in that case, and + // ClearWeak() on an empty handle dereferences a null slot. There is also no + // function left to make strong again. + if (existing->second->fn.IsEmpty()) { + THROW_ERR_INVALID_ARG_VALUE(env, "Callback not found"); + return; + } + existing->second->fn.ClearWeak(); } @@ -1156,6 +1165,14 @@ void DynamicLibrary::UnrefCallback(const FunctionCallbackInfo& args) { return; } + // The callback function may already have been collected by a previous + // unrefCallback() call. The persistent handle is empty in that case, and + // SetWeak() on an empty handle dereferences a null slot. + if (existing->second->fn.IsEmpty()) { + THROW_ERR_INVALID_ARG_VALUE(env, "Callback not found"); + return; + } + existing->second->fn.SetWeak(); } diff --git a/test/ffi/test-ffi-weakref-calls.js b/test/ffi/test-ffi-weakref-calls.js index 1a2a2eaeab87..b158ee810776 100644 --- a/test/ffi/test-ffi-weakref-calls.js +++ b/test/ffi/test-ffi-weakref-calls.js @@ -51,6 +51,37 @@ test('ffi refCallback retains callback function', async (t) => { lib.unregisterCallback(pointer); }); +test('callback ref/unref throw after callback function is collected', async (t) => { + const { lib } = ffi.dlopen(libraryPath, fixtureSymbols); + t.after(() => lib.close()); + + let callback = () => 1; + const ref = new WeakRef(callback); + const pointer = lib.registerCallback( + { arguments: ['i32'], return: 'i32' }, + callback, + ); + + lib.unrefCallback(pointer); + callback = null; + + await gcUntil( + 'callback ref/unref throw after callback function is collected', + () => ref.deref() === undefined, + ); + + t.assert.throws(() => lib.unrefCallback(pointer), { + code: 'ERR_INVALID_ARG_VALUE', + message: /Callback not found/, + }); + t.assert.throws(() => lib.refCallback(pointer), { + code: 'ERR_INVALID_ARG_VALUE', + message: /Callback not found/, + }); + + lib.unregisterCallback(pointer); +}); + test('callback ref/unref/unregister throw when library is closed', (t) => { const { lib } = ffi.dlopen(libraryPath, fixtureSymbols); const callback = lib.registerCallback(() => {});