Skip to content
160 changes: 27 additions & 133 deletions src/jsc/bindings/JSFFIFunction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -184,24 +184,26 @@

} // namespace JSC

// Shared tail for the FFI_Callback_* entry points: call back into JS and leave any exception
// pending on the VM, like any other host function. Clearing + re-throwing it (the old NakedPtr
// dance) violated VM::setException's precondition for worker.terminate()'s TerminationException
// once the outermost VMEntryScope had already retired the termination request.

Check warning on line 190 in src/jsc/bindings/JSFFIFunction.cpp

View check run for this annotation

Claude / Claude Code Review

Comment on invokeFFICallback exceeds 3-line max and contains bug history

nit: this comment block is 4 lines and references deleted code ("the old NakedPtr dance"), running into root `CLAUDE.md` rule 13 ("Keep code comments to 3 lines max") and the durable-content guideline ("no bug history — that belongs in the PR description"). The PR description already covers the NakedPtr/`VM::setException` history; consider trimming to just the durable invariant — leave the exception pending, don't clear+rethrow (TerminationException) — in ≤3 lines.
Comment thread
robobun marked this conversation as resolved.
Outdated
static JSC::EncodedJSValue invokeFFICallback(Zig::GlobalObject* globalObject, JSC::JSFunction* function, JSC::MarkedArgumentBuffer& arguments)
{
auto& vm = JSC::getVM(globalObject);
auto scope = DECLARE_THROW_SCOPE(vm);
auto result = JSC::profiledCall(globalObject, JSC::ProfilingReason::API, function, JSC::getCallData(function), JSC::jsUndefined(), arguments);
RETURN_IF_EXCEPTION(scope, JSC::JSValue::encode(JSC::jsNull()));
return JSC::JSValue::encode(result);
}

extern "C" JSC::EncodedJSValue
FFI_Callback_call(FFICallbackFunctionWrapper& wrapper, size_t argCount, JSC::EncodedJSValue* args)
{
auto* function = wrapper.m_function.get();
auto* globalObject = wrapper.globalObject.get();
auto& vm = JSC::getVM(globalObject);
JSC::MarkedArgumentBuffer arguments;
for (size_t i = 0; i < argCount; ++i)
arguments.appendWithCrashOnOverflow(JSC::JSValue::decode(args[i]));
WTF::NakedPtr<JSC::Exception> exception;
auto result = JSC::profiledCall(globalObject, JSC::ProfilingReason::API, function, JSC::getCallData(function), JSC::jsUndefined(), arguments, exception);
if (exception) [[unlikely]] {
auto scope = DECLARE_THROW_SCOPE(vm);
scope.throwException(globalObject, exception);
return JSC::JSValue::encode(JSC::jsNull());
}

return JSC::JSValue::encode(result);
return invokeFFICallback(wrapper.globalObject.get(), wrapper.m_function.get(), arguments);
}

extern "C" void
Expand All @@ -214,186 +216,87 @@

WebCore::ScriptExecutionContext::postTaskTo(wrapper.m_contextId, [argsVec = WTF::move(argsVec), protectedWrapper = Ref { wrapper }](WebCore::ScriptExecutionContext& ctx) mutable {
Comment thread
robobun marked this conversation as resolved.
auto* globalObject = uncheckedDowncast<Zig::GlobalObject>(ctx.jsGlobalObject());
auto& vm = JSC::getVM(globalObject);
// The worker may have been terminated (or the VM begun shutting down) between
// enqueue and dispatch; never re-enter JS on a stopped global.
if (Zig::GlobalObject::scriptExecutionStatus(globalObject, globalObject) != JSC::ScriptExecutionStatus::Running) [[unlikely]]
return;
Comment thread
robobun marked this conversation as resolved.
Outdated
JSC::MarkedArgumentBuffer arguments;
auto* function = protectedWrapper->m_function.get();
for (size_t i = 0; i < argsVec.size(); ++i)
arguments.appendWithCrashOnOverflow(JSC::JSValue::decode(argsVec[i]));
WTF::NakedPtr<JSC::Exception> exception;
JSC::profiledCall(globalObject, JSC::ProfilingReason::API, function, JSC::getCallData(function), JSC::jsUndefined(), arguments, exception);
if (exception) [[unlikely]] {
auto scope = DECLARE_THROW_SCOPE(vm);
scope.throwException(globalObject, exception);
return;
}
invokeFFICallback(globalObject, protectedWrapper->m_function.get(), arguments);
});
Comment thread
robobun marked this conversation as resolved.
}

