JSC Node-API: reference primitives and coerce property receivers (fixes a RELEASE_ASSERT on non-object exceptions)

[matthargett]

Problem

ToJSObject in the JavaScriptCore Node-API backend is a reinterpret_cast whose assert is compiled out in RelWithDebInfo, so every entry point that handed a caller value straight to a JSObject* API tripped JavaScriptCore's RELEASE_ASSERT (JSC::JSCell::getOwnPropertySlot) when given a primitive. macOS reports that as EXC_BREAKPOINT/SIGKILL, so the process just disappears with exit 137.

The reachable case is any non-object exception: node-addon-api's Napi::Error wraps a pending exception with napi_create_reference, which walked straight into JSObjectHasPropertyForKey on the value. throw "text" from script does it, and so does JavaScriptCore's own execution-time-limit termination exception, which has been the bare string "JavaScript execution terminated." since 2021 (VM::ensureTerminationException) — terminating a busy worker through JSContextGroupSetExecutionTimeLimit killed the process.

Changes

• napi_create_reference follows Node's rules: objects keep the existing sentinel scheme; symbols are held for the life of the reference (the C API has no weak handle for them, and a weak symbol reference must keep resolving while the symbol is alive); other primitives are strong while the count is positive and released at zero (Node-API 10 semantics; napi_invalid_arg before 10, as Node does). napi_get_reference_value now reports NULL instead of leaving *result unset, and napi_reference_unref refuses an already-zero count (napi_generic_failure, as Node).
• Property get/set/has/delete (named, keyed, indexed) and napi_get_prototype coerce the receiver with ToObject like Node's CHECK_TO_OBJECT; null/undefined report napi_object_expected with the TypeError pending.
• napi_wrap/napi_unwrap/napi_remove_wrap/napi_add_finalizer, napi_get_value_external, napi_get_array_length (napi_array_expected), napi_call_function, napi_new_instance and napi_instanceof (napi_function_expected) validate their object/function argument first instead of reinterpreting it.

Tests

• NodeApi.PrimitiveExceptionSurvivesNativeCatch (every engine except the V8JSI shim, which surfaces a script throw of a primitive as jsi::JSError rather than Napi::Error): throw 'plain text' caught as Napi::Error, Message() callable, runtime still evaluates afterwards. Crashed the process on JavaScriptCore before this change; on Hermes the script has to go through Napi::Eval, since Env::RunScript calls the 4-argument napi_run_script that Hermes does not implement.
• NodeApi.PropertyAccessCoercesPrimitiveReceiver and NodeApi.ReferencesToPrimitivesFollowNode (JavaScriptCore only, via a new JSRUNTIMEHOST_NAPI_ENGINE_JAVASCRIPTCORE test define): pin the coercion and reference semantics, including the NAPI_VERSION >= 10 branch.

Verified locally on macOS (system JavaScriptCore): full UnitTests green. Fork twin: rebeckerspecialties#24.

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings remain in the implementation, along with test-coverage gaps.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR hardens the JavaScriptCore Node-API backend against primitive values and adds regression coverage.

Changes:

• Adds primitive-aware reference lifecycle handling.
• Coerces property receivers and validates API arguments.
• Adds JavaScriptCore-specific tests and build configuration.

File summaries

File	Changes and final review findings
Tests/UnitTests/Shared/Shared.cpp	Adds exception, property, and reference regression tests. Nit (2 votes): NAPI 10 reference behavior is not exercised by the default test build.
Tests/UnitTests/CMakeLists.txt	Enables JavaScriptCore-specific tests. Nit (2 votes): The Android JSC target lacks the corresponding test definition.
Core/Node-API/Source/js_native_api_javascriptcore.cc	Implements coercion, validation, and primitive references. Critical (2 votes): Zero-count object references may promote stale pointers. Critical (3 votes): napi_call_function does not handle primitive receivers. Moderate (3 votes): napi_instanceof accepts any object as a constructor. Moderate (1 vote): Zero-count primitive references incorrectly return success when re-referenced. Moderate (1 vote): Callable validation is missing in napi_call_function. Moderate (1 vote): napi_get_value_external accepts ordinary objects instead of rejecting them.

Review details

Suppressed comments (3)

Core/Node-API/Source/js_native_api_javascriptcore.cc:808

• After a Node-API 10 primitive reference is unref'd from 1 to 0, unref() clears _value. A subsequent napi_reference_ref() enters this early return, leaves the count at zero, and the public function still returns napi_ok; the API contract requires an error when a zero-count target is unavailable. Return success/failure from ref() and propagate napi_generic_failure here, with a regression assertion.

    if (_value == nullptr) {
      // A primitive released at count zero cannot come back; Node reports a count of zero too.
      return;

Core/Node-API/Source/js_native_api_javascriptcore.cc:1765

• JSValueIsObject is not a callable check. A plain object passes this guard and is still handed to JSObjectCallAsFunction, so the call reports a pending TypeError instead of rejecting the non-function during argument validation; the existing napi_typeof predicate (JSObjectIsFunction/JSObjectIsConstructor) shows how this backend handles callable constructors. Check that predicate before invoking.

  RETURN_STATUS_IF_FALSE(env, JSValueIsObject(env->context, ToJSValue(func)), napi_function_expected);

Core/Node-API/Source/js_native_api_javascriptcore.cc:2161

• The new object check prevents the cast from crashing on primitives, but ordinary objects still take the info == nullptr path and return napi_ok with *result == nullptr. napi_get_value_external should reject a non-external value; the V8 backend returns napi_invalid_arg for this case (js_native_api_v8.cc:2632-2638). Check the retrieved ExternalInfo type and return napi_invalid_arg instead.

  RETURN_STATUS_IF_FALSE(env, JSValueIsObject(env->context, ToJSValue(value)), napi_invalid_arg);

  ExternalInfo* info = NativeInfo::Get<ExternalInfo>(ToJSObject(env, value));
  *result = (info != nullptr && info->Type() == NativeType::External) ? info->Data() : nullptr;

• Files reviewed: 3/3 changed files
• Comments generated: 5
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[matthargett]

Agreed that the NAPI_VERSION >= 10 branch is not exercised by the checked-in configuration: the tree is pinned to 5 (see the discussion on #229), and the only difference above 10 is that non-symbol primitives are held instead of refused, which the fork's NAPI_VERSION-bump branch builds with. I would rather not add a second JavaScriptCore job to the matrix in a crash-fix PR; if you prefer, I can drop the version-gated branch here and reintroduce it with the version bump.

[matthargett]

Review addressed in f8e8152 (weak-object promotion check, receiver coercion in napi_call_function, instanceof constructor check, Android test define + receiver regression test). Fork twin rebeckerspecialties#24 at that head: 24/24 green — https://github.com/rebeckerspecialties/JsRuntimeHost/actions/runs/34841369594

[matthargett]

Follow-ups filed separately: #246 makes the JSI shim surface script exceptions from Napi::Eval as Napi::Error — once it lands, the JSRUNTIMEHOST_NAPI_ENGINE_JSI gate on NodeApi.PrimitiveExceptionSurvivesNativeCatch here can go; #244 restores the standard napi_run_script C declaration and makes the source-URL form a C++ overload, which is why this PR's test had to switch to Napi::Eval on Hermes.