Diagnosability: route unhandled promise rejections to the host handler (#196)#204
Draft
bkaradzic-microsoft wants to merge 3 commits into
Draft
Diagnosability: route unhandled promise rejections to the host handler (#196)#204bkaradzic-microsoft wants to merge 3 commits into
bkaradzic-microsoft wants to merge 3 commits into
Conversation
A fire-and-forget rejected promise (an un-awaited fetch() that fails, or a throw inside a .then with no .catch) previously vanished silently: AppRuntime::Dispatch only catches synchronous Napi::Error throws, and no engine had a promise-rejection tracker, so the embedder's UnhandledExceptionHandler never fired and the process exited 0. - AppRuntime::Options gains opt-in EnableUnhandledPromiseRejectionTracking (default false = no behavior change). When set, unhandled rejections route into the existing UnhandledExceptionHandler. - AppRuntime::OnUnhandledPromiseRejection(const Napi::Error&) forwards to the handler; the napi_value -> Napi::Error wrapping is done per-engine (the JSI shim's napi.h has no Napi::Value/Error(napi_env, napi_value) constructor, so shared code must not do it). - V8: Isolate::SetPromiseRejectCallback, with reporting deferred via Dispatch so a rejection handled synchronously in the same turn is not reported (Node-like). - Chakra: the OS EdgeMode runtime exposes no host promise-rejection tracker (JsSetHostPromiseRejectionTracker is ChakraCore-only), so the option is a no-op here. - Native tests (skipped on non-V8 backends, including the Android JNI target which now defines JSRUNTIMEHOST_NAPI_ENGINE): a fire-and-forget rejection reaches the handler; a synchronously-handled rejection does not. JavaScriptCore and JSI trackers are follow-ups. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tness
Responds to the review on the unhandled-promise-rejection handler:
- Implement the JavaScriptCore backend in this PR (no longer a follow-up).
AppRuntime_JavaScriptCore.cpp registers a JS callback via
JSGlobalContextSetUnhandledRejectionCallback (forward-declared SPI, guarded
with __builtin_available). JSC fires it at the microtask checkpoint only for
still-unhandled rejections, so the adapter is a thin direct report with no
candidate bookkeeping.
- Factor out the engine-agnostic bookkeeping into a shared header,
AppRuntime_PromiseRejection.h: ToError() plus a PromiseRejectionTracker<>
template that collects no-handler rejections, drops them on handler-added, and
reports survivors at end-of-turn. Included only by the V8/JSC TUs (the JSI
napi.h shim lacks the napi_value -> Napi::Value bridge ToError needs).
- Always track unhandled rejections (routed to UnhandledExceptionHandler):
removed the opt-in Options::EnableUnhandledPromiseRejectionTracking flag.
- V8 robustness fixes:
* Drain candidates into a local (std::move) before reporting, so a host
handler that synchronously rejects another promise cannot invalidate the
iterator mid-flush.
* Match handler-added events by promise object identity instead of
v8::Object::GetIdentityHash (which is not unique and could drop rejections).
- Tests: replace the runtime JSRUNTIMEHOST_NAPI_ENGINE string compare with a
compile-time gate. CMake now emits JSRUNTIMEHOST_NAPI_ENGINE_<engine> (e.g.
_V8, _JavaScriptCore) for both the desktop and Android UnitTests targets, and
the rejection tests compile their body only on supported engines.
- Consolidate the coverage documentation onto AppRuntime.h
(OnUnhandledPromiseRejection): V8 and JavaScriptCore supported; Chakra
(in-box/EdgeMode) and JSI no-op. Trim the scattered Chakra note accordingly.
Validated on Win32: V8 Release -- both AppRuntime rejection tests pass and
JavaScript.All stays green (203 passing) with always-on tracking; Chakra Debug
compiles and the rejection tests skip via the compile-time gate.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The macOS CI matrix built fine, but the Linux JSC job (WebKitGTK via gcc) failed: JSGlobalContextSetUnhandledRejectionCallback is an Apple SPI absent from WebKitGTK, and __builtin_available / its iOS/macOS platform names are Clang/Apple-only, so the unguarded registration didn't compile under gcc. - AppRuntime_JavaScriptCore.cpp: wrap the rejection-callback machinery (forward declaration, thread_local context, callback, and registration) in #if __APPLE__. On non-Apple JSC (WebKitGTK/Linux, Android JSC) tracking is now a documented no-op, consistent with the existing #if __APPLE__ guard around JSGlobalContextSetInspectable. - Shared.cpp: tighten the compile-time gate to V8 || (JavaScriptCore && __APPLE__), so the rejection tests skip on non-Apple JSC where the hook is unavailable (otherwise they would hang waiting for a report). - AppRuntime.h: note that JSC support is Apple-only (WebKitGTK JSC is a no-op). Re-validated on Win32 V8 Release: AppRuntime rejection tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bkaradzic-microsoft
added a commit
that referenced
this pull request
Jul 7, 2026
#200) Implements the fetch/XHR diagnosability work tracked in #196 — making transport failures observable to the embedding app instead of opaque — plus modern `AbortSignal` support. Requires the UrlLib transport-error accessors merged in BabylonJS/UrlLib#33 (this PR bumps the UrlLib pin to `e86ffb3`). > **Note:** the unhandled-promise-rejection hop that was originally part of this PR has been **split out into #204** for its own design discussion (it's engine-conditional — V8/Apple JSC only — and needs an `#ifdef`-free rewrite). Everything remaining here is uniform across all JS engines and carries no engine/platform preprocessor guards. Each hop is a self-contained commit. --- ### Hop 2 — fetch/XHR transport-error fidelity Previously every transport failure (DNS failure, connection refused, TLS rejection, missing `app:///` asset) was flattened: `fetch()` rejected with a plain `Error{"fetch: network request failed"}` (no `cause`/`code`/`url`, stack snapshotted in a worker tick = zero user frames); XHR exposed nothing beyond `status === 0`. - `fetch` rejects with a **`TypeError`** (stable message `"fetch failed"`), detail nested under **`error.cause = {code, detail, url, status}`** (Node/undici shape, not top-level own-properties). - `cause.code`/`cause.detail` come from UrlLib; present on Apple/Linux, omitted on Windows/Android while the standard shape (`TypeError` + stable message + `cause.url/status`) is preserved everywhere — a strict, additive superset of the spec. (This variance is a UrlLib HTTP-backend difference, not a JS-engine one.) - The JS call-site **stack is captured synchronously** inside `fetch()` (via the global `Error` constructor, which materializes frames even on Chakra) and reattached to the rejection. - `XMLHttpRequest` gains additive read-only **`errorCode`/`errorDetail`** accessors; its standard `error` event + `status === 0` behavior is unchanged. ### Hop 4 — `init.signal` + modern AbortSignal `fetch()` ignored `init.signal` entirely (`arcana::cancellation::none()`), and the `AbortSignal` polyfill predated the modern spec. - `AbortSignal`: `reason` (read-only), `throwIfAborted()`, static `AbortSignal.abort(reason?)`, read-only `aborted`, and a default **`AbortError`** reason (an `Error` whose `name` is `"AbortError"`; no `DOMException` polyfill exists). `AbortController.abort(reason?)` forwards the reason. - `fetch` honors `init.signal` via its JS interface: an already-aborted signal rejects synchronously; an in-flight abort cancels the transport (`UrlRequest::Abort()`) and rejects with the signal's `reason`. --- ### Validation Built and ran the UnitTests suite on **Win32 / Chakra** and **Win32 / V8**; CI is green across the full matrix (Chakra/V8/JSC/JSI × Win32/UWP/Android/iOS/macOS/Linux, incl. sanitizers). New tests (all engine-agnostic): fetch `TypeError`+`cause` shape (refused / missing-asset), XHR `errorCode`/`errorDetail`, the modern `AbortSignal` API, and fetch `AbortError` (pre-aborted + in-flight). ### Cross-repo follow-up `UrlRequest::Abort()` only cancels the Windows backend today — making it interrupt the curl/NSURLSession transports is a separate UrlLib change (noted in #196). `AbortSignal.timeout()` is also a follow-up. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Branimir Karadžić (via Copilot) <223556219+Copilot@users.noreply.github.com>
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.
Split out of #200 so the universal fetch/XHR + AbortSignal work can land without being blocked on this feature's design discussion. Part of the diagnosability work tracked in #196.
What this does
A fire-and-forget rejected promise (an un-awaited
fetch()that rejects, or a throw in a.thenwith no.catch) currently vanishes silently —AppRuntime::Dispatchonly catches synchronous throws. This routes unhandled promise rejections into the existingUnhandledExceptionHandler(the Sentry/Bugsnag hook), matching the browserunhandledrejectionbehavior. Reporting is deferred to the end of the turn, so a rejection handled synchronously in the same turn (e.g.const p = Promise.reject(e); p.catch(...)) is not reported.Engine coverage (the reason this is split out)
Coverage depends on whether the underlying engine exposes a host promise-rejection hook:
Isolate::SetPromiseRejectCallbackJSGlobalContextSetUnhandledRejectionCallback(Apple SPI)JsSetHostPromiseRejectionTrackeris ChakraCore-onlyOpen design questions
@bghgary raised two concerns on #200 that this PR exists to resolve properly rather than rush:
Is an engine-partial diagnosability feature worth shipping? Today this is always-on where supported, silent no-op elsewhere. That's a footgun: an embedder wiring up telemetry gets async-rejection coverage on V8/Apple but silently nothing on Chakra — the default engine on the Win32/UWP path — with no signal that coverage is missing. Options to discuss:
Avoid
#ifdefs. The current implementation uses#if __APPLE__inside the shared JSC file and#if defined(<engine>)guards in the tests, which cuts against the repo convention of expressing engine/platform divergence via CMake-selected source files (AppRuntime_${ENGINE}+AppRuntime_${PLATFORM}). Planned rework before this merges:AppRuntime_${PLATFORM}mechanism) with a no-op on Unix/Linux JSC — instead of#if __APPLE__insideAppRuntime_JavaScriptCore.cpp.SupportsUnhandledRejectionTracking()) so the tests do a runtimeGTEST_SKIP()instead of a compile-time#if.Contents
Core/AppRuntime— V8 and Apple-JSC rejection callbacks routed throughOnUnhandledPromiseRejection→UnhandledExceptionHandler; Chakra/JSI documented no-ops. NewAppRuntime_PromiseRejection.hhelper.Tests/UnitTests/Shared/Shared.cpp: fire-and-forget rejection reaches the handler; a synchronously-handled rejection does not. Currently guarded to the supporting engines (to be converted to a runtime capability skip per the rework above).Status
Draft — opening for the design discussion above. The
#ifdef-free rework and the loud-no-op decision should land before this is marked ready.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com