extern "C" JSC::EncodedJSValue
FFI_Callback_call_0(FFICallbackFunctionWrapper& wrapper, size_t argCount, JSC::EncodedJSValue* args)
{
auto* function = wrapper.m_function.get();
auto* globalObject = wrapper.globalObject.get();
auto& vm = JSC::getVM(globalObject);

JSC::MarkedArgumentBuffer arguments;

WTF::NakedPtr<JSC::Exception> exception;
auto result = JSC::profiledCall(globalObject, JSC::ProfilingReason::API, function, JSC::getCallData(function), JSC::jsUndefined(), arguments, exception);
if (exception) [[unlikely]] {
auto scope = DECLARE_THROW_SCOPE(vm);
scope.throwException(globalObject, exception);
return JSC::JSValue::encode(JSC::jsNull());
}

return JSC::JSValue::encode(result);
return invokeFFICallback(wrapper.globalObject.get(), wrapper.m_function.get(), arguments);
}

extern "C" JSC::EncodedJSValue
FFI_Callback_call_1(FFICallbackFunctionWrapper& wrapper, size_t argCount, JSC::EncodedJSValue* args)
{
auto* function = wrapper.m_function.get();
auto* globalObject = wrapper.globalObject.get();
auto& vm = JSC::getVM(globalObject);

JSC::MarkedArgumentBuffer arguments;
arguments.append(JSC::JSValue::decode(args[0]));

WTF::NakedPtr<JSC::Exception> exception;
auto result = JSC::profiledCall(globalObject, JSC::ProfilingReason::API, function, JSC::getCallData(function), JSC::jsUndefined(), arguments, exception);
if (exception) [[unlikely]] {
auto scope = DECLARE_THROW_SCOPE(vm);
scope.throwException(globalObject, exception);
return JSC::JSValue::encode(JSC::jsNull());
}

return JSC::JSValue::encode(result);
return invokeFFICallback(wrapper.globalObject.get(), wrapper.m_function.get(), arguments);
}

extern "C" JSC::EncodedJSValue
FFI_Callback_call_2(FFICallbackFunctionWrapper& wrapper, size_t argCount, JSC::EncodedJSValue* args)
{
auto* function = wrapper.m_function.get();
auto* globalObject = wrapper.globalObject.get();
auto& vm = JSC::getVM(globalObject);

JSC::MarkedArgumentBuffer arguments;
arguments.append(JSC::JSValue::decode(args[0]));
arguments.append(JSC::JSValue::decode(args[1]));

WTF::NakedPtr<JSC::Exception> exception;
auto result = JSC::profiledCall(globalObject, JSC::ProfilingReason::API, function, JSC::getCallData(function), JSC::jsUndefined(), arguments, exception);
if (exception) [[unlikely]] {
auto scope = DECLARE_THROW_SCOPE(vm);
scope.throwException(globalObject, exception);
return JSC::JSValue::encode(JSC::jsNull());
}

return JSC::JSValue::encode(result);
return invokeFFICallback(wrapper.globalObject.get(), wrapper.m_function.get(), arguments);
}

extern "C" JSC::EncodedJSValue FFI_Callback_call_3(FFICallbackFunctionWrapper& wrapper, size_t argCount, JSC::EncodedJSValue* args)
{
auto* function = wrapper.m_function.get();
auto* globalObject = wrapper.globalObject.get();
auto& vm = JSC::getVM(globalObject);

JSC::MarkedArgumentBuffer arguments;
arguments.append(JSC::JSValue::decode(args[0]));
arguments.append(JSC::JSValue::decode(args[1]));
arguments.append(JSC::JSValue::decode(args[2]));

WTF::NakedPtr<JSC::Exception> exception;
auto result = JSC::profiledCall(globalObject, JSC::ProfilingReason::API, function, JSC::getCallData(function), JSC::jsUndefined(), arguments, exception);
if (exception) [[unlikely]] {
auto scope = DECLARE_THROW_SCOPE(vm);
scope.throwException(globalObject, exception);
return JSC::JSValue::encode(JSC::jsNull());
}

return JSC::JSValue::encode(result);
return invokeFFICallback(wrapper.globalObject.get(), wrapper.m_function.get(), arguments);
}

