Skip to content

Commit 2eef639

Browse files
CopilotCopilot
committed
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_<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>
1 parent 2e75705 commit 2eef639

8 files changed

Lines changed: 215 additions & 113 deletions

File tree

‎Core/AppRuntime/Include/Babylon/AppRuntime.h‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -21,15 +21,6 @@ namespace Babylon
2121
// Optional handler for unhandled exceptions.
2222
std::function<void(const Napi::Error&)> UnhandledExceptionHandler{DefaultUnhandledExceptionHandler};
2323

24-
// When true, unhandled promise rejections are routed to UnhandledExceptionHandler so
25-
// the embedder's crash/telemetry pipeline can observe fire-and-forget failures (e.g. an
26-
// un-awaited fetch() that rejects). Defaults to false to preserve existing behavior --
27-
// when false, no per-engine rejection tracker is installed. Reporting is deferred to the
28-
// end of the current turn, so a rejection that is handled synchronously (e.g.
29-
// `const p = Promise.reject(e); p.catch(...)`) does not reach the handler. Implemented for
30-
// the Chakra and V8 engines.
31-
bool EnableUnhandledPromiseRejectionTracking{false};
32-
3324
// Defines whether to enable the debugger. Only implemented for V8 and Chakra.
3425
bool EnableDebugger{false};
3526

@@ -54,10 +45,20 @@ namespace Babylon
5445

5546
void Dispatch(Dispatchable<void(Napi::Env)> callback);
5647

57-
// Routes an unhandled promise rejection to the embedder's UnhandledExceptionHandler. Called
58-
// by the per-engine promise-rejection tracker installed in RunEnvironmentTier when
59-
// Options::EnableUnhandledPromiseRejectionTracking is set. Intended for internal
60-
// (engine-implementation) use.
48+
// Routes an unhandled promise rejection to the embedder's UnhandledExceptionHandler (which
49+
// defaults to a benign logger), so an embedder's crash/telemetry pipeline can observe
50+
// fire-and-forget failures (e.g. an un-awaited fetch() that rejects) -- matching the browser
51+
// `unhandledrejection` behavior. Reporting is deferred to the end of the turn, so a rejection
52+
// handled synchronously (e.g. `const p = Promise.reject(e); p.catch(...)`) is not reported.
53+
//
54+
// Coverage is determined by whether the engine exposes a host promise-rejection hook:
55+
// * V8 (Isolate::SetPromiseRejectCallback) and JavaScriptCore
56+
// (JSGlobalContextSetUnhandledRejectionCallback) -- supported.
57+
// * Chakra (in-box/EdgeMode) and JSI -- no-op: neither exposes such a hook
58+
// (JsSetHostPromiseRejectionTracker is ChakraCore-only, and neither jsi::Runtime nor
59+
// V8JSI surfaces the V8 callback).
60+
//
61+
// Intended for internal (engine-implementation) use.
6162
void OnUnhandledPromiseRejection(const Napi::Error& error);
6263

6364
// Default unhandled exception handler that outputs the error message to the program output.

‎Core/AppRuntime/Source/AppRuntime_Chakra.cpp‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,9 @@ namespace Babylon
5151
},
5252
&dispatchFunction));
5353

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

5958
ThrowIfFailed(JsProjectWinRTNamespace(L"Windows"));
6059

‎Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,49 @@
11
#include "AppRuntime.h"
2+
#include "AppRuntime_PromiseRejection.h"
23
#include <napi/env.h>
34

