Skip to content

Debuggability: fetch/XHR error fidelity, unhandled-rejection handler, and AbortSignal (#196)#200

Open
bkaradzic-microsoft wants to merge 6 commits into
BabylonJS:mainfrom
bkaradzic-microsoft:hop2-fetch-error-fidelity
Open

Debuggability: fetch/XHR error fidelity, unhandled-rejection handler, and AbortSignal (#196)#200
bkaradzic-microsoft wants to merge 6 commits into
BabylonJS:mainfrom
bkaradzic-microsoft:hop2-fetch-error-fidelity

Conversation

@bkaradzic-microsoft

@bkaradzic-microsoft bkaradzic-microsoft commented Jun 16, 2026

Copy link
Copy Markdown
Member

Implements the diagnosability work tracked in #196 — making fetch()/XMLHttpRequest transport failures, unhandled promise rejections, and aborts observable to the embedding app instead of opaque. Originally split across #200/#201/#202; consolidated here into one PR since it's a single feature in one repo (and hops 2 and 4 both touch Fetch.cpp).

Each hop is a self-contained commit. Requires the UrlLib transport-error accessors merged in BabylonJS/UrlLib#33 (this PR bumps the UrlLib pin to e86ffb3).


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.
  • 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 3 — unhandled promise rejection → host handler

A fire-and-forget rejected promise (un-awaited fetch() failure, or a throw in a .then with no .catch) vanished silently: AppRuntime::Dispatch only catches synchronous throws.

  • Opt-in AppRuntime::Options::EnableUnhandledPromiseRejectionTracking (default false). When set, unhandled rejections route into the existing UnhandledExceptionHandler (the Sentry/Bugsnag hook).
  • V8: Isolate::SetPromiseRejectCallback, with reporting deferred via Dispatch so a synchronously-handled rejection 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 documented no-op. JSC/JSI are follow-ups.

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 (203 mocha tests + native tests) and Win32 / V8; CI is green across the full matrix (Chakra/V8/JSC/JSI × Win32/UWP/Android/iOS/macOS/Linux, incl. sanitizers).

New tests: fetch TypeError+cause shape (refused / missing-asset), XHR errorCode/errorDetail, the modern AbortSignal API, fetch AbortError (pre-aborted + in-flight), and native unhandled-rejection tests (V8-gated; skipped on engines without a tracker).

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() and JSC/JSI rejection trackers are also follow-ups.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Previously every fetch()/XMLHttpRequest transport failure (DNS failure,
connection refused, TLS failure, missing local asset, ...) was flattened to a
single opaque signal -- fetch rejected with `std::runtime_error{"fetch: network
request failed"}` (a plain Error with no cause/code/url and a stack snapshotted
in a worker-thread scheduler tick, i.e. zero user frames), and XHR exposed
nothing beyond status 0. Field crash reports could not tell the failure classes
apart. This is hop 2 of BabylonJS#196, now unblocked by the
UrlLib transport-error accessors (BabylonJS/UrlLib#33).

fetch:
- Reject with a TypeError whose message is the stable string "fetch failed"
  (kept constant so crash-report grouping stays intact), carrying the variable
  detail on `error.cause = { code, detail, url, status }` -- the Node/undici
  shape -- rather than as non-standard top-level own-properties.
- `cause.code`/`cause.detail` come from UrlLib's ErrorSymbol()/ErrorString();
  they are present on backends that populate them (Apple, Linux) and omitted on
  those that do not yet (Windows, Android), while the standard observable shape
  (TypeError + stable message + cause.url/status) is preserved everywhere. This
  is a strict, additive superset of the spec: spec-conformant code sees only a
  TypeError, as in a browser; BN-aware code can distinguish failure classes.
- Capture the JS call-site stack synchronously inside fetch(), before SendAsync()
  hands off to a worker thread, and reattach it to the rejection so crash reports
  attribute the failing call. Uses the global JS Error constructor so engines
  that materialize .stack from that path (incl. Chakra) capture live frames.

XMLHttpRequest:
- Add non-standard, additive read-only `errorCode`/`errorDetail` accessors that
  expose the same UrlLib detail; the standard error event + status===0 behavior
  is unchanged.

Also bump the UrlLib pin to e86ffb3 (BabylonJS#33) so the accessors are available, add
JS tests for the rejection shape and XHR diagnostics, and document both in the
polyfill Readmes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 16, 2026 19:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves diagnosability of transport-level failures in the fetch() and XMLHttpRequest polyfills by preserving structured error detail (from UrlLib) and capturing a synchronous call-site stack before work hops to a worker thread.

Changes:

  • fetch() now rejects transport failures as TypeError("fetch failed") with a structured error.cause object and an optionally reattached call-site stack.
  • XMLHttpRequest gains additive errorCode / errorDetail accessors for transport-failure diagnostics.
  • UrlLib pin is bumped to include transport-error accessor support; unit tests and READMEs are updated accordingly.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
Tests/UnitTests/Scripts/tests.ts Adds unit tests asserting the new fetch TypeError + cause shape and XHR errorCode/errorDetail behavior.
Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h Declares new errorCode / errorDetail accessors.
Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp Implements the new diagnostics accessors and wires them as instance properties.
Polyfills/XMLHttpRequest/Readme.md Documents the new XHR transport-error diagnostics.
Polyfills/Fetch/Source/Fetch.cpp Implements fetch failed TypeError rejections with cause and synchronous stack capture/reattachment.
Polyfills/Fetch/Readme.md Documents the new fetch transport-failure rejection shape and stack behavior.
CMakeLists.txt Updates UrlLib dependency pin to the commit providing transport-error accessors.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread Polyfills/Fetch/Source/Fetch.cpp
Comment thread Polyfills/XMLHttpRequest/Readme.md
@bkaradzic-microsoft bkaradzic-microsoft marked this pull request as draft June 16, 2026 19:58
…or event

- CaptureCallSiteStack: detect the global Error constructor via IsUndefined()/IsNull() instead of IsFunction(), matching the repo pattern (some JavaScriptCore/JSI builds report constructors as typeof 'object', which would silently disable synchronous stack capture).

- XMLHttpRequest Readme: add the 'error' event to the supported-events list, since the implementation and tests rely on it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI added 2 commits June 16, 2026 16:18
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>
fetch() previously ignored init.signal entirely (it passed
arcana::cancellation::none() to both continuations), so AbortController could
never cancel a request or reject with AbortError, and the AbortSignal polyfill
predated the modern spec. This is hop 4 of BabylonJS#196.

AbortSignal:
- Add `reason` (read-only) and `throwIfAborted()`.
- Add static `AbortSignal.abort(reason?)`.
- Make `aborted` read-only (remove the setter).
- Abort(reason) now records the reason, defaulting to an AbortError (an Error
  whose name is "AbortError", as there is no DOMException polyfill).
- AbortController.abort(reason) forwards the reason to the signal.

fetch:
- Honor init.signal via its JS interface (so fetch stays decoupled from the
  AbortController C++ types): an already-aborted signal rejects synchronously
  with the signal's reason; otherwise an "abort" listener cancels the transport
  (UrlRequest::Abort()) and the completion continuation rejects with the
  signal's reason (an AbortError) instead of a network-error TypeError. The
  listener is torn down when the request settles, breaking the ownership cycle.

Transport cancellation is effective on backends where UrlRequest::Abort() is
implemented (Windows today; the curl/NSURLSession backends are a UrlLib
follow-up noted in BabylonJS#196). AbortSignal.timeout() is left as a follow-up.

Adds JS tests for the modern AbortSignal API and for fetch rejecting with an
AbortError both pre-aborted and in-flight (validated on Win32, where the UrlLib
backend honors Abort()).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bkaradzic-microsoft bkaradzic-microsoft changed the title fetch/XHR: surface transport-failure detail (TypeError + cause + sync stack) — hop 2 of #196 Debuggability: fetch/XHR error fidelity, unhandled-rejection handler, and AbortSignal (#196) Jun 16, 2026
@bkaradzic-microsoft bkaradzic-microsoft marked this pull request as ready for review June 18, 2026 15:12

@bghgary bghgary left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Reviewed by Copilot on behalf of @bghgary]

Comments inline. Main asks: implement JSC in this PR (it has the API, CI covers it); drop the opt-in flag — always track, routing to the existing UnhandledExceptionHandler; and make the two test-gating mechanisms consistent (compile-time #if defined, not a runtime engine-name string compare). Plus two minor V8-tracker robustness notes.

Comment on lines +114 to +119
for (auto& entry : tracker.unhandled)
{
const v8::Local<v8::Value> reason = entry.second.second.Get(tracker.isolate);
tracker.runtime->OnUnhandledPromiseRejection(ToError(env, JsValueFromV8LocalValue(reason)));
}
tracker.unhandled.clear();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a host handler ever rejects a promise synchronously, OnPromiseReject would insert into tracker.unhandled mid-iteration (rehash → invalidated iterator). Drain into a local first:

Suggested change
for (auto& entry : tracker.unhandled)
{
const v8::Local<v8::Value> reason = entry.second.second.Get(tracker.isolate);
tracker.runtime->OnUnhandledPromiseRejection(ToError(env, JsValueFromV8LocalValue(reason)));
}
tracker.unhandled.clear();
auto unhandled = std::move(tracker.unhandled);
tracker.unhandled.clear();
for (auto& entry : unhandled)
{
const v8::Local<v8::Value> reason = entry.second.second.Get(tracker.isolate);
tracker.runtime->OnUnhandledPromiseRejection(ToError(env, JsValueFromV8LocalValue(reason)));
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 97e2c68. The deferred flush now moves the candidate list into a local before reporting (const std::vector<CandidateT> candidates = std::move(m_candidates); m_candidates.clear(); in PromiseRejectionTracker::Flush), so a host handler that synchronously rejects another promise re-enters Add() against the fresh, empty member vector and can no longer invalidate the in-progress iteration. The bookkeeping now lives in the shared AppRuntime_PromiseRejection.h.


v8::HandleScope handleScope{tracker->isolate};
const v8::Local<v8::Promise> promise{message.GetPromise()};
const int hash{promise->GetIdentityHash()};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetIdentityHash() isn't unique: two concurrently-unhandled promises can collide on hash, so the second overwrites the first's entry and a handler later added to either erases the shared slot — a still-unhandled rejection can be silently dropped. Low-probability on this diagnostics path, but the keying is lossy.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 97e2c68. Candidates are now held in a std::vector and matched by promise object identity rather than a hash key: kPromiseHandlerAddedAfterReject removes via candidate.promise.Get(isolate) == promise, so colliding GetIdentityHash() values can no longer overwrite/drop a still-unhandled rejection. GetIdentityHash() is gone from this path.

Comment on lines +29 to +30
// `const p = Promise.reject(e); p.catch(...)`) does not reach the handler. Implemented for
// the Chakra and V8 engines.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop this flag and always track — route unhandled rejections to the existing UnhandledExceptionHandler (defaults to the benign logger), matching browser behavior. Document coverage once here, accurately: fires on V8 and JSC; a no-op on Chakra (in-box) and JSI — neither exposes a host rejection hook (JsSetHostPromiseRejectionTracker is ChakraCore-only; neither jsi::Runtime nor V8JSI exposes the V8 callback). The current "Chakra and V8" is wrong.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 97e2c68. Removed Options::EnableUnhandledPromiseRejectionTracking; unhandled rejections are now always routed to UnhandledExceptionHandler (defaults to the benign logger), matching browser behavior. Coverage is documented once on OnUnhandledPromiseRejection in AppRuntime.h: V8 and JavaScriptCore supported; Chakra (in-box/EdgeMode) and JSI no-op. Validated that JavaScript.All stays green (203 passing) on V8 with always-on tracking, i.e. the existing JS suite leaves no stray rejections.

Comment on lines +54 to +57
// Options::EnableUnhandledPromiseRejectionTracking is not honored on this backend: the OS
// EdgeMode Chakra runtime (chakrart.h) does not expose a host promise-rejection tracker
// (JsSetHostPromiseRejectionTracker is ChakraCore-only), so there is no hook to route
// unhandled rejections from. The option is implemented for the V8 backend.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document coverage once in the header rather than a lone per-backend note here. With JSC added, V8 and JSC have it; Chakra and JSI both can't — Chakra has no jsrt host hook, and neither jsi::Runtime nor V8JSI exposes one (would require forking V8JSI). A single Chakra note shouldn't imply the others are covered.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 97e2c68. Trimmed this to a one-line note that defers to AppRuntime.h for the full coverage matrix, so it no longer implies the other backends are covered.

// (via AppRuntime::Dispatch) so a synchronous `Promise.reject(e); ...; p.catch(...)` is
// removed before it is ever reported. Keyed by promise identity hash; the promise and reason
// are held in v8::Global handles so they survive until the deferred flush runs.
struct V8RejectionTracker

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR should include the JSC implementation, not defer it. JSC exposes JSGlobalContextSetUnhandledRejectionCallback (macOS 10.15.4+/iOS 13.4+) and CI already runs the JSC matrix, so there's no validation barrier. The candidate map / deferred-flush / ToError bookkeeping here is engine-agnostic — factor it out so the JSC backend is a thin adapter, not a second copy. (Chakra and JSI stay no-ops: neither exposes a host rejection hook.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 97e2c68. JSC is now implemented in this PR. AppRuntime_JavaScriptCore.cpp registers a callback via JSGlobalContextSetUnhandledRejectionCallback (forward-declared SPI from JSContextRefPrivate.h, guarded with __builtin_available(macOS 10.15.4, iOS 13.4, *)). The engine-agnostic bookkeeping (ToError + the candidate/deferred-flush logic) is factored into AppRuntime_PromiseRejection.h as PromiseRejectionTracker<CandidateT>; V8 instantiates it with a V8RejectionCandidate. JSC needs no candidate map — its hook fires at the microtask checkpoint only for still-unhandled rejections — so its adapter is a thin direct report. I'm watching the macOS/JSC matrix; if the private symbol isn't in the CI SDK headers the forward declaration should still link against the framework export, and I'll iterate on that job if needed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is fully green now. The Apple JSC matrix (macOS/iOS) validates the adapter; the first run surfaced one real wrinkle on the non-Apple JSC builds — JSGlobalContextSetUnhandledRejectionCallback is an Apple SPI absent from WebKitGTK, and __builtin_available is Clang/Apple-only, so the Linux (WebKitGTK/gcc) and Android JSC jobs failed to compile. Fixed in b2553a2 by guarding the rejection machinery with #if __APPLE__ (consistent with the existing JSGlobalContextSetInspectable guard) and tightening the test gate to V8 || (JavaScriptCore && __APPLE__). WebKitGTK/Android JSC are now documented no-ops. All jobs pass on b2553a2.

Comment thread Tests/UnitTests/Shared/Shared.cpp Outdated
Comment on lines +288 to +291
#if defined(JSRUNTIMEHOST_NAPI_ENGINE)
if (std::string_view{JSRUNTIMEHOST_NAPI_ENGINE} != "V8")
{
GTEST_SKIP() << "unhandled promise rejection tracking is only implemented for the V8 backend";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This runtime string-compare is inconsistent with the JSI gate below (#if !defined(JSRUNTIMEHOST_NAPI_ENGINE_JSI), lines 353/423). Converge on that compile-time boolean pattern: have CMake emit JSRUNTIMEHOST_NAPI_ENGINE_${NAPI_JAVASCRIPT_ENGINE} (parallel to _JSI) and gate with #if defined(JSRUNTIMEHOST_NAPI_ENGINE_V8), dropping the stringly-typed JSRUNTIMEHOST_NAPI_ENGINE="..." macro and the std::string_view compare (a "v8"/"V8" typo silently misbehaves). The JSI gate is necessarily compile-time, so that's the direction. The larger Shared.cpp split into per-area/per-engine files is a separate follow-up.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 97e2c68. CMake now emits JSRUNTIMEHOST_NAPI_ENGINE_${NAPI_JAVASCRIPT_ENGINE} (parallel to _JSI) for both the desktop and Android UnitTests targets, and the rejection tests gate with #if !defined(JSRUNTIMEHOST_NAPI_ENGINE_V8) && !defined(JSRUNTIMEHOST_NAPI_ENGINE_JavaScriptCore) — the std::string_view compare and the stringly-typed JSRUNTIMEHOST_NAPI_ENGINE="..." reliance are gone from these tests. Agreed the broader per-area/per-engine Shared.cpp split is a separate follow-up.

…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>
@bkaradzic-microsoft

Copy link
Copy Markdown
Member Author

@bghgary Thanks for the thorough review — all of it is addressed in 97e2c68 (hop 3 refactor):

  • JSC implemented in this PR via JSGlobalContextSetUnhandledRejectionCallback; engine-agnostic bookkeeping factored into AppRuntime_PromiseRejection.h (PromiseRejectionTracker<CandidateT> + ToError), so JSC is a thin adapter and V8 instantiates the shared tracker.
  • Flag dropped — unhandled rejections always route to UnhandledExceptionHandler; coverage documented once on AppRuntime.h (V8 + JSC supported; Chakra/JSI no-op).
  • V8 iterator invalidation fixed (drain candidates via std::move before reporting).
  • V8 GetIdentityHash collision fixed (vector + promise-identity match, hash key removed).
  • Compile-time test gate — CMake emits JSRUNTIMEHOST_NAPI_ENGINE_${ENGINE}; tests gate on #if defined(...), no more std::string_view compare.
  • Chakra note trimmed to defer to the header.

Validated locally 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 gate. JSC validates on the macOS CI matrix; watching that job (the rejection callback uses an SPI from JSContextRefPrivate.h, forward-declared and __builtin_available-guarded — I'll iterate if the CI SDK doesn't link it).

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>
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.

4 participants