extern "C" JSC::EncodedJSValue FFI_Callback_call_4(FFICallbackFunctionWrapper& wrapper, size_t argCount, JSC::EncodedJSValue* args)
{
auto* function = wrapper.m_function.get();
auto* globalObject = wrapper.globalObject.get();
auto& vm = JSC::getVM(globalObject);

JSC::MarkedArgumentBuffer arguments;
arguments.append(JSC::JSValue::decode(args[0]));
arguments.append(JSC::JSValue::decode(args[1]));
arguments.append(JSC::JSValue::decode(args[2]));
arguments.append(JSC::JSValue::decode(args[3]));

WTF::NakedPtr<JSC::Exception> exception;
auto result = JSC::profiledCall(globalObject, JSC::ProfilingReason::API, function, JSC::getCallData(function), JSC::jsUndefined(), arguments, exception);
if (exception) [[unlikely]] {
auto scope = DECLARE_THROW_SCOPE(vm);
scope.throwException(globalObject, exception);
return JSC::JSValue::encode(JSC::jsNull());
}

return JSC::JSValue::encode(result);
return invokeFFICallback(wrapper.globalObject.get(), wrapper.m_function.get(), arguments);
}

extern "C" JSC::EncodedJSValue FFI_Callback_call_5(FFICallbackFunctionWrapper& wrapper, size_t argCount, JSC::EncodedJSValue* args)
{
auto* function = wrapper.m_function.get();
auto* globalObject = wrapper.globalObject.get();
auto& vm = JSC::getVM(globalObject);

JSC::MarkedArgumentBuffer arguments;
arguments.append(JSC::JSValue::decode(args[0]));
arguments.append(JSC::JSValue::decode(args[1]));
arguments.append(JSC::JSValue::decode(args[2]));
arguments.append(JSC::JSValue::decode(args[3]));
arguments.append(JSC::JSValue::decode(args[4]));

WTF::NakedPtr<JSC::Exception> exception;
auto result = JSC::profiledCall(globalObject, JSC::ProfilingReason::API, function, JSC::getCallData(function), JSC::jsUndefined(), arguments, exception);
if (exception) [[unlikely]] {
auto scope = DECLARE_THROW_SCOPE(vm);
scope.throwException(globalObject, exception);
return JSC::JSValue::encode(JSC::jsNull());
}

return JSC::JSValue::encode(result);
return invokeFFICallback(wrapper.globalObject.get(), wrapper.m_function.get(), arguments);
}

extern "C" JSC::EncodedJSValue
FFI_Callback_call_6(FFICallbackFunctionWrapper& wrapper, size_t argCount, JSC::EncodedJSValue* args)
{
auto* function = wrapper.m_function.get();
auto* globalObject = wrapper.globalObject.get();
auto& vm = JSC::getVM(globalObject);

JSC::MarkedArgumentBuffer arguments;
arguments.append(JSC::JSValue::decode(args[0]));
arguments.append(JSC::JSValue::decode(args[1]));
arguments.append(JSC::JSValue::decode(args[2]));
arguments.append(JSC::JSValue::decode(args[3]));
arguments.append(JSC::JSValue::decode(args[4]));
arguments.append(JSC::JSValue::decode(args[5]));

WTF::NakedPtr<JSC::Exception> exception;
auto result = JSC::profiledCall(globalObject, JSC::ProfilingReason::API, function, JSC::getCallData(function), JSC::jsUndefined(), arguments, exception);
if (exception) [[unlikely]] {
auto scope = DECLARE_THROW_SCOPE(vm);
scope.throwException(globalObject, exception);
return JSC::JSValue::encode(JSC::jsNull());
}

return JSC::JSValue::encode(result);
return invokeFFICallback(wrapper.globalObject.get(), wrapper.m_function.get(), arguments);
}

