AppRuntime: route unhandled promise rejections to the host handler (V8) — hop 3 of #196#201
Closed
bkaradzic-microsoft wants to merge 4 commits into
Closed
Conversation
Previously a fire-and-forget rejected promise (e.g. an un-awaited fetch() that fails, or a throw inside a .then with no .catch) vanished silently: AppRuntime::Dispatch only catches synchronous Napi::Error throws, and no engine had a promise-rejection tracker wired, so the embedder's UnhandledExceptionHandler (the Sentry/Bugsnag hook) never fired and the process exited 0. This is hop 3 of BabylonJS#196. - AppRuntime::Options gains opt-in EnableUnhandledPromiseRejectionTracking (default false = no behavior change; no tracker is installed). When set, unhandled rejections route into the existing UnhandledExceptionHandler. - AppRuntime::OnUnhandledPromiseRejection(napi_value) wraps the rejection reason as a Napi::Error (forwarding Error-like reasons as-is, stringifying others) and invokes the handler. - V8 (tested): Isolate::SetPromiseRejectCallback. Reporting is deferred via Dispatch, so a rejection handled synchronously in the same turn (`const p = Promise.reject(e); p.catch(...)`) is dropped before it is reported -- matching Node semantics. Promise/reason held in v8::Global handles across the hop. - Chakra: the OS EdgeMode runtime (chakrart.h) exposes no host promise-rejection tracker (JsSetHostPromiseRejectionTracker is ChakraCore-only), so the option is a documented no-op on this backend. - Native tests (Shared.cpp): a fire-and-forget rejection reaches the handler; a synchronously-handled rejection does not. Skipped on the Chakra backend. JavaScriptCore and JSI trackers are follow-ups (see BabylonJS#196). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The tests enabled tracking and waited for a report, but only V8 implements the tracker, so on JavaScriptCore/JSI/EdgeMode-Chakra the ReachesHandler test timed out (30s) and failed the suite. Skip both unless the engine is V8. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The JSI Node-API shim's napi.h has no Napi::Value/Error(napi_env, napi_value) constructor (it wraps jsi::Value), so the shared AppRuntime.cpp could not compile for JSI. OnUnhandledPromiseRejection now takes a const Napi::Error&, and the napi_value->Napi::Error wrapping is done in AppRuntime_V8.cpp (the V8/standard shim that supports it). Also drops an unused structured-binding to satisfy -Werror. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Android test app compiles Shared.cpp via its own cpp/CMakeLists.txt, which did not define JSRUNTIMEHOST_NAPI_ENGINE, so the engine-gated unhandled-rejection tests were not skipped on the (non-V8) JavaScriptCore backend and the ReachesHandler test timed out (30s). Mirror the desktop test target's define. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
Consolidated into #200 — the whole #196 chain is now a single PR (hop 3 is the second commit there). Closing to avoid three PRs for one feature in one repo. The unhandled-rejection work, all CI fixes (V8-gating, the JSI napi.h build fix, and the Android JNI engine define), and the native tests are preserved in #200. |
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.
Hop 3 of #196 — make fire-and-forget promise rejections reach the embedding app instead of vanishing.
Problem
AppRuntime::Dispatchonly catches synchronousNapi::Errorthrows from dispatched callbacks, and no engine had a promise-rejection tracker wired anywhere in Core. So a fire-and-forgetfetch()that rejects (or a throw inside any.thenwith no.catch) never reached the embedder'sUnhandledExceptionHandler— the Sentry/Bugsnag hook — and the process exited0.Approach
AppRuntime::Options::EnableUnhandledPromiseRejectionTracking(opt-in, defaultfalse). Whenfalse, no tracker is installed and behavior is unchanged. Whentrue, unhandled rejections route into the existingUnhandledExceptionHandler.AppRuntime::OnUnhandledPromiseRejection(napi_value)wraps the rejection reason as aNapi::Error(forwardingError-like reasons as-is to preserve message/stack/cause; stringifying non-objects) and invokes the handler.Dispatch, so a rejection that is handled synchronously in the same turn (const p = Promise.reject(e); p.catch(...)) is dropped before it is ever reported — no false positives.Engine support
Isolate::SetPromiseRejectCallback; promise/reason held inv8::Globalacross the deferred flush).chakrart.h) exposes no host promise-rejection tracker (JsSetHostPromiseRejectionTrackeris ChakraCore-only, and JsRuntimeHost builds against EdgeMode viaUSE_EDGEMODE_JSRT).JSGlobalContextSetUnhandledRejectionCallback(JSC) and the JSI tracker. Happy to add in this PR or separately; would value guidance on the JSI/V8JSI path.Validation
Native tests in
Shared.cpp(skipped on the Chakra backend, which can't support the feature):UnhandledPromiseRejectionReachesHandler— a fire-and-forgetPromise.reject(new Error('boom'))reaches the host handler with the right message.SynchronouslyHandledRejectionDoesNotReachHandler— a synchronously-handled rejection does not reach the handler (verifies the deferral).Built and ran on Win32 / V8 (both pass) and Win32 / Chakra (framework compiles, both tests skip as expected). The opt-in default keeps the existing
JavaScript, Allsuite unaffected.Notes for reviewers
OnUnhandledPromiseRejectionis exposed as a publicAppRuntimemethod (documented "internal, engine-implementation use") because the V8 rejection callback is a bare function pointer whose helpers are free functions and can't reach a private member.reinterpret_cast<napi_value>(*local)bridge (mirrored fromjs_native_api_v8.h, which isn't on the public include path) with the samestatic_assert.Part of the #196 chain (hop 2 is #200).
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com