Diagnosability: route unhandled promise rejections to the host handler (#196)

[bkaradzic-microsoft]

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 .then with no .catch) currently vanishes silently — AppRuntime::Dispatch only catches synchronous throws. This routes unhandled promise rejections into the existing UnhandledExceptionHandler (the Sentry/Bugsnag hook), matching the browser unhandledrejection behavior. 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:

Engine	Hook	Behavior
V8 (all platforms)	Isolate::SetPromiseRejectCallback	tracked
JavaScriptCore (Apple)	JSGlobalContextSetUnhandledRejectionCallback (Apple SPI)	tracked
JavaScriptCore (Linux / WebKitGTK)	SPI not exposed	no-op
Chakra (in-box / EdgeMode)	JsSetHostPromiseRejectionTracker is ChakraCore-only	no-op
JSI / V8JSI	no host callback surfaced	no-op

Open design questions

@bghgary raised two concerns on #200 that this PR exists to resolve properly rather than rush:

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

   • Make the no-op loud: log a one-time warning (or debug-assert) when tracking is requested on an engine that can't honor it, so embedders aren't misled.
   • Gate it behind an explicit opt-in so enabling it is a conscious choice.
   • Hold it until Chakra coverage exists.

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

   • Move the Apple-JSC SPI glue into a platform-selected source (the AppRuntime_${PLATFORM} mechanism) with a no-op on Unix/Linux JSC — instead of #if __APPLE__ inside AppRuntime_JavaScriptCore.cpp.
   • Expose a small capability symbol per backend (e.g. SupportsUnhandledRejectionTracking()) so the tests do a runtime GTEST_SKIP() instead of a compile-time #if.

Contents

• Core/AppRuntime — V8 and Apple-JSC rejection callbacks routed through OnUnhandledPromiseRejection → UnhandledExceptionHandler; Chakra/JSI documented no-ops. New AppRuntime_PromiseRejection.h helper.
• Native tests in 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

Fixes #196

[matthargett]

Strong preference for always-on where supported + a loud one-time no-op warning elsewhere, with the capability query promoted to public API rather than test-only. Opt-in safety nets don't get enabled until after the incident, and holding for Chakra means holding forever — JsSetHostPromiseRejectionTracker is ChakraCore-only and in-box jsrt is frozen, so that column never changes. With SupportsUnhandledRejectionTracking() public, an embedder's telemetry init can assert coverage at startup, which turns the partial-coverage footgun from silent into checkable. We'd wire exactly that assert into BabylonNative's Playground as the reference pattern.

+1 to the #ifdef-free rework — the per-backend capability symbol is the same thing the public query needs.

One design ask: route rejections through an internal observed/handled seam instead of straight into UnhandledExceptionHandler. The follow-up that makes browser-style reporters drop in unmodified is dispatching real unhandledrejection/rejectionhandled PromiseRejectionEvents on globalThis, with the native handler as backstop — and the end-of-turn deferral bookkeeping here is already the hard half of rejectionhandled semantics. Happy to take that follow-up once this lands, this will be a huge improvement over the divergent JS code people have to write in frameworks like RN that are way out of spec.

[Copilot]

Pull request overview

Routes unhandled JavaScript Promise rejections into the existing AppRuntime::Options::UnhandledExceptionHandler (where supported by the underlying JS engine) to improve diagnosability of fire-and-forget async failures, and adds unit tests to validate the behavior.

Changes:

• Add engine-side unhandled promise rejection hooks for V8 and Apple JavaScriptCore, forwarding to AppRuntime::OnUnhandledPromiseRejection.
• Introduce AppRuntime_PromiseRejection.h with shared rejection bookkeeping/wrapping helpers.
• Add engine-gated unit tests and CMake compile definitions to enable per-engine test selection.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
Tests/UnitTests/Shared/Shared.cpp	Adds unit tests for “unhandled rejection reaches handler” and “synchronously handled rejection does not”.
Tests/UnitTests/CMakeLists.txt	Defines engine macros (string + per-engine define) to enable compile-time test gating.
Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt	Mirrors engine macro compile definitions for the Android JNI unit test target.
Core/AppRuntime/Source/AppRuntime.cpp	Adds AppRuntime::OnUnhandledPromiseRejection forwarding to the host handler.
Core/AppRuntime/Source/AppRuntime_V8.cpp	Installs a V8 SetPromiseRejectCallback hook and defers reporting to end-of-turn via a tracker.
Core/AppRuntime/Source/AppRuntime_PromiseRejection.h	Adds shared helper to wrap rejection reasons and track/report candidates (V8).
Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp	Registers Apple JSC SPI unhandled-rejection callback (when available) and forwards to host handler.
Core/AppRuntime/Source/AppRuntime_Chakra.cpp	Documents that unhandled-rejection tracking is a no-op on in-box/EdgeMode Chakra.
Core/AppRuntime/Include/Babylon/AppRuntime.h	Documents and exposes OnUnhandledPromiseRejection for engine backends to call.

---

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

[bkaradzic-microsoft]

Apologies for the slow reply on this one -- it sat while the split-out PRs ahead of it went through review.

Taking the points in order.

Always-on + loud no-op, and a public capability query. I agree, and the reasoning about opt-in safety nets matches what I have seen. The one thing I would push back on slightly is where the loudness goes: a one-time warning logged from the runtime is easy to miss in an embedder that has already routed logging somewhere, and it fires at a point where the embedder cannot do anything but read it. A public SupportsUnhandledRejectionTracking() is the part that actually turns the footgun from silent into checkable, because it lets the assert live in the embedder's telemetry init where a failure is actionable. So: public query as the primary mechanism, warning as the backstop for embedders that never call it. That is a superset of what you asked for, not a substitute, so I do not think we disagree.

You are also right that holding for Chakra means holding forever. JsSetHostPromiseRejectionTracker is ChakraCore-only and in-box jsrt is frozen, so that column never changes -- which makes "hold until coverage is complete" equivalent to "never ship it".

The #ifdef-free rework. Agreed, and as you say the per-backend capability symbol is the same thing the public query needs, so it is one change rather than two. The Apple SPI glue moves into a platform-selected source and the tests switch from a compile-time #if to a runtime GTEST_SKIP() on the capability.

The observed/handled seam. This is the most interesting ask and I had not thought about it in those terms. You are right that the end-of-turn deferral bookkeeping is the hard half of rejectionhandled: the tracker already has to distinguish "rejected with no handler" from "handler attached before the turn ended", which is exactly the state a rejectionhandled event needs. Routing through an internal seam rather than calling UnhandledExceptionHandler directly costs nothing now and is the difference between your follow-up being a drop-in and being a rewrite, so I will do it. Happy to hand the PromiseRejectionEvent dispatch to you once this lands.

@bghgary -- before I build this out, one decision is yours rather than mine. This PR exists because you raised whether an engine-partial diagnosability feature should ship at all. The proposal above is always-on where supported, public capability query, loud one-time warning where not, which is a deliberate choice to ship partial coverage and make the gap visible rather than gate it behind an opt-in or hold it. If you would rather it be opt-in, that changes the shape of the API and I would rather find out now than after the rework. Could you settle that one?

Separately, the four review comments on this PR are addressed in 207b97a -- the ToError one was a real bug (Promise.reject({ code: 42 }) arrived with an empty message) and now has a regression test.