extern "C" JSC::EncodedJSValue
FFI_Callback_call_7(FFICallbackFunctionWrapper& wrapper, size_t argCount, JSC::EncodedJSValue* args)
{
auto* function = wrapper.m_function.get();
auto* globalObject = wrapper.globalObject.get();
auto& vm = JSC::getVM(globalObject);

JSC::MarkedArgumentBuffer arguments;
arguments.append(JSC::JSValue::decode(args[0]));
arguments.append(JSC::JSValue::decode(args[1]));
Expand All @@ -402,14 +305,5 @@
arguments.append(JSC::JSValue::decode(args[4]));
arguments.append(JSC::JSValue::decode(args[5]));
arguments.append(JSC::JSValue::decode(args[6]));

WTF::NakedPtr<JSC::Exception> exception;
auto result = JSC::profiledCall(globalObject, JSC::ProfilingReason::API, function, JSC::getCallData(function), JSC::jsUndefined(), arguments, exception);
if (exception) [[unlikely]] {
auto scope = DECLARE_THROW_SCOPE(vm);
scope.throwException(globalObject, exception);
return JSC::JSValue::encode(JSC::jsNull());
}

return JSC::JSValue::encode(result);
return invokeFFICallback(wrapper.globalObject.get(), wrapper.m_function.get(), arguments);
}
99 changes: 98 additions & 1 deletion test/js/bun/ffi/ffi.test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { afterAll, describe, expect, it } from "bun:test";
import { existsSync } from "fs";
import { isGlibcVersionAtLeast } from "harness";
import { bunEnv, bunExe, isGlibcVersionAtLeast, tempDir } from "harness";
import { platform } from "os";

import {
Expand Down Expand Up @@ -677,6 +677,103 @@ it(".ptr is not leaked", () => {
}
});

it("JSCallback exceptions propagate out of the native call", () => {
const callback = new JSCallback(
() => {
throw new Error("boom");
},
{ returns: "int32_t", args: [] },
);
const call = new CFunction({ ptr: callback.ptr, returns: "int32_t", args: [] });
try {
expect(call).toThrow("boom");
} finally {
callback.close();
}
});

// worker.terminate() while the worker is inside a threadsafe JSCallback made FFI_Callback_*
// clear and re-throw JSC's TerminationException, which trips
// "ASSERTION FAILED: !isTerminationException(exception) || hasTerminationRequest()" in
// JSC::VM::setException and re-enters JS on the terminated worker VM.
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
it("JSCallback tolerates worker.terminate() arriving inside the callback", async () => {
Comment thread
robobun marked this conversation as resolved.
Outdated
using dir = tempDir("ffi-jscallback-terminate", {
"main.js": `
import { join } from "node:path";
import { Worker } from "node:worker_threads";

const sab = new SharedArrayBuffer(4);
const flag = new Int32Array(sab);

const worker = new Worker(join(import.meta.dir, "worker.js"), { workerData: sab });
let terminating = false;
worker.on("error", err => {
console.error("worker error:", err);
process.exit(1);
});
worker.on("exit", code => {
if (!terminating) {
console.error("worker exited early:", code);
process.exit(1);
}
});

// Wait until the worker thread is inside the native -> JS callback frame.
while (Atomics.load(flag, 0) === 0) {
await Bun.sleep(5);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

terminating = true;
await worker.terminate();
console.log("done");
`,
"worker.js": `
import { CFunction, JSCallback } from "bun:ffi";
import { workerData } from "node:worker_threads";

const flag = new Int32Array(workerData);

const callback = new JSCallback(
() => {
// Tell the parent we are inside the native -> JS callback frame, then
// spin until worker.terminate() delivers the TerminationException.
Atomics.store(flag, 0, 1);
while (true) {}
},
{ returns: "void", args: [], threadsafe: true },
);

// CFunction turns the callback's native function pointer back into a callable, so
// each fire() re-enters JS through the native FFI trampoline. A threadsafe callback
// enqueues a task instead of calling synchronously, so both run at the top of the
// worker's event loop once this module finishes evaluating.
const fire = new CFunction({ ptr: callback.ptr, returns: "void", args: [] });
fire();
fire();

// Keep the worker alive until the queued callback tasks run.
setInterval(() => {}, 1000);
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "main.js"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
// stderr is not asserted (debug builds write benign noise there), but including it in the
// received object surfaces the crash output whenever one of the other fields mismatches.
expect({ stdout, stderr, exitCode, signalCode: proc.signalCode }).toEqual({
stdout: "done\n",
stderr: expect.any(String),
exitCode: 0,
signalCode: null,
});
});

const libPath =
platform() === "darwin"
? "/usr/lib/libSystem.B.dylib"
Expand Down
Loading