Skip to content

fix(ffi/objc): forward off-thread method and function callbacks to the JS thread - #103

Open
NathanWalker wants to merge 1 commit into
feat/napi-host-runloop-callbacksfrom
feat/napi-desktop-callbacks
Open

NathanWalker wants to merge 1 commit into
feat/napi-host-runloop-callbacksfrom
feat/napi-desktop-callbacks

Conversation

@NathanWalker

@NathanWalker NathanWalker commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #102 (feat/napi-host-runloop-callbacks).

This is part 1 of the split requested in review: the off-thread method/function crash fix only. The other parts are now separate PRs:

Summary

 native callback on a thread other than the JS thread (Node-API builds)
-  block               -> per-block tsfn, caller waits for JS
-  method / C function -> JS runs on the caller's thread -> V8 abort
+  method / function / block -> one tsfn per env, caller waits for JS
+                               (autorelease pool per forwarded call)
  • Same blocking semantics blocks already had; methods and C function pointers now take that path instead of running JS on the caller's thread.
  • One threadsafe function per env replaces the one created per block.
  • An Objective-C exception raised while the forwarded call runs is rethrown on the caller's thread. If the env is gone, the caller gets a zeroed return value.
  • ENABLE_JS_RUNTIME builds are unchanged.

Evidence

