Repository navigation
[CI twin] #189 N-API v7 across runtimes - #4
Draft
matthargett wants to merge 72 commits into
Draft
matthargett wants to merge 72 commits into
matthargett wants to merge 72 commits into
Conversation
This was referenced Jun 5, 2026
matthargett
force-pushed
the
napi-v7
branch
7 times, most recently
from
July 26, 2026 12:15
c33f9eb to
231fa40
Compare
matthargett
pushed a commit
that referenced
this pull request
Sep 1, 2026
…cope closes (BabylonJS#223) [Updated by Copilot on behalf of @bghgary] `napi_escape_handle` inserted the escaped handle at the scope start index so it would live in the parent scope, but `napi_close_escapable_handle_scope` recomputed `scope_start` from the token and called `resize(scope_start)`, freeing the very handle the close was supposed to preserve. Every caller of `napi_escape_handle` got a dangling `napi_value` back. This is reachable from ordinary code, not just direct N-API use. `Napi::ObjectReference::Get` uses an `EscapableHandleScope` and `Napi::Error::Message()` / `what()` are built on it, so reading the message of a native error on QuickJS was a heap-use-after-free. `Napi::FunctionReference::Call` and `MakeCallback` escape as well, which puts every WebSocket, `setTimeout`, `XMLHttpRequest` and `AbortSignal` callback on this path: instrumenting `napi_escape_handle` counted ~201 escapes in a single `JavaScript.All` run with no escape-specific test in scope. **How it was found.** BabylonNative [#1835](BabylonJS/BabylonNative#1835) adds tests that make a native module throw. `ExternalCallback::Callback` calls `e.what()` when there is no pending QuickJS exception, walking straight into the freed handle; its `Ubuntu_Clang_QuickJS` job segfaulted while every other engine and platform passed. ``` #0 ToJSValue js_native_api_quickjs.cc:302 #3 Napi::Error::Message #4 Napi::Error::what #5 ExternalCallback::Callback js_native_api_quickjs.cc:164 freed by: #1 napi_close_escapable_handle_scope js_native_api_quickjs.cc:1939 #2 Napi::ObjectReference::Get ``` ## The change Each open escapable scope gets a record on the env, keyed by a monotonic counter that is handed out as the opaque token. The escaped handle lives in that record until `napi_close_escapable_handle_scope` pushes it onto the handle stack, once the scope's own handles are gone; it lands at `scope_start`, in the parent scope, so it outlives the close. The token is a counter rather than a position because two escapable scopes opened with no handle allocated between them occupy the same position. Keyed on that, their escaped handles collide and the second scope to escape is refused with `napi_escape_called_twice` having never escaped. The handle stack is never modified in the middle, which matters: inserting at `scope_start` shifts every entry above it and invalidates the recorded start of any nested scope still open, reintroducing the same dangling value by a different route. A close whose recorded start is past the end of the stack now reports `napi_handle_scope_mismatch` rather than resizing, which previously grew the stack with null entries for the next close to dereference. Env teardown frees handles still held for scopes that were never closed. `napi_open_handle_scope` keeps its position-derived token: a position is all a regular scope needs, and its comment now says not to key per-scope state on it, which is the mistake the escapable version made. ## Chakra and JavaScriptCore Both returned the escapee without tracking scopes, so neither could report `napi_escape_called_twice`. Both now track open escapable scopes; values there are rooted independently of any scope, so this is the error contract only. That removes the need for `JSRUNTIMEHOST_NAPI_ESCAPE_HANDLE_IS_PASSTHROUGH`, so `SecondEscapeIsRejected` runs on every backend rather than being compiled out on two of them. ## Testing Four tests in `Tests/UnitTests/Shared/Shared.cpp`: - `EscapedHandleOutlivesItsScope` — reproduces the original `heap-use-after-free` under ASan without the fix. - `NestedEscapableScopesBothEscape` — fails on every run against the pre-fix implementation. - `SecondEscapeIsRejected` — the `napi_escape_called_twice` contract. - `AdjacentEscapableScopesEscapeIndependently` — two scopes with no handle allocated between them; confirmed to fail against the position-derived token and pass with the counter. Each test closes its escapable scopes on every exit path. Leaving one open made the enclosing `Napi::HandleScope` fail to close, and `Napi::Error::Fatal` throws from a destructor that is implicitly `noexcept`, so a failing assertion terminated the process instead of reporting `FAILED`. Verified locally at this head on Windows Release: QuickJS 10/10 and Chakra 10/10. V8, JavaScriptCore and Hermes are covered by CI. The BabylonNative #1835 end-to-end run (clang + QuickJS + RelWithDebInfo, changing only this dependency: `master` gives exit 139, 1, 139; this branch gives exit 0 × 5, clean 16/16) was made against `6238b5ab`, before the scope-identity change. --------- Co-authored-by: Branimir Karadzic <branimirkaradzic@gmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Gary Hsu <bghgary@users.noreply.github.com> Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09 Copilot-Session: c26bf58d-8462-4ea4-908d-67d366b657c5
matthargett
pushed a commit
that referenced
this pull request
Sep 1, 2026
### Problem
`napi_throw`, `napi_throw_error`, `napi_throw_type_error` and
`napi_throw_range_error` returned `napi_pending_exception` after
successfully scheduling the throw.
In Node-API that status means *"this call failed because an exception
was already pending"*, not *"a throw is now pending"*. The upstream
implementation returns `napi_clear_last_error(env)` (i.e. `napi_ok`).
Because the QuickJS port reported failure,
`Error::ThrowAsJavaScriptException` in `napi-inl.h` took its failure
branch on **every** native throw:
```cpp
napi_status status = napi_throw(_env, Value());
#ifdef NAPI_CPP_EXCEPTIONS
if (status != napi_ok) {
throw Error::New(_env); // consumes the exception that was just set
}
#endif
```
`Error::New(env)` calls `napi_get_and_clear_last_exception`, so the
pending JS exception is discarded and a fresh C++ exception is thrown
out of `details::WrapCallback`. `ExternalCallback::Callback` then
catches it, observes `!JS_HasException(ctx)`, and rebuilds the error
from `e.what()`.
By that point the `HandleScope` opened by `ThrowAsJavaScriptException`
has been destroyed during unwinding, so stringifying the message reads
freed memory.
### Impact
Two symptoms, both of which reproduce today:
1. **Wrong error surfaced to JS.** The real error is replaced by
`InternalError: Uncaught C++ exception: <message>`. Every native throw
on QuickJS is affected, so `err.name` and `err instanceof TypeError` are
wrong throughout.
2. **Use-after-free.** On Linux this segfaults. Backtrace from a
BabylonNative CI core dump:
```
#0 js_dup quickjs.c:1628 <-- SIGSEGV
#1 js_force_tostring quickjs.c:4813
#3 JS_ToCStringLen
#4 napi_get_value_string_utf8 js_native_api_quickjs.cc:696
#5 Napi::String::Utf8Value napi-inl.h:1118
#7 Napi::Error::Message napi-inl.h:3087
#8 Napi::Error::what napi-inl.h:3157
#9 ExternalCallback::Callback js_native_api_quickjs.cc:164
```
The `JSValue` being stringified carries `JS_TAG_STRING` with an
unaligned, freed pointer.
I instrumented the `catch` in `ExternalCallback::Callback` in a
BabylonNative QuickJS build and confirmed that **all ~50 native throws**
in that test run escaped `WrapCallback` with `hasExc=0`. After this
change the count is 0.
### Fix
Return `napi_ok` from the four throw entry points, matching upstream.
The exception stays pending, `WrapCallback` returns normally, and the
fragile `e.what()` fallback is never entered.
### Test
Added a strict assertion to the existing `URLSearchParams.set()` arity
throw, checking the error type and exact message rather than a
substring. The pre-existing `.to.throw()` test could not catch this,
because `"Uncaught C++ exception: <msg>"` still *contains* the expected
substring.
Verified on Linux QuickJS (RelWithDebInfo):
| | result |
|---|---|
| without the C++ change | `expected 'InternalError' to equal 'Error'` —
212 passing, **1 failing** |
| with the C++ change | **213 passing**, 10/10 gtest |
Also verified in a BabylonNative QuickJS build on Windows: 21/21 gtest,
49 JS assertions, exit 0, and zero escapes from `WrapCallback`.
Co-authored-by: Branimir Karadzic <branimirkaradzic@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09
matthargett
force-pushed
the
napi-v7
branch
7 times, most recently
from
September 1, 2026 03:50
e813292 to
434fd79
Compare
…JS#232) V8 finishes asynchronous WebAssembly compilation on a background thread and then posts a **foreground** task to settle the promise on the isolate thread. `AppRuntime_JSI` already ran those via `TaskRunnerAdapter`; the direct-V8 path dropped them, so asynchronous WebAssembly promises could hang. Synchronous `new WebAssembly.Module` was unaffected. Foreground posts now go through `V8ForegroundTaskRunner`, which dispatches `v8::Task::Run` through AppRuntime. Delayed posts use `DelayedTaskScheduler`, extracted into Foundation from the timer queue behind `setTimeout` / `setInterval`. AppRuntime registers its scheduler with the environment for the polyfill to borrow; standalone `JsRuntime` instances retain their original API and do not acquire timer ownership. `TimeoutDispatcher` owns a fallback scheduler when no host scheduler is registered. Interval rearming remains on the JavaScript thread after the callback. Delayed V8 posts round their absolute requested time upward to microsecond precision, and completed pending records are removed even when completion precedes `Schedule` returning. Shared callback state and explicit host unregistration guard teardown independently of retained task-runner references. `V8Platform` and the foreground runner are in dedicated files; the wake-and-pump bridge and `PumpMessageLoop` are gone. Added asynchronous WebAssembly promise-settlement tests plus native timing, pending-record, cancellation, scheduler-ownership, and shutdown regressions. Validation for 7192ac0 (Windows Release): V8 11.9 passed 17 selected native tests, including the JavaScript suite with 228 passing; Chakra passed 9 selected native tests, including 225 JavaScript tests with the 3 V8-only tests skipped. The 16 native timer/lifetime tests also passed 25 repeated iterations. The V8 implementation files syntax-compile against the actual Android V8 11.0 headers; Android device execution remains for CI. --------- Co-authored-by: Branimir Karadzic <branimirkaradzic@gmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09 Copilot-Session: da74bc94-a7dc-4817-bd81-59b5c6b123fc Copilot-Session: 54aaa1e1-b4b3-46e7-9de4-5a56add4ac42
…ction when the receiver was undefined, so the VM forcibly substituted the global object even in strict mode. The new implementation always routes through Function.prototype.call, preserving the exact thisArg. This only affected JSC: Chakra already pushes recv onto the argv array before invoking JsCallFunction, and V8 hands the raw recv value to Function::Call. Neither engine coerces in strict mode, so no additional fixes were required.
…bug fix in strict mode was actually found by the suite! The failing behavior was exercised by Tests/NodeApi/test/js-native-api/3_callbacks/test.js. New cmake targets emit a node-lite binary and a NodeApiTests binary, all currently enabled tests for currently supported NAPI v5 pass on Mac. Next step is to enable them for running in Android simulator.
…x handles during instrumentation, causing crashes, and now route console output through the new NodeLiteRuntime::Callbacks. On Android we forward stdout/stderr to logcat via callbacks to work around this for now. Added Android-specific shims (node_lite_android.cpp, child_process_android.cpp) so native module loading uses dlopen and JS child_process.spawnSync safely reports “unsupported”. Extended the Node‑API harness to allow in-process execution: RunNodeLiteScript captures output, SetNodeApiTestEnvironment lets the JNI layer provide a base directory and asset manager, and the GTest registration path uses that configuration instead of shelling out to the node_lite executable
…-- a use-after-free. Will check sanitizers under Android next
…ned into our copy of the JSI code.
… since they found bugs on macOS + JSC
BigInt: create via the macOS-15+ C API (JSBigIntCreateWith*) with a JS-BigInt-global eval fallback for jsc-android/older; extract (get_value_*) via BigInt.asIntN/asUintN + toString round-trips; typeof reports napi_bigint (kJSTypeBigInt on macOS 15+); create_bigint_words enforces the Node INT_MAX / RangeError size limits before reading the words buffer. Detach: ArrayBuffer.prototype.transfer() (ES2024) with .detached for is_detached; ENOTSUP throw where transfer() is absent. test_bigint green on JSC/macOS.
…cate) macOS 15+/iOS 18+ use the kJSTypeBigInt fast path; older JSC (jsc-android ~2020) does not surface kJSTypeBigInt through the C API, so fall back to a cached `typeof v === 'bigint'` predicate created at env init. Keeps the v6 BigInt conformance test green on the jsc-android target.
Chakra: add napi_set/get_instance_data (per-env slot, finalized at env teardown by ~napi_env__) and the BigInt create/get functions as feature-detection stubs that throw a JS-catchable ENOTSUP error -- the Win10 OS edge-mode Chakra predates BigInt and exposes no JsBigInt* API, so there is no value-preserving fallback. JSI (Core/Node-API-JSI) does not implement the v6/v7 C surface, so its conformance addons are gated to the v1-v5 set. V8 + JavaScriptCore execute the v6/v7 tests for real; Chakra links them.
…him not green on-device) test_instance_data (v6) runs on both Android engines; BigInt runs for real on V8. On jsc-android the eval-based BigInt shim fails on-device with no error detail from the in-process runner (macOS JSC + V8 both pass test_bigint), so it's gated to V8 and tracked as a follow-up.
…jsc-android ~2020) jsc-android (~2020) ships without BigInt -- its parser even rejects `0n` literals -- so the eval-based BigInt shim can't work there. Detect BigInt once at env init (`typeof BigInt === 'function' && typeof BigInt(1) === 'bigint'`, which parses on every JSC) and have the create/get paths throw a JS-catchable ENOTSUP when it's absent, matching the Chakra backend. macOS/iOS JSC (which have BigInt) are unaffected.
…e BigInt by engine support The standard test_bigint uses `0n` literals, which don't parse on engines without BigInt (jsc-android ~2020, Win10 Chakra). Add a feature-detection fallback that asserts napi_create_bigint_int64 throws ENOTSUP, and gate it to run exactly where BigInt is absent (Chakra; jsc-android) while the real test_bigint runs on V8 + Apple JSC. Keeps the desktop + Android run-lists in sync.
The in-process runner keeps the assertion/exception message + stack in ProcessResult.std_error, which never reached the device log -- making on-device conformance failures undebuggable (a 0 ms FAILED with no detail). Log it on non-zero status. This is what surfaced the jsc-android BigInt SyntaxError.
The addons were enabled for every engine except JSI, so the backends upstream has added since (QuickJS, Hermes) picked up addons whose napi_* symbols they do not export and failed at link -- QuickJS has napi_create_bigint_int64 but no napi_create_bigint_words / napi_get_value_bigint_words, and no napi_add_finalizer (test_instance_data). Switch to an allow-list of the engines whose v6/v7 surface is implemented and which the suite has actually been run against (V8, JavaScriptCore, Chakra), so a newly added backend is opted in deliberately rather than by default.
…e suite The Android app carries its own NODE_API_AVAILABLE_NATIVE_TESTS list, which gave every non-V8 engine test_instance_data plus the ENOTSUP BigInt fallback. On QuickJS that is wrong twice over: it has no napi_add_finalizer (test_instance_data), and it *does* have BigInt, so the ENOTSUP fallback fails. Restrict the v6/v7 suites to V8 + JavaScriptCore, matching Tests/NodeApi/CMakeLists.txt.
The NODE_API_AVAILABLE_NATIVE_TESTS list is only meaningful when the in-process harness sources are actually compiled, which is now gated to the validated engines (V8, JavaScriptCore).
…e conformance addons `if(JSR_NODE_API_HARNESS_SRC)` guarded the NODE_API_AVAILABLE_NATIVE_TESTS definition, but the variable stopped being set once the harness sources became unconditional (the engine-specific `#ifdef`s they needed are now JSR_NAPI_ENGINE_* compile definitions, so they build for every engine). The guard was therefore always false: the define was never emitted, and Shared.cpp ran no Node-API conformance addons at all on device. Both Android jobs were green because the suite was empty, not because it passed. Gate on the engine allow-list directly instead. Android now runs the addons again: 20 tests / 18 passed / 2 intentional skips on both V8 and JavaScriptCore.
…s BigInt
jsc-android's `latest` tag is still 250231.0.0 (WebKit r250230, Sept 2019), which
compiles BigInt behind the `useBigInt` runtime option, defaulted off -- so its
parser rejects `0n` and the standard test_bigint cannot even be parsed. The `next`
tag, 294992.0.0 (WebKit r294992, July 2022), is the newest published build and has
BigInt enabled by default.
Verified on-device (arm64-v8a, API 29) rather than inferred:
r250231 r294992
typeof BigInt undefined function
typeof BigInt(1) ReferenceError bigint
typeof 0n SyntaxError bigint
BigInt.asIntN / asUintN ReferenceError function / function
BigInt.asUintN(64, -1n) SyntaxError 18446744073709551615
BigInt64Array undefined function
ArrayBuffer.prototype.transfer undefined undefined
structuredClone undefined undefined
So Android JavaScriptCore now runs the real test_bigint instead of the ENOTSUP
test_bigint_unsupported fallback; Chakra keeps the fallback. napi_detach_arraybuffer
stays feature-detected/ENOTSUP on Android: neither build has
ArrayBuffer.prototype.transfer, which is the only public detach path, and that is a
hard limit of the newest jsc-android.
Two mechanical consequences of the bump: r294992 publishes only the `-intl`
variant, so the AAR path changes, and it statically links libc++ (no
libc++_shared.so DT_NEEDED, unlike r250231), removing an ABI coupling.
Full on-device suite, both engines: 20 tests, 18 passed, 2 intentional skips, 0 failures.
…d at env init
Two independent problems in the same code, both about reaching JS intrinsics from
the C entry points.
1. Every BigInt entry point resolved its intrinsics on the live global object, once
per call: BigIntLow64 re-read `BigInt` and `BigInt.asIntN`/`asUintN`,
BigIntToString read `toString` off the boxed value (so through
BigInt.prototype), and the create path evaluated a `BigInt("...")` source
string. napi_detach_arraybuffer and napi_is_detached_arraybuffer did the same
with `transfer` and `detached`. All of those are reachable from script, so a
page could replace them and steer an addon's napi_*_bigint_* results -- the
invariant BabylonJS#116 established for napi_call_function with a cached
Function.prototype.call, not carried through to these.
Capture them once at env construction instead, before any user script can run:
the constructor, asIntN/asUintN, BigInt.prototype.toString, a `(v) => -v`
helper (StringToBigInt accepts a sign only on decimal literals, so the hex
words path cannot spell a negative value), ArrayBuffer.prototype.transfer, and
the `detached` getter out of its property descriptor. The create path now calls
the captured constructor rather than evaluating source, which also drops a
parse per BigInt created.
Capturing by property lookup doubles as the feature probe, replacing the eval
probe: on an engine without BigInt the lookups just fail. That matters because
a probe containing a `0n` literal is a SyntaxError -- not a false result -- on
jsc-android r250231.
NodeApi.BigIntIgnoresMonkeyPatchedIntrinsics covers this. Verified it fails
against the old lookup (returns the patched 1234, lossless false) and passes
with the fix, on both the C API and the fallback path.
2. The BigInt C API guards were wrong on two axes, verified by compiling against
each SDK rather than reading the annotations:
* `#if defined(__MAC_OS_X_VERSION_MAX_ALLOWED) && >= 150000` is false on every
non-macOS Apple SDK, so the whole fast path was compiled out of iOS and
visionOS builds -- including iOS 18.3 and visionOS 26.1, where the API is
available. visionOS additionally reports __IPHONE_OS_VERSION_MAX_ALLOWED as
17.0, so it needs a clause of its own.
* `__builtin_available(macOS 15.0, *)` does not guard iOS or visionOS at all:
the `*` asserts availability on unlisted platforms. Had the `#if` been
"fixed" without this, an iOS 17 deployment would have called a null weak
symbol. clang confirms it -- the call still warns as unguarded under that
form at iOS 17 / visionOS 1.
Both are replaced by one JSR_JSC_HAS_BIGINT_C_API macro pair naming every
platform. Compiles clean and takes the intended path at macOS 13/15,
iOS 17/18.3, visionOS 1/26.1 and tvOS 18.
Also makes deinit_symbol null-safe: the intrinsics are null on an engine without
BigInt, and it unprotected unconditionally.
Verified: macOS JavaScriptCore 13/13 UnitTests and 15/16 conformance (1 quarantined),
on both the C API path and with it forced off; Android arm64 JavaScriptCore and V8
21 tests / 19 passed / 2 intentional skips each.
js_native_api.h and js_native_api_types.h already default NAPI_VERSION to 7 on this branch, but napi.h still pinned 5 -- and because all three use `#ifndef`, whichever is reached first wins. Every consumer includes <napi/napi.h> (all of Babylon Native, and every node-addon-api addon), so 5 won essentially everywhere: Napi::BigInt, Napi::BigInt64Array, Value::IsBigInt(), GetInstanceData<T>() and Napi::Addon stayed compiled out even though the C entry points behind them are implemented and pass conformance. Found because a test in Tests/UnitTests could not name napi_create_bigint_int64: that translation unit was seeing NAPI_VERSION=5 through napi.h, not the 7 the C headers declare. Note for BabylonJS#229: this makes the three headers agree, it does not decide the policy question. If NAPI_VERSION goes back to 5 with features ungated individually, all three move together.
Dropping the platform #ifdef around the `typeof v === 'bigint'` fallback left it running in napi_typeof's default branch on every engine -- so every napi_typeof of an ordinary object paid a JS call before reaching the object handling, on a very hot path. Probe once at env init whether JSValueGetType actually classifies a BigInt on this engine and OS, and consult the predicate only when it does not. That is also strictly better than gating on the SDK or __builtin_available: the enumerator can be compiled in while the JSC actually deployed never produces it, and the probe asks the engine instead of assuming. macOS JavaScriptCore 13/13 + 15/16 conformance; Android JavaScriptCore 21 tests / 19 passed / 2 intentional skips.
It had no coverage at all: the upstream conformance test for detach lives in test_typedarray, which needs v9's node_api_basic_env and so cannot be enabled yet. Detach is not uniform across the engines here -- ArrayBuffer.prototype.transfer() (ES2024) is the only public detach path, since the JavaScriptCore C API has no detach entry point -- so the test asserts whichever half of the contract applies: the buffer really detaches, or the failure is a catchable JS error carrying code ENOTSUP rather than a bare napi_status an addon cannot tell from a real error. Both halves are exercised by the matrix: macOS JavaScriptCore and V8 take the detach path, Android JavaScriptCore takes ENOTSUP (neither jsc-android r250231 nor r294992 has transfer -- verified on device). macOS 14/14; Android JavaScriptCore 22 tests / 20 passed / 2 intentional skips.
The body was commented out and replaced with a bare `*result = true;` in BabylonJS#70 ("Update Node-API to latest from node.js"), so the API answered "detached" for every value handed to it, including a freshly created, perfectly live ArrayBuffer. Anything branching on it -- an addon guarding a zero-copy path, or a detach-then-verify sequence -- got the wrong answer with no error to notice. The pinned V8 does declare ArrayBuffer::WasDetached() (v8-array-buffer.h), so the commented-out implementation compiles as written; restore it. Caught by NodeApi.DetachArrayBufferOrReportsUnsupported on Android_V8 and Win32_x64_V8. Also record the napi_status of each napi_is_detached_arraybuffer call in that test, so an engine that fails the query is not mistaken for one that answered "detached". Android arm64 V8: 22 tests / 20 passed / 2 intentional skips.
…ead of after waitpid Both SpawnSync implementations waited for the child to exit before reading either pipe. A child that writes more than one pipe buffer (16 KiB on macOS, 64 KiB on Linux, 4 KiB for a default Windows anonymous pipe) blocks on write while the parent blocks on wait -- a deadlock that a chatty conformance test can reach. POSIX: close the write ends, then poll() both read ends and service whichever has data until both hit EOF, then waitpid. Windows: the parent's copies of the write handles were only closed after the wait, so ReadFile could see EOF only because the child was already gone; close them immediately after CreateProcess, drain each pipe on its own thread, then wait for the exit code. Addresses the review on BabylonJS#116.
napi_detach_arraybuffer and napi_is_detached_arraybuffer were absent from the Chakra backend entirely, so linking anything that referenced them failed -- which is how this surfaced (Win32_x64_Chakra and Win32_x86_Chakra failed at Build Solution once a test called them). Win10's OS Chakra has no way to detach an ArrayBuffer: the jsrt runtime exposes no detach entry point, and the engine predates ES2024 ArrayBuffer.prototype.transfer. So napi_detach_arraybuffer follows the same feature-detection-by-exception pattern as BigInt here -- a JS-catchable error tagged ENOTSUP -- rather than a bare status an addon cannot distinguish from a real failure. napi_is_detached_arraybuffer can be answered truthfully rather than stubbed: since nothing can detach a buffer on this engine, an ArrayBuffer that still reports storage through JsGetArrayBufferStorage is live, and a non-ArrayBuffer is not detached either (matching Node's contract).
matthargett
force-pushed
the
napi-v7
branch
from
September 13, 2026 07:09
5cb1587 to
3b38a98
Compare
## Summary Follow-up to BabylonJS#230 (comment). The UTF-16 decoder previously dropped a trailing odd byte and created JavaScript strings from unpaired surrogates. Replacement mode in the Encoding Standard requires U+FFFD for both, in either endianness. `DecodeUtf16` now: - emits U+FFFD for a leftover odd byte - emits U+FFFD for a lone lead or trail surrogate - treats a lead followed by a non-trail as one replacement, then reprocesses the second unit - treats an unpaired lead plus a leftover odd byte as a single end-of-queue replacement The README describes this behavior and the still-unsupported `fatal` option. ## Timer cleanup after rebase Correction to the original description, per [Gary's review](BabylonJS#235 (review)): `Dispatch` always passed `0`, so the `id == 0` branch was **always taken**. The **nonzero ID-reuse path** was dead. The latest `main` already includes the timer refactor from BabylonJS#232, which removes `DispatchImpl` and guards against duplicate-ID insertion. The rebase keeps that implementation unchanged and drops this PR's superseded timer changes. The remaining diff contains only TextDecoder, its documentation, and the UTF-16 regression cases. ## Regression coverage - Trailing odd byte, both endiannesses - Lone lead / lone trail, both endiannesses - Lead + BMP (`U+D800` then `'A'`) produces `"\uFFFDA"` - Lead + leftover odd byte produces a single `"\uFFFD"` Existing valid-pair and BOM cases are unchanged. --------- Co-authored-by: Branimir Karadzic <branimirkaradzic@gmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da74bc94-a7dc-4817-bd81-59b5c6b123fc
## Context `build-linux.yml` set job-level `CC`/`CXX` env alongside the `-D CMAKE_C_COMPILER` / `-D CMAKE_CXX_COMPILER` configure flags. The `-D` flags already select the compiler for both the gcc and clang matrix legs, so the env block was redundant. No behavior change: on the parallel UrlLib CI cleanup both the gcc and clang Linux jobs stayed green with the env removed. The Run Tests step's `env` (`TSAN_OPTIONS` / `JSC_useConcurrentGC`) is untouched. Part of a small cross-repo consistency pass — UrlLib landed the same cleanup. BabylonNative was checked and intentionally left alone: its `build-linux.yml` env is load-bearing there (its configure passes no `-D` compiler flags). [Created by Copilot on behalf of @bghgary] Co-authored-by: Gary Hsu <223556219+Copilot@users.noreply.github.com>
[Created by Copilot on behalf of @bghgary] Native and JavaScript runtime tests for unrelated features were grouped together, making coverage hard to navigate. This change groups the existing tests by feature and packages the generated test bundle and fixtures together under `Assets`. The same tests continue to run through the existing runner. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Upstream BabylonJS#257 reorganized the unit tests by feature: Tests/UnitTests/{Android,Shared,Scripts} moved under Tests/UnitTests/Source, Shared.cpp was split into Tests.*.cpp, and RunTests() now lives in Source/App.cpp. Re-homed this PR's Android Node-API harness accordingly: - Source/App.cpp: the in-process harness bootstrap (asset manifest copy, JNI files dir, ConfigureNodeApiTests/RegisterNodeApiTests around RunTests, SetNodeApiTestEnvironment) moves here from Shared.cpp; Source/App.h already carried the declaration. - Source/Tests.NodeApi.cpp: upstream's split file as-is (the native NodeApi tests were unchanged by this branch). - Source/Android/app/src/main/cpp/CMakeLists.txt: upstream's per-feature source list plus the JSR_NODE_API_HARNESS_SRC sources. - Source/Android/app/build.gradle: upstream replaced copyScripts with per-variant test-bundle tasks and cleared the main asset set, so the main set now holds only the generated Node-API assets; copyNodeApiTests points one level deeper at Tests/NodeApi/test and, like the bundle tasks, is a dependency of the lint tasks that read assets independently. - Source/Android/tools/wrap.sh: moved with the Android project (build.gradle references it relative to the app project).
Brings in upstream main through the BabylonJS#116 branch so this PR keeps stacking on it. Upstream BabylonJS#257 reorganized the unit tests by feature, so the two things this branch had added to Tests/UnitTests/Shared/Shared.cpp move with it: - Tests.NodeApi.cpp: BigIntIgnoresMonkeyPatchedIntrinsics and DetachArrayBufferOrReportsUnsupported (with the ScriptLoader include they use). - App.cpp: the on-device logcat detail for in-process node_lite failures, inside the Android harness bootstrap that BabylonJS#116 moved here. The Android CMakeLists keeps upstream's per-feature source list and this branch's unconditional harness sources (the engine allow-list only gates the NODE_API_AVAILABLE_NATIVE_TESTS define).
matthargett
changed the title
…abylonJS#218) [Updated by Copilot on behalf of @bghgary] Fixes BabylonJS#216. ## Context `napi_get_property_names` should return enumerable string-keyed properties across the prototype chain, matching `for...in`. Before this PR, JavaScriptCore threw, Chakra and QuickJS returned only own properties, and JSI had an unimplemented stub. The three Node-API backends now share a property-name walk; JSI uses its own implementation. The walk must account for non-enumerable own properties hiding inherited names, terminate when a Proxy returns a repeated prototype, and keep the returned status consistent with `napi_get_last_error_info`. JavaScriptCore’s `napi_get_prototype` also needed correction for primitives and the null end of a prototype chain. ## Review follow-up A retained JavaScriptCore context could collect reference sentinels after their environment was deleted, causing a use-after-free. This branch addresses that lifetime defect as well as the conformance changes. ## Coverage and limits The feature-named unit tests cover own and inherited names, shadowing, symbols, arrays, prototype termination, error reporting, and reference lifetime. Proxy-dependent cases are skipped on JavaScriptCore because its prototype lookup does not invoke the Proxy trap. Hermes’ `for...in` shadowing discrepancy remains tracked in [BabylonJS#219](BabylonJS#219); `napi_get_all_property_names` remains V8-only. --------- Co-authored-by: Branimir Karadzic <branimirkaradzic@gmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Branimir Karadzic <branimirkaradzic@Bgmail.com> Co-authored-by: Gary Hsu <bghgary@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae Copilot-Session: 79ad319b-72f9-4b93-9c81-57858177c0a7 Copilot-Session: af4ea82e-5fb0-4e56-b71a-f8ff3932d033 Copilot-Session: 54aaa1e1-b4b3-46e7-9de4-5a56add4ac42 Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305 Copilot-Session: fc6f260a-f58d-49ed-9bed-d69248708138
BabylonJS#218 moved the JavaScriptCore reference tracking into a shared state that the sentinel finalizer holds weakly, so a finalizer that runs after the environment is gone simply finds no state. That covers the case this branch guarded with the shutting_down check inside the finalizer; the upstream lambda is kept and the flag stays for the destructor's own use.
Only the include block overlapped: BabylonJS#218's shared property-name helper include joins this branch's BigInt C API availability guard at the top of the JavaScriptCore backend.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CI twin of BabylonJS#189 (not for review here; the discussion is upstream). Re-synced 2026-09-25 to upstream main @ 62818ee through BabylonJS#116's head (631b745), adopting BabylonJS#257's per-feature test layout; 24/24 green at 3b38a98 before the re-sync.