5+
// JSGlobalContextSetUnhandledRejectionCallback is declared in the private JavaScriptCore header
6+
// <JavaScriptCore/JSContextRefPrivate.h>, which is not part of the public macOS/iOS SDK. The symbol
7+
// is exported by the JavaScriptCore framework (SPI), so it is forward-declared here and the call is
8+
// guarded with __builtin_available (it is JSC_API_AVAILABLE(macos(10.15.4), ios(13.4))). It registers
9+
// a JS function invoked at the microtask checkpoint with (promise, reason) for each promise that is
10+
// still unhandled at that point, so -- unlike V8 -- no deferral or candidate bookkeeping is needed.
11+
extern "C" void JSGlobalContextSetUnhandledRejectionCallback(JSGlobalContextRef ctx, JSObjectRef function, JSValueRef* exception);
12+
413
namespace Babylon
514
{
15+
namespace
16+
{
17+
// JSObjectMakeFunctionWithCallback takes no user-data argument; each AppRuntime owns its JSC
18+
// context on a dedicated thread, so a thread_local associates the callback with this runtime.
19+
struct JSCRejectionContext
20+
{
21+
AppRuntime* runtime{};
22+
napi_env env{};
23+
};
24+
25+
thread_local JSCRejectionContext* t_rejectionContext{nullptr};
26+
27+
// Mirrors ToNapi (js_native_api_javascriptcore.cc): napi_value is a JSValueRef in the
28+
// JavaScriptCore Node-API shim.
29+
napi_value JsValueToNapi(JSValueRef value)
30+
{
31+
return reinterpret_cast<napi_value>(const_cast<OpaqueJSValue*>(value));
32+
}
33+
34+
JSValueRef OnUnhandledRejection(JSContextRef ctx, JSObjectRef, JSObjectRef, size_t argumentCount, const JSValueRef arguments[], JSValueRef*)
35+
{
36+
JSCRejectionContext* context{t_rejectionContext};
37+
if (context != nullptr && argumentCount >= 2)
38+
{
39+
const Napi::Env env{context->env};
40+
context->runtime->OnUnhandledPromiseRejection(Internal::ToError(env, JsValueToNapi(arguments[1])));
41+
}
42+
43+
return JSValueMakeUndefined(ctx);
44+
}
45+
}
46+
647
void AppRuntime::RunEnvironmentTier(const char*)
748
{
849
auto globalContext = JSGlobalContextCreateInGroup(nullptr, nullptr);
@@ -16,8 +57,21 @@ namespace Babylon
1657

1758
Napi::Env env = Napi::Attach(globalContext);
1859

60+
// Always track unhandled promise rejections (routed to the host UnhandledExceptionHandler).
61+
JSCRejectionContext rejectionContext{this, env};
62+
t_rejectionContext = &rejectionContext;
63+
if (__builtin_available(iOS 13.4, macOS 10.15.4, *))
64+
{
65+
JSStringRef callbackName = JSStringCreateWithUTF8CString("onUnhandledRejection");
66+
JSObjectRef callback = JSObjectMakeFunctionWithCallback(globalContext, callbackName, OnUnhandledRejection);
67+
JSStringRelease(callbackName);
68+
JSGlobalContextSetUnhandledRejectionCallback(globalContext, callback, nullptr);
69+
}
70+
1971
Run(env);
2072

73+
t_rejectionContext = nullptr;
74+
2175
JSGlobalContextRelease(globalContext);
2276

2377
// Detach must come after JSGlobalContextRelease since it triggers finalizers which require env.

‎Core/AppRuntime/Source/AppRuntime_PromiseRejection.h‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
#pragma once
2+
3+
#include "AppRuntime.h"
4+
5+
#include <napi/napi.h>
6+
7+
#include <algorithm>
8+
#include <utility>
9+
#include <vector>
10+
11+
namespace Babylon::Internal
12+
{
13+
// Wraps an unhandled-promise rejection reason as a Napi::Error: an Error-like object passes
14+
// through (preserving message/stack/cause); any other value is stringified so the host handler
15+
// always receives a Napi::Error. Lives here rather than in shared AppRuntime.cpp because the
16+
// napi_value -> Napi::Value bridge is unavailable on the JSI Node-API shim, and only the engines
17+
// that support rejection tracking (V8, JavaScriptCore) include this header.
18+
inline Napi::Error ToError(Napi::Env env, napi_value reason)
19+
{
20+
const Napi::Value value{env, reason};
21+
return value.IsObject()
22+
? Napi::Error{env, reason}
23+
: Napi::Error::New(env, value.ToString().Utf8Value());
24+
}
25+
26+
// Engine-agnostic bookkeeping for engines that report an unhandled rejection immediately and a
27+
// later handler-added event separately (V8): collect candidates as promises reject without a
28+
// handler, drop them when a handler is attached, and report the survivors to the host handler at
29+
// the end of the current turn -- so a rejection handled synchronously within the same turn is
30+
// never reported. Engines whose host hook already fires only for still-unhandled rejections at
31+
// the microtask checkpoint (JavaScriptCore) report directly and do not need this.
32+
//
33+
// CandidateT is engine-specific and must provide:
34+
// void Report(AppRuntime& runtime, Napi::Env env) const;
35+
// // convert its stored reason to a Napi::Error (via ToError) and call
36+
// // runtime.OnUnhandledPromiseRejection
37+
template<typename CandidateT>
38+
class PromiseRejectionTracker
39+
{
40+
public:
41+
explicit PromiseRejectionTracker(AppRuntime& runtime)
42+
: m_runtime{runtime}
43+
{
44+
}
45+
46+
void Add(CandidateT candidate)
47+
{
48+
m_candidates.push_back(std::move(candidate));
49+
50+
if (!m_flushScheduled)
51+
{
52+
m_flushScheduled = true;
53+
m_runtime.Dispatch([this](Napi::Env env) { Flush(env); });
54+
}
55+
}
56+
57+
template<typename PredicateT>
58+
void Remove(PredicateT predicate)
59+
{
60+
m_candidates.erase(
61+
std::remove_if(m_candidates.begin(), m_candidates.end(), std::move(predicate)),
62+
m_candidates.end());
63+
}
64+
65+
private:
66+
void Flush(Napi::Env env)
67+
{
68+
m_flushScheduled = false;
69+
70+
// Move the candidates out before reporting: a host handler could synchronously reject
71+
// another promise, re-entering Add() and mutating m_candidates mid-iteration.
72+
const std::vector<CandidateT> candidates = std::move(m_candidates);
73+
m_candidates.clear();
74+
75+
for (const CandidateT& candidate : candidates)
76+
{
77+
candidate.Report(m_runtime, env);
78+
}
79+
}
80+
81+
AppRuntime& m_runtime;
82+
std::vector<CandidateT> m_candidates;
83+
bool m_flushScheduled{false};
84+
};
85+
}

‎Core/AppRuntime/Source/AppRuntime_V8.cpp‎

Lines changed: 36 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#include "AppRuntime.h"
2+
#include "AppRuntime_PromiseRejection.h"
23
#include <napi/env.h>
34

45
#include <libplatform/libplatform.h>
@@ -8,8 +9,6 @@
89
#endif
910

1011
#include <optional>
11-
#include <unordered_map>
12-
#include <utility>
1312

1413
namespace Babylon
1514
{
@@ -64,26 +63,6 @@ namespace Babylon
6463

6564
std::unique_ptr<Module> Module::s_module;
6665

67-
// Tracks promises rejected without a handler so they can be reported once the current turn
68-
// settles. V8 fires kPromiseRejectWithNoHandler when a promise is rejected with no handler
69-
// and kPromiseHandlerAddedAfterReject if one is attached afterwards; reporting is deferred
70-
// (via AppRuntime::Dispatch) so a synchronous `Promise.reject(e); ...; p.catch(...)` is
71-
// removed before it is ever reported. Keyed by promise identity hash; the promise and reason
72-
// are held in v8::Global handles so they survive until the deferred flush runs.
73-
struct V8RejectionTracker
74-
{
75-
AppRuntime* runtime{};
76-
v8::Isolate* isolate{};
77-
std::unordered_map<int, std::pair<v8::Global<v8::Promise>, v8::Global<v8::Value>>> unhandled{};
78-
bool flushScheduled{false};
79-
};
80-
81-
// The promise-rejection callback is a bare function pointer with no user-data argument. Each
82-
// AppRuntime owns a dedicated isolate running on its own thread, and V8 invokes the callback
83-
// on that thread, so a thread_local pointer associates the callback with the right tracker
84-
// without risking isolate-data-slot collisions with the Node-API shim.
85-
thread_local V8RejectionTracker* t_rejectionTracker{nullptr};
86-
8766
// Mirrors v8impl::JsValueFromV8LocalValue (js_native_api_v8.h), which is internal to the
8867
// Node-API V8 shim and not on the public include path.
8968
static_assert(sizeof(v8::Local<v8::Value>) == sizeof(napi_value),
@@ -93,31 +72,30 @@ namespace Babylon
9372
return reinterpret_cast<napi_value>(*local);
9473
}
9574

96-
// Wrap a rejection reason as a Napi::Error. An Error-like object is forwarded as-is
97-
// (preserving message/stack/cause); any other value is stringified so the embedder's handler
98-
// always receives a Napi::Error. Done here (not in shared code) because the napi_value ->
99-
// Napi::Value bridge is specific to the V8/standard Node-API shim.
100-
Napi::Error ToError(Napi::Env env, napi_value reason)
75+
// A promise rejected without a handler, awaiting end-of-turn reporting. The promise and
76+
// reason are held in v8::Global handles so they survive until the deferred flush; the promise
77+
// is retained so a later handler-added event can drop this candidate by object identity
78+
// (v8::Object::GetIdentityHash is not unique, so identity comparison is used instead).
79+
struct V8RejectionCandidate
10180
{
102-
const Napi::Value reasonValue{env, reason};
103-
return reasonValue.IsObject()
104-
? Napi::Error{env, reason}
105-
: Napi::Error::New(env, reasonValue.ToString().Utf8Value());
106-
}
107-
108-
void FlushUnhandledRejections(V8RejectionTracker& tracker, Napi::Env env)
109-
{
110-
tracker.flushScheduled = false;
81+
v8::Isolate* isolate{};
82+
v8::Global<v8::Promise> promise;
83+
v8::Global<v8::Value> reason;
11184

112-
v8::Isolate::Scope isolateScope{tracker.isolate};
113-
v8::HandleScope handleScope{tracker.isolate};
114-
for (auto& entry : tracker.unhandled)
85+
void Report(AppRuntime& runtime, Napi::Env env) const
11586
{
116-
const v8::Local<v8::Value> reason = entry.second.second.Get(tracker.isolate);
117-
tracker.runtime->OnUnhandledPromiseRejection(ToError(env, JsValueFromV8LocalValue(reason)));
87+
v8::HandleScope handleScope{isolate};
88+
runtime.OnUnhandledPromiseRejection(Internal::ToError(env, JsValueFromV8LocalValue(reason.Get(isolate))));
11889
}
119-
tracker.unhandled.clear();
120-
}
90+
};
91+
92+
using V8RejectionTracker = Internal::PromiseRejectionTracker<V8RejectionCandidate>;
93+
94+
// The promise-rejection callback is a bare function pointer with no user-data argument. Each
95+
// AppRuntime owns a dedicated isolate running on its own thread, and V8 invokes the callback
96+
// on that thread, so a thread_local pointer associates the callback with the right tracker
97+
// without risking isolate-data-slot collisions with the Node-API shim.
98+
thread_local V8RejectionTracker* t_rejectionTracker{nullptr};
12199

122100
void OnPromiseReject(v8::PromiseRejectMessage message)
123101
{
@@ -127,31 +105,25 @@ namespace Babylon
127105
return;
128106
}
129107

130-
v8::HandleScope handleScope{tracker->isolate};
108+
v8::Isolate* isolate{v8::Isolate::GetCurrent()};
109+
v8::HandleScope handleScope{isolate};
131110
const v8::Local<v8::Promise> promise{message.GetPromise()};
132-
const int hash{promise->GetIdentityHash()};
133111

134112
switch (message.GetEvent())
135113
{
136114
case v8::kPromiseRejectWithNoHandler:
137115
{
138-
const v8::Local<v8::Value> reason{message.GetValue()};
139-
tracker->unhandled[hash] = {
140-
v8::Global<v8::Promise>{tracker->isolate, promise},
141-
v8::Global<v8::Value>{tracker->isolate, reason}};
142-
143-
if (!tracker->flushScheduled)
144-
{
145-
tracker->flushScheduled = true;
146-
tracker->runtime->Dispatch([tracker](Napi::Env env) {
147-
FlushUnhandledRejections(*tracker, env);
148-
});
149-
}
116+
tracker->Add(V8RejectionCandidate{
117+
isolate,
118+
v8::Global<v8::Promise>{isolate, promise},
119+
v8::Global<v8::Value>{isolate, message.GetValue()}});
150120
break;
151121
}
152122
case v8::kPromiseHandlerAddedAfterReject:
153123
{
154-
tracker->unhandled.erase(hash);
124+
tracker->Remove([isolate, promise](const V8RejectionCandidate& candidate) {
125+
return candidate.promise.Get(isolate) == promise;
126+
});
155127
break;
156128
}
157129
default:
@@ -182,12 +154,10 @@ namespace Babylon
182154

183155
Napi::Env env = Napi::Attach(context);
184156

185-
V8RejectionTracker rejectionTracker{this, isolate, {}, false};
186-
if (m_options.EnableUnhandledPromiseRejectionTracking)
187-
{
188-
t_rejectionTracker = &rejectionTracker;
189-
isolate->SetPromiseRejectCallback(OnPromiseReject);
190-
}
157+
// Always track unhandled promise rejections (routed to the host UnhandledExceptionHandler).
158+
V8RejectionTracker rejectionTracker{*this};
159+
t_rejectionTracker = &rejectionTracker;
160+
isolate->SetPromiseRejectCallback(OnPromiseReject);
191161

192162
#ifdef ENABLE_V8_INSPECTOR
193163
std::optional<V8InspectorAgent> agent;
@@ -212,11 +182,8 @@ namespace Babylon
212182
}
213183
#endif
214184

215-
if (m_options.EnableUnhandledPromiseRejectionTracking)
216-
{
217-
isolate->SetPromiseRejectCallback(nullptr);
218-
t_rejectionTracker = nullptr;
219-
}
185+
isolate->SetPromiseRejectCallback(nullptr);
186+
t_rejectionTracker = nullptr;
220187

221188
Napi::Detach(env);
222189
}

‎Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ add_library(UnitTestsJNI SHARED
2323

2424
target_compile_definitions(UnitTestsJNI PRIVATE JSRUNTIMEHOST_PLATFORM="${JSRUNTIMEHOST_PLATFORM}")
2525
target_compile_definitions(UnitTestsJNI PRIVATE JSRUNTIMEHOST_NAPI_ENGINE="${NAPI_JAVASCRIPT_ENGINE}")
26+
# Engine-specific compile-time gate (e.g. JSRUNTIMEHOST_NAPI_ENGINE_V8), matching Tests/UnitTests/CMakeLists.txt.
27+
target_compile_definitions(UnitTestsJNI PRIVATE JSRUNTIMEHOST_NAPI_ENGINE_${NAPI_JAVASCRIPT_ENGINE})
2628
target_compile_definitions(UnitTestsJNI PRIVATE ARCANA_TEST_HOOKS)
2729

2830
target_include_directories(UnitTestsJNI

0 commit comments

Comments
 (0)