macOS 26.6.2 (arm64), --macos --no-engine build. Checks: a JS method invoked with performSelectorInBackground:withObject: (5 calls), a JS block on a global dispatch queue, a JS C function pointer through dispatch_async_f, and a second block afterwards.

  • Before (fix(ffi/objc): enter a Node callback scope for native callbacks from a host-owned run loop #102): the method case aborts with Fatal error in v8::HandleScope::CreateHandle().
  • After: all four pass, with every call running on the JS thread, on Node v26.10.0, Deno 2.9.1, Bun 1.4.2 and Electron 45.1.0.
  • MACOS_TEST_FFI_BACKEND=napi npm run test:macos: tests=714, failures=0, errors=0, skipped=17 (the napi bridge in engine builds; the changed code is Node-API-only, so this checks engine builds are unaffected)

Merge Danger

Door: two-way

Blast Radius: Node-API builds, off-thread callbacks

@coderabbitai

coderabbitai Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 73042d03-0852-4b52-ab46-a68b89a58462

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@ammarahm-ed

Copy link
Copy Markdown
Collaborator

This PR fixes four unrelated problems at once, and three of them change behavior outside the Node build. Please split it up. Notes on each part:

Context: none of this affects NativeScript apps on iOS. There our runtime owns the engine. A background-thread callback takes the V8 lock and runs JS on its own thread, and Hermes releases its lock during native calls, which is how the VideoToolbox-style deadlock is avoided there. Every framework is also linked at launch, so all classes exist when we register them. In Node we can't do any of that, so the threadsafe function is the only way back to the JS thread.

1. Off-thread method and function callbacks crash.
This is a real bug in the Node build. The focused fix is to send methods and functions through the same blocking threadsafe-function path blocks already use. Using one threadsafe function per env instead of one per block is fine. This part is small, changes no behavior, and can go in first on its own.

2. Queueing void callbacks so the caller returns at once.
I agree this is the only robust way out of the deadlock. "Queue only while JS is inside a native call" is racy and still deadlocks in the VideoToolbox case. But it changes callback semantics, so it should be its own PR, with these fixed:

  • Unknown argument types are copied by default. Flip it: list the types that are safe to copy and block for everything else. A struct passed by value that contains pointers is copied shallowly and can dangle by the time JS runs.
  • No backpressure. The queue is unbounded and keeps every argument retained. If JS falls behind a capture stream, retained sample buffers pile up and the pool stalls, which is the same symptom this PR is trying to fix. At least document it, and ideally give callers a way to drop work.
  • Native code that reuses a buffer once the callback returns. Audio tap buffers are one case, I believe. A queued callback reads overwritten data. Document it.
  • Errors. A JS error in a queued callback becomes a fatal process exception, while the same error on the JS thread goes through the normal uncaught-error handling. They should behave the same.

3. Metadata generator change.
The generator is shared with iOS, so the next metadata regen changes NativeScript iOS apps too. Block parameters that used to be pointers become objects, and the typings change. That breaks existing code, so the description's claim that our runtime builds are unchanged isn't true. It also hides a dependency on part 2: the VideoToolbox case only passes with regenerated metadata, because a raw-pointer argument isn't allowed in the queue and the call goes back to blocking. Needs its own PR and an iOS test run.

4. releaseNativeObject.
The need is real, and the implementation looks right. It's a new public API, though, so it should match Android's __releaseNativeCounterpart in name and behavior, so plugins can call one thing on both platforms. Own PR.

5. Class lookup for lazily loaded frameworks.
It's a real problem, but the fix sits on the hot path. Every class not found in the map now costs a string allocation plus a name lookup on every object wrap, at every superclass level for private subclasses like __NSCFString. The unresolved list almost never empties, so that cost never goes away. Either remember the classes that already missed, or resolve pending classes when a framework image loads, so lookups stay a single pointer-map hit.

Also:

  • Please drop the reformatting (about 150 lines). ObjCBridge.h is rewrapped to 80 columns while the other files are rewrapped to 100.
  • Run npm run test:macos before merging any of these.

…e JS thread

Methods and C function pointers implemented in JS ran on the caller's
thread when invoked from another one, which aborts V8. They now take the
same blocking threadsafe-function path blocks use. One threadsafe function
per env replaces the one created per block, and each forwarded call runs
in its own autorelease pool.
@NathanWalker
NathanWalker force-pushed the feat/napi-desktop-callbacks branch from b6b27f6 to e4c8455 Compare November 7, 2026 19:50
@NathanWalker NathanWalker changed the title fix(ffi/objc): forward foreign-thread callbacks without deadlocks, add releaseNativeObject fix(ffi/objc): forward off-thread method and function callbacks to the JS thread Nov 7, 2026
@NathanWalker

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I split this into five PRs as suggested:

Part PR Base
Off-thread method/function crash fix: the same blocking threadsafe-function path blocks already use, with one tsfn per env #103 (this PR, rewritten) #102
Queued void callbacks #115 #103
Metagen block parameter typedefs #116 android-react-native
__releaseNativeCounterpart #113 android-react-native
Class lookup for frameworks loaded after init #114 android-react-native
  • fix(ffi/objc): forward off-thread method and function callbacks to the JS thread #103: the per-block tsfn is gone, along with its registration in Block.mm and the condvar fields. Methods and functions check jsThreadId like blocks do. An exception thrown on the JS thread is rethrown on the calling thread, and the return value is zeroed if the call never ran.
  • feat(ffi/objc): queue void callbacks from other threads instead of waiting #115: only an explicit allowlist of argument types is queued: scalars, plain structs (an ffi_type walk rejects structs with pointer fields), retained ObjC/CF objects, and copied blocks. Everything else, plus setters and non-void returns, still waits for JS. Errors from a queued callback go through the normal uncaught-error handling. The missing backpressure and buffer reuse are documented in the code and the PR. Whether to add a way to drop queued work is left as an open question there.
  • fix(metagen): keep typedef'd CF types for block parameters #116: includes an iOS simulator run. The 8 failures are the same double/Float64 cases as on the base generator. The PR is marked as a breaking change for apps that regenerate metadata, with the typings impact and a migration note, and adds a regression test that fails on the old generator. It also raises the iOS deployment target to 15.0 in its own commit, because Xcode 27 can't build below that.
  • feat(ffi/objc): add __releaseNativeCounterpart to the Node-API bridge #113: matches Android's name and argument errors. It now releases instances of JS-defined classes, which the original refused, because Android and the existing tests expect that.
  • fix(ffi/objc): map metadata classes from frameworks loaded after init #114: a found class costs the same single map lookup as before. Classes from frameworks loaded later are resolved from an objc_addLoadImageFunc hook, so a miss costs only an atomic compare.

All the reformatting is dropped. Every PR passes MACOS_TEST_FFI_BACKEND=napi npm run test:macos and the default V8-bridge run with 0 failures. The off-thread checks for #103 and #115 pass on Node, Deno and Bun, and #103's also pass in Electron.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants