Uh oh!
There was an error while loading. Please reload this page.
Add macOS unit tests - #7
Merged
Merged
Conversation
bghgary
marked this pull request as ready for review
February 1, 2023 22:01
bghgary added a commit
to bghgary/JsRuntimeHost
that referenced
this pull request
Apr 17, 2026
The Ubuntu ThreadSanitizer job on Linux was hitting the 30-minute timeout ~45-75% of runs in a silent hang (no TSan report, no output progress). Reproduced locally in WSL Ubuntu 24.04 with the exact same packages as CI. Root cause ---------- JSC's concurrent garbage collector on Linux suspends each mutator thread at GC safepoints using a SIGUSR1-based protocol: Collector Thread Mutator Thread N ---------------- ---------------- pthread_kill(N, SIGUSR1) -> (signal handler runs, sem_post) sem_wait(sem) <- (handler returns) Under ThreadSanitizer, signal delivery is intercepted and serialized. When the mutator is inside an instrumented section, TSan defers the SIGUSR1 handler indefinitely. The Collector Thread's sem_wait then blocks forever, hanging the whole process. Confirmed with a gdb capture of a hung inferior: Thread 33 "ollector Thread": BabylonJS#5 __interceptor_sem_wait BabylonJS#6 WTF::Thread::suspend(WTF::ThreadSuspendLocker const&) BabylonJS#7-BabylonJS#21 [JSC GC stop-the-world path] Thread 3 "UnitTests": <pending SIGUSR1> (never delivered) macOS JSC uses Mach thread_suspend() rather than Unix signals, which is why the macOS TSan job has been passing in ~2.7 min the whole time. Fix --- Set JSC_useConcurrentGC=0 for the Ubuntu_ThreadSanitizer job only. This removes the dedicated Collector Thread; GC runs on the mutator without any cross-thread signaling. Also revert the 30-min timeout bump — with the hang fixed the Linux TSan job should finish in roughly the same time as macOS TSan (~3 min). Verification ------------ - Default (concurrent GC on) : 9-11 hangs per 20 runs - JSC_useConcurrentGC=0 : 0 hangs per 30 runs [Created by Copilot on behalf of @bghgary] Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CedricGuillemet added a commit
to CedricGuillemet/JsRuntimeHost
that referenced
this pull request
Jun 3, 2026
Captured a full backtrace via lldb -k (see prior CI commit). The smoking gun: frame BabylonJS#5: ___BUG_IN_CLIENT_OF_LIBMALLOC_POINTER_BEING_FREED_WAS_NOT_ALLOCATED frame BabylonJS#6: napi_delete_reference at hermes_napi_reference.cpp:113 frame BabylonJS#7: Napi::Reference<...>::~Reference at napi-inl.h:3262 frame BabylonJS#8: Napi::ObjectReference::~ObjectReference frame BabylonJS#10: Babylon::Polyfills::Internal::URL::~URL at URL.h:10 frame BabylonJS#12: Napi::ObjectWrap<...URL>::FinalizeCallback at napi-inl.h:4963 frame BabylonJS#13: napi_env__::shutdown at hermes_napi.cpp:214 frame BabylonJS#16: hermes::vm::Runtime::~Runtime Root cause: Hermes's napi_env__::shutdown() iterates refListHead_ and delete ref; one at a time. It only sets ref->deletionPending_ on the *current* ref before its finalize_cb fires. If the finalizer transitively destroys a node-addon-api wrapper (Napi::Reference / Napi::ObjectReference) whose underlying napi_ref was already deleted earlier in the same loop, napi_delete_reference reads ref->deletionPending_ from freed memory and proceeds to delete ref again -> double-free. The exact path: URL (an ObjectWrap subclass) has a Napi::ObjectReference member m_searchParamsReference. addReference prepends to the linked list, so m_searchParamsReference's ref is processed BEFORE URL's wrap ref. When URL's wrap finalizer runs delete this, ~URL destroys m_searchParamsReference, whose destructor calls napi_delete_reference on the already-freed sibling ref. macOS libmalloc detects this (malloc: *** error for object 0x...: pointer being freed was not allocated -> SIGABRT). Linux glibc and Windows CRT happen to miss it. Fix: PATCH Hermes shutdown() to mark ALL refs deletionPending in a pre-pass BEFORE iterating. Apply as a FetchContent PATCH_COMMAND via the new ApplyPatchIfNeeded.cmake helper (idempotent — uses git apply --check --reverse to detect already-applied state across reconfigure). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bkaradzic-microsoft added a commit
that referenced
this pull request
Aug 20, 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
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
No description provided.