From 2d712e5f36538858edf117922562d9d8df01ed5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Branimir=20Karad=C5=BEi=C4=87=20=28via=20Copilot=29?= <223556219+Copilot@users.noreply.github.com> Date: Tue, 16 Jun 2026 12:48:44 -0700 Subject: [PATCH 1/6] Hop 2: surface transport-failure detail through fetch and XMLHttpRequest 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/JsRuntimeHost#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 (#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> --- CMakeLists.txt | 2 +- Polyfills/Fetch/Readme.md | 33 +++++++ Polyfills/Fetch/Source/Fetch.cpp | 96 +++++++++++++++++-- Polyfills/XMLHttpRequest/Readme.md | 16 +++- .../XMLHttpRequest/Source/XMLHttpRequest.cpp | 20 ++++ .../XMLHttpRequest/Source/XMLHttpRequest.h | 2 + Tests/UnitTests/Scripts/tests.ts | 58 +++++++++++ 7 files changed, 219 insertions(+), 8 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 714e175b..513fb318 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -41,7 +41,7 @@ FetchContent_Declare(llhttp EXCLUDE_FROM_ALL) FetchContent_Declare(UrlLib GIT_REPOSITORY https://github.com/BabylonJS/UrlLib.git - GIT_TAG 74985214bd4f83a4906b2c62134ac2f9ab89e1ae + GIT_TAG e86ffb34e77092266145497681efc74e0a920ffe EXCLUDE_FROM_ALL) # -------------------------------------------------- diff --git a/Polyfills/Fetch/Readme.md b/Polyfills/Fetch/Readme.md index 5528774a..00c41cfc 100644 --- a/Polyfills/Fetch/Readme.md +++ b/Polyfills/Fetch/Readme.md @@ -28,3 +28,36 @@ Like `XMLHttpRequest`, `fetch()` supports loading local resources: * Only `GET` and `POST` methods are supported (a `UrlLib` limitation shared with `XMLHttpRequest`). * Only string request bodies are supported. * Consistent with the fetch spec, the promise rejects only on transport-level failures. A completed request with a non-`2xx` status (e.g. `404`) still resolves, with `response.ok === false`. + +## Transport-failure rejections +On a transport-level failure (DNS failure, connection refused, TLS failure, missing local +asset, ...) the promise rejects with a **`TypeError`** whose `message` is the stable string +`"fetch failed"`. The message is intentionally constant so crash-report grouping stays intact; +the variable detail is carried on `error.cause` (the Node/undici shape), never spread across the +message: + +```js +try { + await fetch("https://does-not-resolve.invalid/"); +} catch (error) { + error.message; // "fetch failed" (stable) + error.cause.code; // stable token, e.g. "CURLE_COULDNT_RESOLVE_HOST" (where available) + error.cause.detail; // full normalized UrlLib string (where available) + error.cause.url; // the requested URL + error.cause.status; // 0 for a transport failure +} +``` + +`error.cause.code` / `error.cause.detail` come from `UrlLib`'s normalized transport-error +accessors and are present on the backends that populate them (Apple, Linux); on backends that do +not yet (Windows, Android) they are omitted while the standard observable shape (a `TypeError` +with the stable message, plus `cause.url` / `cause.status`) is preserved. This is a deliberate, +strictly-additive superset of the spec: spec-conformant code only sees a `TypeError`, exactly as +in a browser, while BN-aware diagnostic code can read `cause` to distinguish a DNS failure from a +refused connection or a missing local asset. + +The rejection's `stack` is captured synchronously at the `fetch()` call site (before the request +is handed to a worker thread), so crash reports can attribute the failing call rather than an +empty scheduler tick. (Engines that only materialize `.stack` when an error is thrown may omit +the frames.) + diff --git a/Polyfills/Fetch/Source/Fetch.cpp b/Polyfills/Fetch/Source/Fetch.cpp index 7ffee288..afdef000 100644 --- a/Polyfills/Fetch/Source/Fetch.cpp +++ b/Polyfills/Fetch/Source/Fetch.cpp @@ -37,6 +37,80 @@ namespace Babylon::Polyfills::Internal }); } + // Stable message used for every transport-failure rejection. Browsers and Node both keep + // this constant (the variable detail rides on `cause`) so crash-report grouping stays + // intact; we follow Node/undici's "fetch failed" spelling. + constexpr const char* FETCH_FAILED_MESSAGE = "fetch failed"; + + // Snapshot the JS call-site stack synchronously, inside fetch(), before SendAsync() hands + // the request to a worker thread. The transport-failure rejection is otherwise built in a + // continuation that runs after fetch() has returned, where an Error would capture zero user + // frames. We go through the global JS `Error` constructor (rather than napi_create_error) so + // engines that materialize `.stack` from the JS constructor path capture the live caller + // frames. The result is a plain std::string, safe to carry across the thread hop (unlike a + // Napi::Reference, which must be created and destroyed on the JS thread). Empty if the engine + // does not expose a stack at construction time (e.g. Chakra, which only populates `.stack` + // when an error is thrown) -- in that case the rejection simply carries no synthetic frames. + std::string CaptureCallSiteStack(Napi::Env env) + { + const Napi::Value errorCtor = env.Global().Get("Error"); + if (!errorCtor.IsFunction()) + { + return {}; + } + const Napi::Object error = errorCtor.As().New({}); + const Napi::Value stack = error.Get("stack"); + return stack.IsString() ? stack.As().Utf8Value() : std::string{}; + } + + // Reattach the synchronously-captured frames to the rejection's Error, replacing the captured + // header line (e.g. "Error\n at ...") with one matching the TypeError we actually reject + // with, so the stack reads correctly while preserving the user's call site. + std::string ComposeRejectionStack(const std::string& capturedStack) + { + std::string header{"TypeError: "}; + header += FETCH_FAILED_MESSAGE; + + const auto firstNewline = capturedStack.find('\n'); + if (firstNewline == std::string::npos) + { + return header; + } + return header + capturedStack.substr(firstNewline); + } + + // Build the transport-failure rejection: a TypeError with a stable message, carrying the + // variable detail under `cause` (Node/undici shape) rather than as top-level own-properties. + // `code`/`detail` come from UrlLib's normalized accessors and may be empty on backends that + // do not yet populate them (Windows/Android today) -- in that case the standard observable + // shape (TypeError + stable message + url) is preserved and only the extra detail is absent. + Napi::Error BuildTransportError(Napi::Env env, const UrlLib::UrlRequest& request, const std::string& url, const std::string& capturedStack) + { + Napi::Error error = Napi::TypeError::New(env, FETCH_FAILED_MESSAGE); + + Napi::Object cause = Napi::Object::New(env); + const std::string code{request.ErrorSymbol()}; + const std::string detail{request.ErrorString()}; + if (!code.empty()) + { + cause.Set("code", Napi::String::New(env, code)); + } + if (!detail.empty()) + { + cause.Set("detail", Napi::String::New(env, detail)); + } + cause.Set("url", Napi::String::New(env, url)); + cause.Set("status", Napi::Number::New(env, static_cast(static_cast(request.StatusCode())))); + error.Set("cause", cause); + + if (!capturedStack.empty()) + { + error.Set("stack", Napi::String::New(env, ComposeRejectionStack(capturedStack))); + } + + return error; + } + // fetch only resolves for GET and POST because the underlying UrlLib transport supports nothing else. UrlLib::UrlMethod ParseMethod(const std::string& method) { @@ -306,6 +380,11 @@ namespace Babylon::Polyfills::Internal request->SetRequestBody(std::move(*body)); } + // Snapshot the caller's stack now -- before SendAsync() moves work onto a worker + // thread -- so a transport-failure rejection can be attributed to the fetch() call + // site rather than to an empty scheduler tick. + const std::string capturedStack = CaptureCallSiteStack(env); + // arcana::task::then captures the scheduler by reference (see arcana task.h) and // invokes it on the worker thread when the request completes -- after this fetch() // call has returned. A stack-local scheduler would therefore dangle. Heap-allocate @@ -313,7 +392,7 @@ namespace Babylon::Polyfills::Internal auto scheduler = std::make_shared(JsRuntime::GetFromJavaScript(env)); request->SendAsync() .then(*scheduler, arcana::cancellation::none(), - [deferred, request, env](const arcana::expected& result) { + [deferred, request, env, url, capturedStack](const arcana::expected& result) { const int status = static_cast(request->StatusCode()); // Per the WHATWG fetch spec, only transport-level failures reject. A completed @@ -321,7 +400,11 @@ namespace Babylon::Polyfills::Internal // A status of 0 indicates the transport never produced a response (network error). if (result.has_error() || status == 0) { - throw std::runtime_error{"fetch: network request failed"}; + // Reject with a TypeError carrying the normalized transport detail on `cause` + // (built here, where the UrlRequest's ErrorString()/ErrorSymbol() are still in + // scope) instead of throwing a constant string that discards them. + deferred.Reject(BuildTransportError(env, *request, url, capturedStack).Value()); + return; } auto data = std::make_shared(); @@ -339,10 +422,11 @@ namespace Babylon::Polyfills::Internal }) .then(*scheduler, arcana::cancellation::none(), [deferred, env, scheduler](const arcana::expected& result) { - // A throw from the continuation above (e.g. a network failure or a JS - // exception while building the response) lands here as an error result; - // surface it as a promise rejection so await fetch(...) settles. The - // scheduler is co-owned here so it outlives the in-flight request. + // A throw from the continuation above (e.g. a JS exception while building the + // response) lands here as an error result; surface it as a promise rejection so + // await fetch(...) settles. Transport failures are already rejected above, so this + // only handles unexpected exceptions. The scheduler is co-owned here so it + // outlives the in-flight request. if (result.has_error()) { deferred.Reject(Napi::Error::New(env, result.error()).Value()); diff --git a/Polyfills/XMLHttpRequest/Readme.md b/Polyfills/XMLHttpRequest/Readme.md index 5c40c747..7762c351 100644 --- a/Polyfills/XMLHttpRequest/Readme.md +++ b/Polyfills/XMLHttpRequest/Readme.md @@ -13,4 +13,18 @@ Unlike the web, XMLHttpRequest supports loading local files using two schemes: ## Other things to be aware of: * Only `GET` requests are currently supported -* For `readyState`, we only support `UNSENT`, `OPENED`, and `DONE` \ No newline at end of file +* For `readyState`, we only support `UNSENT`, `OPENED`, and `DONE` + +## Transport-error diagnostics (non-standard) +A transport-level failure surfaces the standard way -- an `error` event followed by `loadend`, +with `status === 0` -- exactly as on the web. In addition, two **non-standard, additive** +read-only properties expose the normalized `UrlLib` transport-error detail so BN-aware code can +tell a DNS failure from a refused connection or a missing local asset: +* `errorCode` -- the stable symbolic token (e.g. `"CURLE_COULDNT_CONNECT"`, `"NSURLErrorTimedOut"`, + `"AppResourceNotFound"`) +* `errorDetail` -- the full normalized `":(): "` string + +Both are empty strings unless the request failed at the transport layer, and are populated only +on backends that expose the detail (Apple, Linux) -- empty on Windows/Android until those +backends populate `UrlLib`'s accessors. Browsers do not expose these properties, so +spec-conformant code is unaffected. diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index 10903cac..495222a6 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp @@ -81,6 +81,12 @@ namespace Babylon::Polyfills::Internal InstanceAccessor("responseURL", &XMLHttpRequest::GetResponseURL, nullptr), InstanceAccessor("status", &XMLHttpRequest::GetStatus, nullptr), InstanceAccessor("statusText", &XMLHttpRequest::GetStatusText, nullptr), + // Non-standard, additive diagnostics: the normalized transport-error detail from + // UrlLib, empty unless the request failed at the transport layer. Browsers do not + // expose these, so spec-conformant code is unaffected; BN-aware code can read them + // to tell a DNS failure from a refused connection or a missing local asset. + InstanceAccessor("errorCode", &XMLHttpRequest::GetErrorCode, nullptr), + InstanceAccessor("errorDetail", &XMLHttpRequest::GetErrorDetail, nullptr), InstanceMethod("getAllResponseHeaders", &XMLHttpRequest::GetAllResponseHeaders), InstanceMethod("getResponseHeader", &XMLHttpRequest::GetResponseHeader), InstanceMethod("setRequestHeader", &XMLHttpRequest::SetRequestHeader), @@ -162,6 +168,20 @@ namespace Babylon::Polyfills::Internal return Napi::String::New(Env(), std::string{m_request.StatusText()}); } + Napi::Value XMLHttpRequest::GetErrorCode(const Napi::CallbackInfo&) + { + // Stable symbolic token for a transport failure (e.g. "CURLE_COULDNT_CONNECT", + // "NSURLErrorTimedOut", "AppResourceNotFound"); empty when there was no transport failure. + return Napi::String::New(Env(), std::string{m_request.ErrorSymbol()}); + } + + Napi::Value XMLHttpRequest::GetErrorDetail(const Napi::CallbackInfo&) + { + // Full normalized ":(): " string; empty when there was no + // transport failure. + return Napi::String::New(Env(), std::string{m_request.ErrorString()}); + } + Napi::Value XMLHttpRequest::GetResponseHeader(const Napi::CallbackInfo& info) { const auto headerName = info[0].As().Utf8Value(); diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h index 307828fd..ed35dbc9 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h @@ -35,6 +35,8 @@ namespace Babylon::Polyfills::Internal Napi::Value GetResponseURL(const Napi::CallbackInfo& info); Napi::Value GetStatus(const Napi::CallbackInfo& info); Napi::Value GetStatusText(const Napi::CallbackInfo& info); + Napi::Value GetErrorCode(const Napi::CallbackInfo& info); + Napi::Value GetErrorDetail(const Napi::CallbackInfo& info); void AddEventListener(const Napi::CallbackInfo& info); void RemoveEventListener(const Napi::CallbackInfo& info); diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index e0647092..23cc76f0 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -153,6 +153,23 @@ describe("XMLHTTPRequest", function () { expect(result.readyState).to.equal(4); }); + it("should expose errorCode/errorDetail diagnostics after a transport failure", async function () { + this.timeout(30000); + const xhr: any = await createRequest("GET", "http://127.0.0.1:1/"); + expect(xhr.status).to.equal(0); + // Non-standard, additive diagnostics: always strings; populated on Apple/Linux and empty + // on Windows/Android until those backends populate UrlLib's accessors. Either way the + // standard error event + status===0 behavior (asserted above) is unchanged. + expect(xhr.errorCode).to.be.a("string"); + expect(xhr.errorDetail).to.be.a("string"); + }); + + it("should expose empty errorCode/errorDetail after a successful request", async function () { + const xhr: any = await createRequest("GET", "app:///Scripts/symlink_target.js"); + expect(xhr.errorCode).to.equal(""); + expect(xhr.errorDetail).to.equal(""); + }); + it("should throw something when opening //", async function () { function openDoubleSlash() { const xhr = new XMLHttpRequest(); @@ -331,6 +348,47 @@ describe("fetch", function () { } expect(rejected).to.equal(true); }); + + it("should reject a transport failure with a TypeError carrying detail on cause", async function () { + this.timeout(30000); + let error: any; + try { + // Nothing listens on this loopback port, so the connection is refused -- a transport + // failure (status 0), distinct from an HTTP error status. + await fetch("http://127.0.0.1:1/"); + } catch (e) { + error = e; + } + expect(error, "fetch should have rejected").to.not.equal(undefined); + // Spec-conformant shape: network errors reject with a TypeError whose message is stable + // (browsers/Node/undici all keep it constant so crash-report grouping stays intact). + expect(error).to.be.an.instanceof(TypeError); + expect(error.message).to.equal("fetch failed"); + // The variable detail rides on `cause` (Node/undici shape), never on the stable message. + expect(error.cause, "error.cause should be populated").to.be.an("object"); + expect(error.cause.url).to.contain("127.0.0.1"); + expect(error.cause.status).to.equal(0); + // On backends where UrlLib populates transport detail (Apple/Linux) `code`/`detail` are + // present stable tokens; on backends that don't yet (Windows/Android) they are absent -- + // the stable observable shape above is preserved either way. + if (error.cause.code !== undefined) { + expect(error.cause.code).to.be.a("string").and.not.equal(""); + expect(error.cause.detail).to.be.a("string").and.not.equal(""); + } + }); + + it("should reject a missing app:// asset with a TypeError (distinct from a network failure)", async function () { + let error: any; + try { + await fetch("app:///does_not_exist.js"); + } catch (e) { + error = e; + } + expect(error, "fetch should have rejected").to.not.equal(undefined); + expect(error).to.be.an.instanceof(TypeError); + expect(error.message).to.equal("fetch failed"); + expect(error.cause.url).to.contain("does_not_exist.js"); + }); }); describe("setTimeout", function () { From 1405c418ac701deade447b3cef9232f0647fe174 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Branimir=20Karad=C5=BEi=C4=87=20=28via=20Copilot=29?= <223556219+Copilot@users.noreply.github.com> Date: Tue, 16 Jun 2026 13:00:19 -0700 Subject: [PATCH 2/6] Address review: robust Error-constructor detection + document XHR error 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> --- Polyfills/Fetch/Source/Fetch.cpp | 7 ++++++- Polyfills/XMLHttpRequest/Readme.md | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/Polyfills/Fetch/Source/Fetch.cpp b/Polyfills/Fetch/Source/Fetch.cpp index afdef000..24c2114f 100644 --- a/Polyfills/Fetch/Source/Fetch.cpp +++ b/Polyfills/Fetch/Source/Fetch.cpp @@ -53,8 +53,13 @@ namespace Babylon::Polyfills::Internal // when an error is thrown) -- in that case the rejection simply carries no synthetic frames. std::string CaptureCallSiteStack(Napi::Env env) { + // Detect the global Error constructor with IsUndefined()/IsNull() rather than + // IsFunction(): some JavaScriptCore/JSI builds classify constructor functions as + // typeof 'object', so napi_typeof reports napi_object and IsFunction() would + // incorrectly skip stack capture even though Error is callable (see the Blob check + // below for the same rationale). Error is always present, so this guard is defensive. const Napi::Value errorCtor = env.Global().Get("Error"); - if (!errorCtor.IsFunction()) + if (errorCtor.IsUndefined() || errorCtor.IsNull()) { return {}; } diff --git a/Polyfills/XMLHttpRequest/Readme.md b/Polyfills/XMLHttpRequest/Readme.md index 7762c351..ea00d006 100644 --- a/Polyfills/XMLHttpRequest/Readme.md +++ b/Polyfills/XMLHttpRequest/Readme.md @@ -5,6 +5,7 @@ Minimal implementation of XMLHttpRequest required to support the Babylon.js Requ We do not support `onload`-style event listeners. Instead, you should listen to events using `addEventListener`. At the moment, we only support the following events: * `loadend` * `readystatechange` +* `error` (fired on a transport failure or a non-`2xx` HTTP response, before `loadend`) ## Local files Unlike the web, XMLHttpRequest supports loading local files using two schemes: From aa5a56265b92087ded2fcef4ed089aca06db14f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Branimir=20Karad=C5=BEi=C4=87=20=28via=20Copilot=29?= <223556219+Copilot@users.noreply.github.com> Date: Tue, 16 Jun 2026 16:18:55 -0700 Subject: [PATCH 3/6] Hop 3: route unhandled promise rejections to the host handler 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> --- Core/AppRuntime/Include/Babylon/AppRuntime.h | 15 +++ Core/AppRuntime/Source/AppRuntime.cpp | 7 ++ Core/AppRuntime/Source/AppRuntime_Chakra.cpp | 6 + Core/AppRuntime/Source/AppRuntime_V8.cpp | 114 ++++++++++++++++++ .../Android/app/src/main/cpp/CMakeLists.txt | 1 + Tests/UnitTests/CMakeLists.txt | 1 + Tests/UnitTests/Shared/Shared.cpp | 67 ++++++++++ 7 files changed, 211 insertions(+) diff --git a/Core/AppRuntime/Include/Babylon/AppRuntime.h b/Core/AppRuntime/Include/Babylon/AppRuntime.h index f6ca7dfd..cab418e6 100644 --- a/Core/AppRuntime/Include/Babylon/AppRuntime.h +++ b/Core/AppRuntime/Include/Babylon/AppRuntime.h @@ -21,6 +21,15 @@ namespace Babylon // Optional handler for unhandled exceptions. std::function UnhandledExceptionHandler{DefaultUnhandledExceptionHandler}; + // When true, unhandled promise rejections are routed to UnhandledExceptionHandler so + // the embedder's crash/telemetry pipeline can observe fire-and-forget failures (e.g. an + // un-awaited fetch() that rejects). Defaults to false to preserve existing behavior -- + // when false, no per-engine rejection tracker is installed. Reporting is deferred to the + // end of the current turn, so a rejection that is handled synchronously (e.g. + // `const p = Promise.reject(e); p.catch(...)`) does not reach the handler. Implemented for + // the Chakra and V8 engines. + bool EnableUnhandledPromiseRejectionTracking{false}; + // Defines whether to enable the debugger. Only implemented for V8 and Chakra. bool EnableDebugger{false}; @@ -45,6 +54,12 @@ namespace Babylon void Dispatch(Dispatchable callback); + // Routes an unhandled promise rejection to the embedder's UnhandledExceptionHandler. Called + // by the per-engine promise-rejection tracker installed in RunEnvironmentTier when + // Options::EnableUnhandledPromiseRejectionTracking is set. Intended for internal + // (engine-implementation) use. + void OnUnhandledPromiseRejection(const Napi::Error& error); + // Default unhandled exception handler that outputs the error message to the program output. static void BABYLON_API DefaultUnhandledExceptionHandler(const Napi::Error& error); diff --git a/Core/AppRuntime/Source/AppRuntime.cpp b/Core/AppRuntime/Source/AppRuntime.cpp index 99298df2..acb8e20e 100644 --- a/Core/AppRuntime/Source/AppRuntime.cpp +++ b/Core/AppRuntime/Source/AppRuntime.cpp @@ -125,4 +125,11 @@ namespace Babylon }); }); } + + void AppRuntime::OnUnhandledPromiseRejection(const Napi::Error& error) + { + // The reason is wrapped into a Napi::Error by the engine implementation (the napi_value -> + // Napi::Value bridge is shim-specific), so this just forwards to the embedder's handler. + m_options.UnhandledExceptionHandler(error); + } } diff --git a/Core/AppRuntime/Source/AppRuntime_Chakra.cpp b/Core/AppRuntime/Source/AppRuntime_Chakra.cpp index 2329ad30..09fcb2ed 100644 --- a/Core/AppRuntime/Source/AppRuntime_Chakra.cpp +++ b/Core/AppRuntime/Source/AppRuntime_Chakra.cpp @@ -50,6 +50,12 @@ namespace Babylon }); }, &dispatchFunction)); + + // 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. + ThrowIfFailed(JsProjectWinRTNamespace(L"Windows")); if (m_options.EnableDebugger) diff --git a/Core/AppRuntime/Source/AppRuntime_V8.cpp b/Core/AppRuntime/Source/AppRuntime_V8.cpp index 1297fdbb..6a4b8958 100644 --- a/Core/AppRuntime/Source/AppRuntime_V8.cpp +++ b/Core/AppRuntime/Source/AppRuntime_V8.cpp @@ -8,6 +8,8 @@ #endif #include +#include +#include namespace Babylon { @@ -61,6 +63,105 @@ namespace Babylon }; std::unique_ptr Module::s_module; + + // Tracks promises rejected without a handler so they can be reported once the current turn + // settles. V8 fires kPromiseRejectWithNoHandler when a promise is rejected with no handler + // and kPromiseHandlerAddedAfterReject if one is attached afterwards; reporting is deferred + // (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 + { + AppRuntime* runtime{}; + v8::Isolate* isolate{}; + std::unordered_map, v8::Global>> unhandled{}; + bool flushScheduled{false}; + }; + + // The promise-rejection callback is a bare function pointer with no user-data argument. Each + // AppRuntime owns a dedicated isolate running on its own thread, and V8 invokes the callback + // on that thread, so a thread_local pointer associates the callback with the right tracker + // without risking isolate-data-slot collisions with the Node-API shim. + thread_local V8RejectionTracker* t_rejectionTracker{nullptr}; + + // Mirrors v8impl::JsValueFromV8LocalValue (js_native_api_v8.h), which is internal to the + // Node-API V8 shim and not on the public include path. + static_assert(sizeof(v8::Local) == sizeof(napi_value), + "Cannot convert between v8::Local and napi_value"); + napi_value JsValueFromV8LocalValue(v8::Local local) + { + return reinterpret_cast(*local); + } + + // Wrap a rejection reason as a Napi::Error. An Error-like object is forwarded as-is + // (preserving message/stack/cause); any other value is stringified so the embedder's handler + // always receives a Napi::Error. Done here (not in shared code) because the napi_value -> + // Napi::Value bridge is specific to the V8/standard Node-API shim. + Napi::Error ToError(Napi::Env env, napi_value reason) + { + const Napi::Value reasonValue{env, reason}; + return reasonValue.IsObject() + ? Napi::Error{env, reason} + : Napi::Error::New(env, reasonValue.ToString().Utf8Value()); + } + + void FlushUnhandledRejections(V8RejectionTracker& tracker, Napi::Env env) + { + tracker.flushScheduled = false; + + v8::Isolate::Scope isolateScope{tracker.isolate}; + v8::HandleScope handleScope{tracker.isolate}; + for (auto& entry : tracker.unhandled) + { + const v8::Local reason = entry.second.second.Get(tracker.isolate); + tracker.runtime->OnUnhandledPromiseRejection(ToError(env, JsValueFromV8LocalValue(reason))); + } + tracker.unhandled.clear(); + } + + void OnPromiseReject(v8::PromiseRejectMessage message) + { + V8RejectionTracker* tracker{t_rejectionTracker}; + if (tracker == nullptr) + { + return; + } + + v8::HandleScope handleScope{tracker->isolate}; + const v8::Local promise{message.GetPromise()}; + const int hash{promise->GetIdentityHash()}; + + switch (message.GetEvent()) + { + case v8::kPromiseRejectWithNoHandler: + { + const v8::Local reason{message.GetValue()}; + tracker->unhandled[hash] = { + v8::Global{tracker->isolate, promise}, + v8::Global{tracker->isolate, reason}}; + + if (!tracker->flushScheduled) + { + tracker->flushScheduled = true; + tracker->runtime->Dispatch([tracker](Napi::Env env) { + FlushUnhandledRejections(*tracker, env); + }); + } + break; + } + case v8::kPromiseHandlerAddedAfterReject: + { + tracker->unhandled.erase(hash); + break; + } + default: + { + // kPromiseRejectAfterResolved / kPromiseResolveAfterResolved carry no actionable + // unhandled-rejection signal. + break; + } + } + } } void AppRuntime::RunEnvironmentTier(const char* executablePath) @@ -81,6 +182,13 @@ namespace Babylon Napi::Env env = Napi::Attach(context); + V8RejectionTracker rejectionTracker{this, isolate, {}, false}; + if (m_options.EnableUnhandledPromiseRejectionTracking) + { + t_rejectionTracker = &rejectionTracker; + isolate->SetPromiseRejectCallback(OnPromiseReject); + } + #ifdef ENABLE_V8_INSPECTOR std::optional agent; if (m_options.EnableDebugger) @@ -104,6 +212,12 @@ namespace Babylon } #endif + if (m_options.EnableUnhandledPromiseRejectionTracking) + { + isolate->SetPromiseRejectCallback(nullptr); + t_rejectionTracker = nullptr; + } + Napi::Detach(env); } diff --git a/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt b/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt index 0af5caa8..c870151c 100644 --- a/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt +++ b/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt @@ -22,6 +22,7 @@ add_library(UnitTestsJNI SHARED ${UNIT_TESTS_DIR}/Shared/Shared.cpp) target_compile_definitions(UnitTestsJNI PRIVATE JSRUNTIMEHOST_PLATFORM="${JSRUNTIMEHOST_PLATFORM}") +target_compile_definitions(UnitTestsJNI PRIVATE JSRUNTIMEHOST_NAPI_ENGINE="${NAPI_JAVASCRIPT_ENGINE}") target_compile_definitions(UnitTestsJNI PRIVATE ARCANA_TEST_HOOKS) target_include_directories(UnitTestsJNI diff --git a/Tests/UnitTests/CMakeLists.txt b/Tests/UnitTests/CMakeLists.txt index 2dbc7619..c12b12f8 100644 --- a/Tests/UnitTests/CMakeLists.txt +++ b/Tests/UnitTests/CMakeLists.txt @@ -45,6 +45,7 @@ endif() add_executable(UnitTests ${SOURCES} ${SCRIPTS} ${TYPE_SCRIPTS} ${ASSETS}) target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_PLATFORM="${JSRUNTIMEHOST_PLATFORM}") +target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_NAPI_ENGINE="${NAPI_JAVASCRIPT_ENGINE}") # The V8JSI Node-API shim does not implement napi_create_dataview, so the # CreateDataViewRejectsOverflowingRange test is compiled out on that backend. diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index a920fa1f..8530b04c 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include namespace @@ -279,6 +280,72 @@ TEST(AppRuntime, DestroyDoesNotDeadlock) testThread.join(); } +TEST(AppRuntime, UnhandledPromiseRejectionReachesHandler) +{ + // EnableUnhandledPromiseRejectionTracking is only implemented on the V8 backend so far (the OS + // EdgeMode Chakra runtime exposes no host promise-rejection hook; JavaScriptCore/JSI are + // follow-ups). Skip elsewhere so the test doesn't hang waiting for a report that never comes. +#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"; + } +#endif + + // A fire-and-forget rejected promise (no handler ever attached) must reach the embedder's + // UnhandledExceptionHandler when EnableUnhandledPromiseRejectionTracking is set. + Babylon::AppRuntime::Options options{}; + options.EnableUnhandledPromiseRejectionTracking = true; + + std::promise rejectionMessage; + options.UnhandledExceptionHandler = [&rejectionMessage](const Napi::Error& error) { + rejectionMessage.set_value(error.Message()); + }; + + Babylon::AppRuntime runtime{options}; + + Babylon::ScriptLoader loader{runtime}; + loader.Eval("Promise.reject(new Error('boom from fire-and-forget'));", ""); + + auto future = rejectionMessage.get_future(); + ASSERT_EQ(future.wait_for(std::chrono::seconds(30)), std::future_status::ready) + << "unhandled rejection did not reach the host handler"; + EXPECT_NE(future.get().find("boom from fire-and-forget"), std::string::npos); +} + +TEST(AppRuntime, SynchronouslyHandledRejectionDoesNotReachHandler) +{ + // Only the V8 backend implements unhandled promise rejection tracking (see the note above). +#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"; + } +#endif + + // A rejection that is handled synchronously in the same turn must NOT reach the handler -- + // reporting is deferred to the end of the turn, by which point the .catch has been attached. + Babylon::AppRuntime::Options options{}; + options.EnableUnhandledPromiseRejectionTracking = true; + + std::atomic handlerFired{false}; + options.UnhandledExceptionHandler = [&handlerFired](const Napi::Error&) { + handlerFired = true; + }; + + Babylon::AppRuntime runtime{options}; + + Babylon::ScriptLoader loader{runtime}; + loader.Eval("const p = Promise.reject(new Error('handled')); p.catch(() => {});", ""); + + // Round-trip a dispatch so any deferred rejection-flush task has run before we check. + std::promise drained; + loader.Dispatch([&drained](Napi::Env) { drained.set_value(); }); + drained.get_future().wait(); + + EXPECT_FALSE(handlerFired.load()) << "a synchronously-handled rejection must not reach the host handler"; +} + // The V8JSI Node-API shim does not implement napi_create_dataview / // napi_get_dataview_info (its DataView::New throws "TODO"), so this native test // only builds on the Chakra, V8, and JavaScriptCore backends. The size_t-width From 92de6a4ba9d80bdb2226a306fc25224a88a0ec6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Branimir=20Karad=C5=BEi=C4=87=20=28via=20Copilot=29?= <223556219+Copilot@users.noreply.github.com> Date: Tue, 16 Jun 2026 16:21:37 -0700 Subject: [PATCH 4/6] Hop 4: honor init.signal in fetch and modernize AbortSignal 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/JsRuntimeHost#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 #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> --- Polyfills/AbortController/Readme.md | 15 +++- .../Source/AbortController.cpp | 6 +- .../AbortController/Source/AbortSignal.cpp | 53 +++++++++++- .../AbortController/Source/AbortSignal.h | 17 +++- Polyfills/Fetch/Source/Fetch.cpp | 84 ++++++++++++++++++- Tests/UnitTests/Scripts/tests.ts | 62 ++++++++++++++ 6 files changed, 225 insertions(+), 12 deletions(-) diff --git a/Polyfills/AbortController/Readme.md b/Polyfills/AbortController/Readme.md index 4ec00a7f..f5b8b596 100644 --- a/Polyfills/AbortController/Readme.md +++ b/Polyfills/AbortController/Readme.md @@ -1,10 +1,21 @@ # AbortController Implements parts of [`AbortController`](https://developer.mozilla.org/en-US/docs/Web/API/AbortController/) and [`AbortSignal`](https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal). Provides a way to trigger the abort signal. *Work In Progress* +Supported on `AbortSignal`: +* `aborted` (read-only) and `reason` +* [`throwIfAborted()`](https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/throwIfAborted) +* static [`AbortSignal.abort(reason?)`](https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/abort) +* `onabort`, `addEventListener("abort", ...)`, `removeEventListener` + +`AbortController.abort(reason?)` forwards the reason to the signal; when no reason is given the +signal's `reason` defaults to an `AbortError` (an `Error` whose `name` is `"AbortError"`, since +there is no `DOMException` polyfill). `fetch()` honors an `AbortSignal` passed via `init.signal`: +an already-aborted signal rejects the promise synchronously, and an in-flight abort cancels the +transport and rejects with the signal's `reason`. (Transport cancellation is effective on backends +where `UrlLib::UrlRequest::Abort()` is implemented.) + Currently not implemented: -* [`ThrowIfAborted`](https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/throwIfAborted) * [`Timeout`](https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/timeout) -* [`Abort`](https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/abort) Both the AbortController and AbortSignal polyfills are initialized inside AbortController's initialize method: ```c++ diff --git a/Polyfills/AbortController/Source/AbortController.cpp b/Polyfills/AbortController/Source/AbortController.cpp index 3e6bcb43..0b5068be 100644 --- a/Polyfills/AbortController/Source/AbortController.cpp +++ b/Polyfills/AbortController/Source/AbortController.cpp @@ -25,12 +25,12 @@ namespace Babylon::Polyfills::Internal return m_signal.Value(); } - void AbortController::Abort(const Napi::CallbackInfo&) + void AbortController::Abort(const Napi::CallbackInfo& info) { AbortSignal* sig = AbortSignal::Unwrap(m_signal.Value()); - + assert(sig != nullptr); - sig->Abort(); + sig->Abort(info.Length() > 0 ? info[0] : info.Env().Undefined()); } AbortController::AbortController(const Napi::CallbackInfo& info) diff --git a/Polyfills/AbortController/Source/AbortSignal.cpp b/Polyfills/AbortController/Source/AbortSignal.cpp index e0e77834..c2d44544 100644 --- a/Polyfills/AbortController/Source/AbortSignal.cpp +++ b/Polyfills/AbortController/Source/AbortSignal.cpp @@ -11,10 +11,13 @@ namespace Babylon::Polyfills::Internal env, JS_ABORT_SIGNAL_CONSTRUCTOR_NAME, { - InstanceAccessor("aborted", &AbortSignal::GetAborted, &AbortSignal::SetAborted), + InstanceAccessor("aborted", &AbortSignal::GetAborted, nullptr), + InstanceAccessor("reason", &AbortSignal::GetReason, nullptr), InstanceAccessor("onabort", &AbortSignal::GetOnAbort, &AbortSignal::SetOnAbort), + InstanceMethod("throwIfAborted", &AbortSignal::ThrowIfAborted), InstanceMethod("addEventListener", &AbortSignal::AddEventListener), InstanceMethod("removeEventListener", &AbortSignal::RemoveEventListener), + StaticMethod("abort", &AbortSignal::AbortStatic), }); env.Global().Set(JS_ABORT_SIGNAL_CONSTRUCTOR_NAME, func); @@ -26,10 +29,30 @@ namespace Babylon::Polyfills::Internal { } - void AbortSignal::Abort() + Napi::Value AbortSignal::CreateAbortError(Napi::Env env, const char* message) { + // There is no DOMException polyfill, so represent the abort reason as an Error whose `name` + // is "AbortError" -- the value web code checks (`err.name === "AbortError"`). + Napi::Error error = Napi::Error::New(env, message); + error.Set("name", Napi::String::New(env, "AbortError")); + return error.Value(); + } + + void AbortSignal::Abort(const Napi::Value& reason) + { + if (m_aborted) + { + return; + } + m_aborted = true; + Napi::Env env = Env(); + const Napi::Value resolvedReason = (reason.IsUndefined() || reason.IsEmpty()) + ? CreateAbortError(env, "The operation was aborted.") + : reason; + m_reason = Napi::Persistent(resolvedReason); + auto onabort = m_onabort.Value(); if (!onabort.IsNull() && !onabort.IsUndefined()) { @@ -39,14 +62,36 @@ namespace Babylon::Polyfills::Internal RaiseEvent("abort"); } + Napi::Value AbortSignal::AbortStatic(const Napi::CallbackInfo& info) + { + Napi::Env env = info.Env(); + Napi::Object signalObject = env.Global().Get(JS_ABORT_SIGNAL_CONSTRUCTOR_NAME).As().New({}); + AbortSignal* signal = AbortSignal::Unwrap(signalObject); + signal->Abort(info.Length() > 0 ? info[0] : env.Undefined()); + return signalObject; + } + Napi::Value AbortSignal::GetAborted(const Napi::CallbackInfo&) { return Napi::Value::From(Env(), m_aborted); } - void AbortSignal::SetAborted(const Napi::CallbackInfo&, const Napi::Value& value) + Napi::Value AbortSignal::GetReason(const Napi::CallbackInfo&) { - m_aborted = value.As(); + if (m_reason.IsEmpty()) + { + return Env().Undefined(); + } + + return m_reason.Value(); + } + + void AbortSignal::ThrowIfAborted(const Napi::CallbackInfo& info) + { + if (m_aborted) + { + throw Napi::Error{info.Env(), GetReason(info)}; + } } Napi::Value AbortSignal::GetOnAbort(const Napi::CallbackInfo&) diff --git a/Polyfills/AbortController/Source/AbortSignal.h b/Polyfills/AbortController/Source/AbortSignal.h index a2de0e4f..a2888c80 100644 --- a/Polyfills/AbortController/Source/AbortSignal.h +++ b/Polyfills/AbortController/Source/AbortSignal.h @@ -17,11 +17,23 @@ namespace Babylon::Polyfills::Internal static void Initialize(Napi::Env env); explicit AbortSignal(const Napi::CallbackInfo& info); - void Abort(); + // Transition the signal to the aborted state with the given reason (undefined -> a default + // "AbortError"), firing onabort and any "abort" listeners. No-op if already aborted. + void Abort(const Napi::Value& reason); + + // Build the default abort reason: an Error whose name is "AbortError" (there is no + // DOMException polyfill), matching what the platform uses when abort() is called with no + // reason and what fetch() rejects with on abort. + static Napi::Value CreateAbortError(Napi::Env env, const char* message); private: Napi::Value GetAborted(const Napi::CallbackInfo& info); - void SetAborted(const Napi::CallbackInfo&, const Napi::Value& value); + + Napi::Value GetReason(const Napi::CallbackInfo& info); + void ThrowIfAborted(const Napi::CallbackInfo& info); + + // AbortSignal.abort(reason?) -- returns an AbortSignal already in the aborted state. + static Napi::Value AbortStatic(const Napi::CallbackInfo& info); Napi::Value GetOnAbort(const Napi::CallbackInfo& info); void SetOnAbort(const Napi::CallbackInfo&, const Napi::Value& value); @@ -33,6 +45,7 @@ namespace Babylon::Polyfills::Internal std::unordered_map> m_eventHandlerRefs; Napi::FunctionReference m_onabort; + Napi::Reference m_reason; bool m_aborted = false; }; } \ No newline at end of file diff --git a/Polyfills/Fetch/Source/Fetch.cpp b/Polyfills/Fetch/Source/Fetch.cpp index 24c2114f..7a500df7 100644 --- a/Polyfills/Fetch/Source/Fetch.cpp +++ b/Polyfills/Fetch/Source/Fetch.cpp @@ -30,6 +30,32 @@ namespace Babylon::Polyfills::Internal std::vector body; }; + // Shared state for honoring an AbortSignal passed via init.signal. Co-owned by the "abort" + // listener (which sets the flag, captures the reason, and cancels the transport) and the + // completion continuation (which reports the AbortError and tears the listener down). + struct AbortState + { + bool aborted{false}; + Napi::Reference reason; + Napi::ObjectReference signal; + Napi::FunctionReference listener; + }; + + // The reason a fetch was aborted: the signal's `reason` (per the modern AbortSignal), or a + // fresh AbortError if the signal does not expose one. + Napi::Value GetAbortReason(Napi::Env env, const Napi::Object& signal) + { + const Napi::Value reason = signal.Get("reason"); + if (!reason.IsUndefined() && !reason.IsNull()) + { + return reason; + } + + Napi::Error error = Napi::Error::New(env, "The operation was aborted."); + error.Set("name", Napi::String::New(env, "AbortError")); + return error.Value(); + } + bool EqualsIgnoreCase(std::string_view a, std::string_view b) { return std::equal(a.begin(), a.end(), b.begin(), b.end(), [](unsigned char l, unsigned char r) { @@ -352,6 +378,7 @@ namespace Babylon::Polyfills::Internal UrlLib::UrlMethod method = UrlLib::UrlMethod::Get; std::optional body; Napi::Value headers = env.Undefined(); + Napi::Value signal = env.Undefined(); if (info.Length() > 1 && info[1].IsObject()) { @@ -374,6 +401,7 @@ namespace Babylon::Polyfills::Internal } headers = init.Get("headers"); + signal = init.Get("signal"); } auto request = std::make_shared(); @@ -390,6 +418,39 @@ namespace Babylon::Polyfills::Internal // site rather than to an empty scheduler tick. const std::string capturedStack = CaptureCallSiteStack(env); + // Honor an AbortSignal passed via init.signal (WHATWG fetch). The signal is used + // through its JS interface (aborted / reason / add/removeEventListener) so fetch + // stays decoupled from the AbortController polyfill's C++ types. + std::shared_ptr abortState; + if (signal.IsObject()) + { + const Napi::Object signalObject = signal.As(); + + // Already aborted: reject synchronously with the signal's reason, never + // touching the transport. + if (signalObject.Get("aborted").ToBoolean().Value()) + { + deferred.Reject(GetAbortReason(env, signalObject)); + return deferred.Promise(); + } + + abortState = std::make_shared(); + abortState->signal = Napi::Persistent(signalObject); + + Napi::Function listener = Napi::Function::New(env, [abortState, request, env](const Napi::CallbackInfo&) { + if (!abortState->aborted) + { + abortState->aborted = true; + abortState->reason = Napi::Persistent(GetAbortReason(env, abortState->signal.Value())); + // Cancel the in-flight transport; the completion continuation then + // rejects with the AbortError instead of a transport TypeError. + request->Abort(); + } + }); + abortState->listener = Napi::Persistent(listener); + signalObject.Get("addEventListener").As().Call(signalObject, {Napi::String::New(env, "abort"), listener}); + } + // arcana::task::then captures the scheduler by reference (see arcana task.h) and // invokes it on the worker thread when the request completes -- after this fetch() // call has returned. A stack-local scheduler would therefore dangle. Heap-allocate @@ -397,7 +458,28 @@ namespace Babylon::Polyfills::Internal auto scheduler = std::make_shared(JsRuntime::GetFromJavaScript(env)); request->SendAsync() .then(*scheduler, arcana::cancellation::none(), - [deferred, request, env, url, capturedStack](const arcana::expected& result) { + [deferred, request, env, url, capturedStack, abortState](const arcana::expected& result) { + // The request has settled: stop listening for aborts (breaking the + // listener <-> abortState ownership cycle) before deciding the outcome. + if (abortState) + { + if (!abortState->signal.IsEmpty() && !abortState->listener.IsEmpty()) + { + Napi::Object signalObject = abortState->signal.Value(); + signalObject.Get("removeEventListener").As().Call(signalObject, {Napi::String::New(env, "abort"), abortState->listener.Value()}); + } + abortState->listener.Reset(); + abortState->signal.Reset(); + + if (abortState->aborted) + { + // Per the fetch spec, an aborted request rejects with the + // signal's reason (an AbortError), not a network error. + deferred.Reject(abortState->reason.Value()); + return; + } + } + const int status = static_cast(request->StatusCode()); // Per the WHATWG fetch spec, only transport-level failures reject. A completed diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index 23cc76f0..20e17de6 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -64,6 +64,37 @@ describe("AbortController", function () { expect(controller.signal.aborted).to.equal(true); }); + + it("AbortSignal.abort() returns a signal already aborted with an AbortError reason", function () { + const signal = (AbortSignal as any).abort(); + expect(signal.aborted).to.equal(true); + expect(signal.reason).to.be.an.instanceof(Error); + expect(signal.reason.name).to.equal("AbortError"); + }); + + it("throwIfAborted() throws the reason only once aborted", function () { + const controller = new AbortController(); + // Not aborted yet: must not throw. + (controller.signal as any).throwIfAborted(); + + controller.abort(); + expect(() => (controller.signal as any).throwIfAborted()).to.throw(); + }); + + it("abort(reason) records the provided reason", function () { + const controller = new AbortController(); + const reason = new Error("custom reason"); + controller.abort(reason); + expect((controller.signal as any).reason).to.equal(reason); + }); + + it("abort() with no reason defaults to an AbortError", function () { + const controller = new AbortController(); + controller.abort(); + const reason = (controller.signal as any).reason; + expect(reason).to.be.an.instanceof(Error); + expect(reason.name).to.equal("AbortError"); + }); }); describe("XMLHTTPRequest", function () { @@ -389,6 +420,37 @@ describe("fetch", function () { expect(error.message).to.equal("fetch failed"); expect(error.cause.url).to.contain("does_not_exist.js"); }); + + it("should reject immediately with an AbortError when the signal is already aborted", async function () { + const controller = new AbortController(); + controller.abort(); + + let error: any; + try { + await fetch("https://github.com/", { signal: controller.signal } as any); + } catch (e) { + error = e; + } + expect(error, "fetch should have rejected").to.not.equal(undefined); + expect(error.name).to.equal("AbortError"); + }); + + it("should reject with an AbortError when aborted in-flight", async function () { + this.timeout(30000); + const controller = new AbortController(); + const promise = fetch("https://github.com/", { signal: controller.signal } as any); + // Abort before the response can arrive. + controller.abort(); + + let error: any; + try { + await promise; + } catch (e) { + error = e; + } + expect(error, "fetch should have rejected").to.not.equal(undefined); + expect(error.name).to.equal("AbortError"); + }); }); describe("setTimeout", function () { From 97e2c6854642c00a66c22b2665b90897a97c87e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Branimir=20Karad=C5=BEi=C4=87=20=28via=20Copilot=29?= <223556219+Copilot@users.noreply.github.com> Date: Thu, 18 Jun 2026 17:35:14 -0700 Subject: [PATCH 5/6] Address review (hop 3): implement JavaScriptCore, always-track, robustness 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_ (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> --- Core/AppRuntime/Include/Babylon/AppRuntime.h | 27 ++--- Core/AppRuntime/Source/AppRuntime_Chakra.cpp | 7 +- .../Source/AppRuntime_JavaScriptCore.cpp | 54 +++++++++ .../Source/AppRuntime_PromiseRejection.h | 85 ++++++++++++++ Core/AppRuntime/Source/AppRuntime_V8.cpp | 105 ++++++------------ .../Android/app/src/main/cpp/CMakeLists.txt | 2 + Tests/UnitTests/CMakeLists.txt | 13 ++- Tests/UnitTests/Shared/Shared.cpp | 35 +++--- 8 files changed, 215 insertions(+), 113 deletions(-) create mode 100644 Core/AppRuntime/Source/AppRuntime_PromiseRejection.h diff --git a/Core/AppRuntime/Include/Babylon/AppRuntime.h b/Core/AppRuntime/Include/Babylon/AppRuntime.h index cab418e6..6d5b6e40 100644 --- a/Core/AppRuntime/Include/Babylon/AppRuntime.h +++ b/Core/AppRuntime/Include/Babylon/AppRuntime.h @@ -21,15 +21,6 @@ namespace Babylon // Optional handler for unhandled exceptions. std::function UnhandledExceptionHandler{DefaultUnhandledExceptionHandler}; - // When true, unhandled promise rejections are routed to UnhandledExceptionHandler so - // the embedder's crash/telemetry pipeline can observe fire-and-forget failures (e.g. an - // un-awaited fetch() that rejects). Defaults to false to preserve existing behavior -- - // when false, no per-engine rejection tracker is installed. Reporting is deferred to the - // end of the current turn, so a rejection that is handled synchronously (e.g. - // `const p = Promise.reject(e); p.catch(...)`) does not reach the handler. Implemented for - // the Chakra and V8 engines. - bool EnableUnhandledPromiseRejectionTracking{false}; - // Defines whether to enable the debugger. Only implemented for V8 and Chakra. bool EnableDebugger{false}; @@ -54,10 +45,20 @@ namespace Babylon void Dispatch(Dispatchable callback); - // Routes an unhandled promise rejection to the embedder's UnhandledExceptionHandler. Called - // by the per-engine promise-rejection tracker installed in RunEnvironmentTier when - // Options::EnableUnhandledPromiseRejectionTracking is set. Intended for internal - // (engine-implementation) use. + // Routes an unhandled promise rejection to the embedder's UnhandledExceptionHandler (which + // defaults to a benign logger), so an embedder's crash/telemetry pipeline can observe + // fire-and-forget failures (e.g. an un-awaited fetch() that rejects) -- matching the browser + // `unhandledrejection` behavior. Reporting is deferred to the end of the turn, so a rejection + // handled synchronously (e.g. `const p = Promise.reject(e); p.catch(...)`) is not reported. + // + // Coverage is determined by whether the engine exposes a host promise-rejection hook: + // * V8 (Isolate::SetPromiseRejectCallback) and JavaScriptCore + // (JSGlobalContextSetUnhandledRejectionCallback) -- supported. + // * Chakra (in-box/EdgeMode) and JSI -- no-op: neither exposes such a hook + // (JsSetHostPromiseRejectionTracker is ChakraCore-only, and neither jsi::Runtime nor + // V8JSI surfaces the V8 callback). + // + // Intended for internal (engine-implementation) use. void OnUnhandledPromiseRejection(const Napi::Error& error); // Default unhandled exception handler that outputs the error message to the program output. diff --git a/Core/AppRuntime/Source/AppRuntime_Chakra.cpp b/Core/AppRuntime/Source/AppRuntime_Chakra.cpp index 09fcb2ed..50e998f1 100644 --- a/Core/AppRuntime/Source/AppRuntime_Chakra.cpp +++ b/Core/AppRuntime/Source/AppRuntime_Chakra.cpp @@ -51,10 +51,9 @@ namespace Babylon }, &dispatchFunction)); - // 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. + // Unhandled promise rejection tracking (OnUnhandledPromiseRejection) is a no-op on this + // backend: the OS EdgeMode Chakra runtime (chakrart.h) exposes no host promise-rejection + // hook (JsSetHostPromiseRejectionTracker is ChakraCore-only). See AppRuntime.h. ThrowIfFailed(JsProjectWinRTNamespace(L"Windows")); diff --git a/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp b/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp index a0322898..978f7260 100644 --- a/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp +++ b/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp @@ -1,8 +1,49 @@ #include "AppRuntime.h" +#include "AppRuntime_PromiseRejection.h" #include +// JSGlobalContextSetUnhandledRejectionCallback is declared in the private JavaScriptCore header +// , which is not part of the public macOS/iOS SDK. The symbol +// is exported by the JavaScriptCore framework (SPI), so it is forward-declared here and the call is +// guarded with __builtin_available (it is JSC_API_AVAILABLE(macos(10.15.4), ios(13.4))). It registers +// a JS function invoked at the microtask checkpoint with (promise, reason) for each promise that is +// still unhandled at that point, so -- unlike V8 -- no deferral or candidate bookkeeping is needed. +extern "C" void JSGlobalContextSetUnhandledRejectionCallback(JSGlobalContextRef ctx, JSObjectRef function, JSValueRef* exception); + namespace Babylon { + namespace + { + // JSObjectMakeFunctionWithCallback takes no user-data argument; each AppRuntime owns its JSC + // context on a dedicated thread, so a thread_local associates the callback with this runtime. + struct JSCRejectionContext + { + AppRuntime* runtime{}; + napi_env env{}; + }; + + thread_local JSCRejectionContext* t_rejectionContext{nullptr}; + + // Mirrors ToNapi (js_native_api_javascriptcore.cc): napi_value is a JSValueRef in the + // JavaScriptCore Node-API shim. + napi_value JsValueToNapi(JSValueRef value) + { + return reinterpret_cast(const_cast(value)); + } + + JSValueRef OnUnhandledRejection(JSContextRef ctx, JSObjectRef, JSObjectRef, size_t argumentCount, const JSValueRef arguments[], JSValueRef*) + { + JSCRejectionContext* context{t_rejectionContext}; + if (context != nullptr && argumentCount >= 2) + { + const Napi::Env env{context->env}; + context->runtime->OnUnhandledPromiseRejection(Internal::ToError(env, JsValueToNapi(arguments[1]))); + } + + return JSValueMakeUndefined(ctx); + } + } + void AppRuntime::RunEnvironmentTier(const char*) { auto globalContext = JSGlobalContextCreateInGroup(nullptr, nullptr); @@ -16,8 +57,21 @@ namespace Babylon Napi::Env env = Napi::Attach(globalContext); + // Always track unhandled promise rejections (routed to the host UnhandledExceptionHandler). + JSCRejectionContext rejectionContext{this, env}; + t_rejectionContext = &rejectionContext; + if (__builtin_available(iOS 13.4, macOS 10.15.4, *)) + { + JSStringRef callbackName = JSStringCreateWithUTF8CString("onUnhandledRejection"); + JSObjectRef callback = JSObjectMakeFunctionWithCallback(globalContext, callbackName, OnUnhandledRejection); + JSStringRelease(callbackName); + JSGlobalContextSetUnhandledRejectionCallback(globalContext, callback, nullptr); + } + Run(env); + t_rejectionContext = nullptr; + JSGlobalContextRelease(globalContext); // Detach must come after JSGlobalContextRelease since it triggers finalizers which require env. diff --git a/Core/AppRuntime/Source/AppRuntime_PromiseRejection.h b/Core/AppRuntime/Source/AppRuntime_PromiseRejection.h new file mode 100644 index 00000000..6121561b --- /dev/null +++ b/Core/AppRuntime/Source/AppRuntime_PromiseRejection.h @@ -0,0 +1,85 @@ +#pragma once + +#include "AppRuntime.h" + +#include + +#include +#include +#include + +namespace Babylon::Internal +{ + // Wraps an unhandled-promise rejection reason as a Napi::Error: an Error-like object passes + // through (preserving message/stack/cause); any other value is stringified so the host handler + // always receives a Napi::Error. Lives here rather than in shared AppRuntime.cpp because the + // napi_value -> Napi::Value bridge is unavailable on the JSI Node-API shim, and only the engines + // that support rejection tracking (V8, JavaScriptCore) include this header. + inline Napi::Error ToError(Napi::Env env, napi_value reason) + { + const Napi::Value value{env, reason}; + return value.IsObject() + ? Napi::Error{env, reason} + : Napi::Error::New(env, value.ToString().Utf8Value()); + } + + // Engine-agnostic bookkeeping for engines that report an unhandled rejection immediately and a + // later handler-added event separately (V8): collect candidates as promises reject without a + // handler, drop them when a handler is attached, and report the survivors to the host handler at + // the end of the current turn -- so a rejection handled synchronously within the same turn is + // never reported. Engines whose host hook already fires only for still-unhandled rejections at + // the microtask checkpoint (JavaScriptCore) report directly and do not need this. + // + // CandidateT is engine-specific and must provide: + // void Report(AppRuntime& runtime, Napi::Env env) const; + // // convert its stored reason to a Napi::Error (via ToError) and call + // // runtime.OnUnhandledPromiseRejection + template + class PromiseRejectionTracker + { + public: + explicit PromiseRejectionTracker(AppRuntime& runtime) + : m_runtime{runtime} + { + } + + void Add(CandidateT candidate) + { + m_candidates.push_back(std::move(candidate)); + + if (!m_flushScheduled) + { + m_flushScheduled = true; + m_runtime.Dispatch([this](Napi::Env env) { Flush(env); }); + } + } + + template + void Remove(PredicateT predicate) + { + m_candidates.erase( + std::remove_if(m_candidates.begin(), m_candidates.end(), std::move(predicate)), + m_candidates.end()); + } + + private: + void Flush(Napi::Env env) + { + m_flushScheduled = false; + + // Move the candidates out before reporting: a host handler could synchronously reject + // another promise, re-entering Add() and mutating m_candidates mid-iteration. + const std::vector candidates = std::move(m_candidates); + m_candidates.clear(); + + for (const CandidateT& candidate : candidates) + { + candidate.Report(m_runtime, env); + } + } + + AppRuntime& m_runtime; + std::vector m_candidates; + bool m_flushScheduled{false}; + }; +} diff --git a/Core/AppRuntime/Source/AppRuntime_V8.cpp b/Core/AppRuntime/Source/AppRuntime_V8.cpp index 6a4b8958..2847797f 100644 --- a/Core/AppRuntime/Source/AppRuntime_V8.cpp +++ b/Core/AppRuntime/Source/AppRuntime_V8.cpp @@ -1,4 +1,5 @@ #include "AppRuntime.h" +#include "AppRuntime_PromiseRejection.h" #include #include @@ -8,8 +9,6 @@ #endif #include -#include -#include namespace Babylon { @@ -64,26 +63,6 @@ namespace Babylon std::unique_ptr Module::s_module; - // Tracks promises rejected without a handler so they can be reported once the current turn - // settles. V8 fires kPromiseRejectWithNoHandler when a promise is rejected with no handler - // and kPromiseHandlerAddedAfterReject if one is attached afterwards; reporting is deferred - // (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 - { - AppRuntime* runtime{}; - v8::Isolate* isolate{}; - std::unordered_map, v8::Global>> unhandled{}; - bool flushScheduled{false}; - }; - - // The promise-rejection callback is a bare function pointer with no user-data argument. Each - // AppRuntime owns a dedicated isolate running on its own thread, and V8 invokes the callback - // on that thread, so a thread_local pointer associates the callback with the right tracker - // without risking isolate-data-slot collisions with the Node-API shim. - thread_local V8RejectionTracker* t_rejectionTracker{nullptr}; - // Mirrors v8impl::JsValueFromV8LocalValue (js_native_api_v8.h), which is internal to the // Node-API V8 shim and not on the public include path. static_assert(sizeof(v8::Local) == sizeof(napi_value), @@ -93,31 +72,30 @@ namespace Babylon return reinterpret_cast(*local); } - // Wrap a rejection reason as a Napi::Error. An Error-like object is forwarded as-is - // (preserving message/stack/cause); any other value is stringified so the embedder's handler - // always receives a Napi::Error. Done here (not in shared code) because the napi_value -> - // Napi::Value bridge is specific to the V8/standard Node-API shim. - Napi::Error ToError(Napi::Env env, napi_value reason) + // A promise rejected without a handler, awaiting end-of-turn reporting. The promise and + // reason are held in v8::Global handles so they survive until the deferred flush; the promise + // is retained so a later handler-added event can drop this candidate by object identity + // (v8::Object::GetIdentityHash is not unique, so identity comparison is used instead). + struct V8RejectionCandidate { - const Napi::Value reasonValue{env, reason}; - return reasonValue.IsObject() - ? Napi::Error{env, reason} - : Napi::Error::New(env, reasonValue.ToString().Utf8Value()); - } - - void FlushUnhandledRejections(V8RejectionTracker& tracker, Napi::Env env) - { - tracker.flushScheduled = false; + v8::Isolate* isolate{}; + v8::Global promise; + v8::Global reason; - v8::Isolate::Scope isolateScope{tracker.isolate}; - v8::HandleScope handleScope{tracker.isolate}; - for (auto& entry : tracker.unhandled) + void Report(AppRuntime& runtime, Napi::Env env) const { - const v8::Local reason = entry.second.second.Get(tracker.isolate); - tracker.runtime->OnUnhandledPromiseRejection(ToError(env, JsValueFromV8LocalValue(reason))); + v8::HandleScope handleScope{isolate}; + runtime.OnUnhandledPromiseRejection(Internal::ToError(env, JsValueFromV8LocalValue(reason.Get(isolate)))); } - tracker.unhandled.clear(); - } + }; + + using V8RejectionTracker = Internal::PromiseRejectionTracker; + + // The promise-rejection callback is a bare function pointer with no user-data argument. Each + // AppRuntime owns a dedicated isolate running on its own thread, and V8 invokes the callback + // on that thread, so a thread_local pointer associates the callback with the right tracker + // without risking isolate-data-slot collisions with the Node-API shim. + thread_local V8RejectionTracker* t_rejectionTracker{nullptr}; void OnPromiseReject(v8::PromiseRejectMessage message) { @@ -127,31 +105,25 @@ namespace Babylon return; } - v8::HandleScope handleScope{tracker->isolate}; + v8::Isolate* isolate{v8::Isolate::GetCurrent()}; + v8::HandleScope handleScope{isolate}; const v8::Local promise{message.GetPromise()}; - const int hash{promise->GetIdentityHash()}; switch (message.GetEvent()) { case v8::kPromiseRejectWithNoHandler: { - const v8::Local reason{message.GetValue()}; - tracker->unhandled[hash] = { - v8::Global{tracker->isolate, promise}, - v8::Global{tracker->isolate, reason}}; - - if (!tracker->flushScheduled) - { - tracker->flushScheduled = true; - tracker->runtime->Dispatch([tracker](Napi::Env env) { - FlushUnhandledRejections(*tracker, env); - }); - } + tracker->Add(V8RejectionCandidate{ + isolate, + v8::Global{isolate, promise}, + v8::Global{isolate, message.GetValue()}}); break; } case v8::kPromiseHandlerAddedAfterReject: { - tracker->unhandled.erase(hash); + tracker->Remove([isolate, promise](const V8RejectionCandidate& candidate) { + return candidate.promise.Get(isolate) == promise; + }); break; } default: @@ -182,12 +154,10 @@ namespace Babylon Napi::Env env = Napi::Attach(context); - V8RejectionTracker rejectionTracker{this, isolate, {}, false}; - if (m_options.EnableUnhandledPromiseRejectionTracking) - { - t_rejectionTracker = &rejectionTracker; - isolate->SetPromiseRejectCallback(OnPromiseReject); - } + // Always track unhandled promise rejections (routed to the host UnhandledExceptionHandler). + V8RejectionTracker rejectionTracker{*this}; + t_rejectionTracker = &rejectionTracker; + isolate->SetPromiseRejectCallback(OnPromiseReject); #ifdef ENABLE_V8_INSPECTOR std::optional agent; @@ -212,11 +182,8 @@ namespace Babylon } #endif - if (m_options.EnableUnhandledPromiseRejectionTracking) - { - isolate->SetPromiseRejectCallback(nullptr); - t_rejectionTracker = nullptr; - } + isolate->SetPromiseRejectCallback(nullptr); + t_rejectionTracker = nullptr; Napi::Detach(env); } diff --git a/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt b/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt index c870151c..54dbda87 100644 --- a/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt +++ b/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt @@ -23,6 +23,8 @@ add_library(UnitTestsJNI SHARED target_compile_definitions(UnitTestsJNI PRIVATE JSRUNTIMEHOST_PLATFORM="${JSRUNTIMEHOST_PLATFORM}") target_compile_definitions(UnitTestsJNI PRIVATE JSRUNTIMEHOST_NAPI_ENGINE="${NAPI_JAVASCRIPT_ENGINE}") +# Engine-specific compile-time gate (e.g. JSRUNTIMEHOST_NAPI_ENGINE_V8), matching Tests/UnitTests/CMakeLists.txt. +target_compile_definitions(UnitTestsJNI PRIVATE JSRUNTIMEHOST_NAPI_ENGINE_${NAPI_JAVASCRIPT_ENGINE}) target_compile_definitions(UnitTestsJNI PRIVATE ARCANA_TEST_HOOKS) target_include_directories(UnitTestsJNI diff --git a/Tests/UnitTests/CMakeLists.txt b/Tests/UnitTests/CMakeLists.txt index c12b12f8..ea8c6c82 100644 --- a/Tests/UnitTests/CMakeLists.txt +++ b/Tests/UnitTests/CMakeLists.txt @@ -46,12 +46,13 @@ endif() add_executable(UnitTests ${SOURCES} ${SCRIPTS} ${TYPE_SCRIPTS} ${ASSETS}) target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_PLATFORM="${JSRUNTIMEHOST_PLATFORM}") target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_NAPI_ENGINE="${NAPI_JAVASCRIPT_ENGINE}") - -# The V8JSI Node-API shim does not implement napi_create_dataview, so the -# CreateDataViewRejectsOverflowingRange test is compiled out on that backend. -if(NAPI_JAVASCRIPT_ENGINE STREQUAL "JSI") - target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_NAPI_ENGINE_JSI) -endif() +# Engine-specific compile-time gate so tests can select behavior by the active Node-API engine +# without a runtime string comparison, e.g.: +# JSRUNTIMEHOST_NAPI_ENGINE_V8, JSRUNTIMEHOST_NAPI_ENGINE_JavaScriptCore, +# JSRUNTIMEHOST_NAPI_ENGINE_Chakra, JSRUNTIMEHOST_NAPI_ENGINE_JSI. +# (JSI compiles out CreateDataViewRejectsOverflowingRange since the V8JSI shim lacks +# napi_create_dataview; V8/JavaScriptCore gate the unhandled-rejection handler tests.) +target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_NAPI_ENGINE_${NAPI_JAVASCRIPT_ENGINE}) target_link_libraries(UnitTests PRIVATE AppRuntime diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 8530b04c..20bcd833 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -282,20 +282,16 @@ TEST(AppRuntime, DestroyDoesNotDeadlock) TEST(AppRuntime, UnhandledPromiseRejectionReachesHandler) { - // EnableUnhandledPromiseRejectionTracking is only implemented on the V8 backend so far (the OS - // EdgeMode Chakra runtime exposes no host promise-rejection hook; JavaScriptCore/JSI are - // follow-ups). Skip elsewhere so the test doesn't hang waiting for a report that never comes. -#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"; - } -#endif - + // Unhandled promise rejection tracking is implemented on the engines that expose a host + // promise-rejection hook: V8 (Isolate::SetPromiseRejectCallback) and JavaScriptCore + // (JSGlobalContextSetUnhandledRejectionCallback). The OS EdgeMode Chakra runtime and the V8JSI + // (JSI) shim expose no such hook, so the body is compiled out there and the test is skipped. +#if !defined(JSRUNTIMEHOST_NAPI_ENGINE_V8) && !defined(JSRUNTIMEHOST_NAPI_ENGINE_JavaScriptCore) + GTEST_SKIP() << "unhandled promise rejection tracking is only implemented for the V8 and JavaScriptCore backends"; +#else // A fire-and-forget rejected promise (no handler ever attached) must reach the embedder's - // UnhandledExceptionHandler when EnableUnhandledPromiseRejectionTracking is set. + // UnhandledExceptionHandler. Babylon::AppRuntime::Options options{}; - options.EnableUnhandledPromiseRejectionTracking = true; std::promise rejectionMessage; options.UnhandledExceptionHandler = [&rejectionMessage](const Napi::Error& error) { @@ -311,22 +307,18 @@ TEST(AppRuntime, UnhandledPromiseRejectionReachesHandler) ASSERT_EQ(future.wait_for(std::chrono::seconds(30)), std::future_status::ready) << "unhandled rejection did not reach the host handler"; EXPECT_NE(future.get().find("boom from fire-and-forget"), std::string::npos); +#endif } TEST(AppRuntime, SynchronouslyHandledRejectionDoesNotReachHandler) { - // Only the V8 backend implements unhandled promise rejection tracking (see the note above). -#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"; - } -#endif - + // Only engines with a host promise-rejection hook implement this tracking (see the note above). +#if !defined(JSRUNTIMEHOST_NAPI_ENGINE_V8) && !defined(JSRUNTIMEHOST_NAPI_ENGINE_JavaScriptCore) + GTEST_SKIP() << "unhandled promise rejection tracking is only implemented for the V8 and JavaScriptCore backends"; +#else // A rejection that is handled synchronously in the same turn must NOT reach the handler -- // reporting is deferred to the end of the turn, by which point the .catch has been attached. Babylon::AppRuntime::Options options{}; - options.EnableUnhandledPromiseRejectionTracking = true; std::atomic handlerFired{false}; options.UnhandledExceptionHandler = [&handlerFired](const Napi::Error&) { @@ -344,6 +336,7 @@ TEST(AppRuntime, SynchronouslyHandledRejectionDoesNotReachHandler) drained.get_future().wait(); EXPECT_FALSE(handlerFired.load()) << "a synchronously-handled rejection must not reach the host handler"; +#endif } // The V8JSI Node-API shim does not implement napi_create_dataview / From b2553a26b3abb5a0e001fd08f450b3e97f9abe14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Branimir=20Karad=C5=BEi=C4=87=20=28via=20Copilot=29?= <223556219+Copilot@users.noreply.github.com> Date: Thu, 18 Jun 2026 17:42:43 -0700 Subject: [PATCH 6/6] Guard the JSC unhandled-rejection SPI to Apple platforms 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> --- Core/AppRuntime/Include/Babylon/AppRuntime.h | 6 ++++-- .../Source/AppRuntime_JavaScriptCore.cpp | 13 ++++++++++++- Tests/UnitTests/Shared/Shared.cpp | 15 ++++++++------- 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/Core/AppRuntime/Include/Babylon/AppRuntime.h b/Core/AppRuntime/Include/Babylon/AppRuntime.h index 6d5b6e40..654da357 100644 --- a/Core/AppRuntime/Include/Babylon/AppRuntime.h +++ b/Core/AppRuntime/Include/Babylon/AppRuntime.h @@ -52,8 +52,10 @@ namespace Babylon // handled synchronously (e.g. `const p = Promise.reject(e); p.catch(...)`) is not reported. // // Coverage is determined by whether the engine exposes a host promise-rejection hook: - // * V8 (Isolate::SetPromiseRejectCallback) and JavaScriptCore - // (JSGlobalContextSetUnhandledRejectionCallback) -- supported. + // * V8 (Isolate::SetPromiseRejectCallback) -- supported on all platforms. + // * Apple JavaScriptCore (JSGlobalContextSetUnhandledRejectionCallback) -- supported. This + // is an SPI present only in Apple's JSC; the WebKitGTK JSC used on Linux does not expose + // it, so tracking is a no-op there. // * Chakra (in-box/EdgeMode) and JSI -- no-op: neither exposes such a hook // (JsSetHostPromiseRejectionTracker is ChakraCore-only, and neither jsi::Runtime nor // V8JSI surfaces the V8 callback). diff --git a/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp b/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp index 978f7260..5215c341 100644 --- a/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp +++ b/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp @@ -1,17 +1,23 @@ #include "AppRuntime.h" -#include "AppRuntime_PromiseRejection.h" #include +#if __APPLE__ +#include "AppRuntime_PromiseRejection.h" + // JSGlobalContextSetUnhandledRejectionCallback is declared in the private JavaScriptCore header // , which is not part of the public macOS/iOS SDK. The symbol // is exported by the JavaScriptCore framework (SPI), so it is forward-declared here and the call is // guarded with __builtin_available (it is JSC_API_AVAILABLE(macos(10.15.4), ios(13.4))). It registers // a JS function invoked at the microtask checkpoint with (promise, reason) for each promise that is // still unhandled at that point, so -- unlike V8 -- no deferral or candidate bookkeeping is needed. +// This is Apple-only: the WebKitGTK JavaScriptCore used on Linux exposes neither this SPI nor +// __builtin_available, so unhandled-rejection tracking is a no-op there (see AppRuntime.h). extern "C" void JSGlobalContextSetUnhandledRejectionCallback(JSGlobalContextRef ctx, JSObjectRef function, JSValueRef* exception); +#endif namespace Babylon { +#if __APPLE__ namespace { // JSObjectMakeFunctionWithCallback takes no user-data argument; each AppRuntime owns its JSC @@ -43,6 +49,7 @@ namespace Babylon return JSValueMakeUndefined(ctx); } } +#endif void AppRuntime::RunEnvironmentTier(const char*) { @@ -57,6 +64,7 @@ namespace Babylon Napi::Env env = Napi::Attach(globalContext); +#if __APPLE__ // Always track unhandled promise rejections (routed to the host UnhandledExceptionHandler). JSCRejectionContext rejectionContext{this, env}; t_rejectionContext = &rejectionContext; @@ -67,10 +75,13 @@ namespace Babylon JSStringRelease(callbackName); JSGlobalContextSetUnhandledRejectionCallback(globalContext, callback, nullptr); } +#endif Run(env); +#if __APPLE__ t_rejectionContext = nullptr; +#endif JSGlobalContextRelease(globalContext); diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 20bcd833..e773a57b 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -283,11 +283,12 @@ TEST(AppRuntime, DestroyDoesNotDeadlock) TEST(AppRuntime, UnhandledPromiseRejectionReachesHandler) { // Unhandled promise rejection tracking is implemented on the engines that expose a host - // promise-rejection hook: V8 (Isolate::SetPromiseRejectCallback) and JavaScriptCore - // (JSGlobalContextSetUnhandledRejectionCallback). The OS EdgeMode Chakra runtime and the V8JSI - // (JSI) shim expose no such hook, so the body is compiled out there and the test is skipped. -#if !defined(JSRUNTIMEHOST_NAPI_ENGINE_V8) && !defined(JSRUNTIMEHOST_NAPI_ENGINE_JavaScriptCore) - GTEST_SKIP() << "unhandled promise rejection tracking is only implemented for the V8 and JavaScriptCore backends"; + // promise-rejection hook: V8 (Isolate::SetPromiseRejectCallback) and Apple JavaScriptCore + // (JSGlobalContextSetUnhandledRejectionCallback, an SPI absent from WebKitGTK/Linux JSC). The OS + // EdgeMode Chakra runtime and the V8JSI (JSI) shim expose no such hook, so the body is compiled + // out there (and on non-Apple JSC) and the test is skipped. +#if !(defined(JSRUNTIMEHOST_NAPI_ENGINE_V8) || (defined(JSRUNTIMEHOST_NAPI_ENGINE_JavaScriptCore) && defined(__APPLE__))) + GTEST_SKIP() << "unhandled promise rejection tracking requires the V8 or Apple JavaScriptCore backend"; #else // A fire-and-forget rejected promise (no handler ever attached) must reach the embedder's // UnhandledExceptionHandler. @@ -313,8 +314,8 @@ TEST(AppRuntime, UnhandledPromiseRejectionReachesHandler) TEST(AppRuntime, SynchronouslyHandledRejectionDoesNotReachHandler) { // Only engines with a host promise-rejection hook implement this tracking (see the note above). -#if !defined(JSRUNTIMEHOST_NAPI_ENGINE_V8) && !defined(JSRUNTIMEHOST_NAPI_ENGINE_JavaScriptCore) - GTEST_SKIP() << "unhandled promise rejection tracking is only implemented for the V8 and JavaScriptCore backends"; +#if !(defined(JSRUNTIMEHOST_NAPI_ENGINE_V8) || (defined(JSRUNTIMEHOST_NAPI_ENGINE_JavaScriptCore) && defined(__APPLE__))) + GTEST_SKIP() << "unhandled promise rejection tracking requires the V8 or Apple JavaScriptCore backend"; #else // A rejection that is handled synchronously in the same turn must NOT reach the handler -- // reporting is deferred to the end of the turn, by which point the .catch has been attached.