Skip to content

Commit 30f5079

Browse files
committed
feat: boundary-catchable uncaughtErrorPolicy throw and pre-dispatch stackTrace
Android parity round for the 9.1 error unification: - error.stackTrace / reason.stackTrace are now populated (combined smart stack) BEFORE the error/unhandledrejection events dispatch, so listeners can read them like on Android, and the fatal tail reuses the derived stack instead of recomputing it. - uncaughtErrorPolicy 'throw' uses a claim-slot handoff: the fatal tail deposits the NativeScriptFatalJSException in a per-isolate registry and schedules an identity-checked clean-frame fallback. Boundaries that report within their own frame (property accessors, adapter reads) claim the deposit under their V8 scopes and @throw it after scope teardown, so a native @try/@catch around that call catches it (matching Android). Block/overridden-method callbacks surface the throw to V8 (so JS-initiated chains keep propagating to JS catchers) and are covered by the deferred fallback, as are loop-originated errors. Double-throw is impossible: single-consumer claim with pointer-identity check on the fallback. - The registry is heap-allocated and never destroyed: a global Runtime can destruct during __cxa_finalize at process exit, and locking a destroyed file-static mutex from ~Runtime would std::terminate.
1 parent 05389dd commit 30f5079

7 files changed

Lines changed: 272 additions & 13 deletions

File tree

‎NativeScript/runtime/ArgConverter.mm‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,18 @@
269269
Local<v8::Message> message = tc.Message();
270270
NativeScriptException::ReportToJsHandlersAndLog(isolate, exception, message);
271271
tc.Reset();
272+
273+
// uncaughtErrorPolicy "throw": the report above runs synchronously here (still
274+
// under the boundary's live V8 scopes). Under the "throw" policy it deposited
275+
// the NSException into the per-isolate slot; claim it and hand it back so the
276+
// caller @throws it AFTER closing every V8 scope (same scopes-before-@throw
277+
// discipline as a branded escape) — a native @try/@catch around the boundary
278+
// then catches it, matching Android. Under the default "report" policy nothing
279+
// is deposited and this is nil.
280+
id policyThrow = NativeScriptException::ClaimPendingPolicyThrow(isolate);
281+
if (policyThrow != nil) {
282+
return policyThrow;
283+
}
272284
return nil;
273285
}
274286

@@ -385,6 +397,26 @@
385397
}
386398
}
387399

400+
// uncaughtErrorPolicy "throw" boundary claim (best-effort). Unlike the
401+
// HandleBoundaryException sites (property accessors, DictionaryAdapter),
402+
// which report the throw SYNCHRONOUSLY within their own frame and can
403+
// therefore claim the deposit here, this path uses tc.ReThrow(): the
404+
// unbranded exception is surfaced to V8 as pending and only reported when it
405+
// reaches an uncaught V8 Invoke boundary UP THE STACK (or is caught in JS
406+
// first). Empirically (verified with instrumentation over the suite,
407+
// including a native-origin NSOperationQueue-invoked block) the message
408+
// listener does NOT run during `tc`'s teardown, so under the "throw" policy
409+
// nothing has been deposited by the time we reach this point and this claim
410+
// is a no-op — those errors are instead handled by the deferred clean-frame
411+
// fallback. The claim is kept as a defensive hook (harmless when the slot is
412+
// empty; identity-checked so it can never steal an unrelated deposit) and to
413+
// mirror the boundary discipline. Under the default "report" policy nothing
414+
// is ever deposited. The NSError-out branch neither reports nor deposits.
415+
// Skip when a branded escape already set pendingThrow.
416+
if (pendingThrow == nil) {
417+
pendingThrow = NativeScriptException::ClaimPendingPolicyThrow(isolate);
418+
}
419+
388420
if (pendingThrow == nil) {
389421
if (!success) {
390422
memset(retValue, 0, cif->rtype->size);

‎NativeScript/runtime/NativeScriptException.h‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,29 @@ class NativeScriptException {
5858
v8::Local<v8::Message> message,
5959
const std::string& stackOverride = "",
6060
const std::string& logPrefix = "");
61+
// uncaughtErrorPolicy "throw" handoff slot (per isolate). When the policy is
62+
// "throw", ReportFatalTail deposits the NSException to throw here (typed `id`
63+
// so this header stays usable from the pure-C++ TUs that include it — mirrors
64+
// ArgConverter.h) and schedules a deferred fallback throw. A JS→native
65+
// boundary that originated the error claims the slot while still under its V8
66+
// scopes and @throws the exception synchronously after scope teardown, so a
67+
// native @try/@catch around the boundary can catch it (Android parity).
68+
// Loop-originated errors (timers, microtasks, rejections) are never claimed
69+
// and are thrown by the deferred fallback from a clean, scope-free frame.
70+
//
71+
// Deposits happen under the isolate lock (reporting is serialized); claims
72+
// happen either at the boundary (under the Locker) or in the deferred
73+
// fallback (mutex only). The identity-checked claim prevents a stale fallback
74+
// block from throwing a newer deposit.
75+
static void DepositPendingPolicyThrow(v8::Isolate* isolate, id exception);
76+
// Returns and clears the isolate's pending policy throw, or nil.
77+
static id ClaimPendingPolicyThrow(v8::Isolate* isolate);
78+
// Claims only when the stored exception IS `expected` (pointer identity);
79+
// used by the deferred fallback so an older block cannot steal/throw a newer
80+
// deposit. Returns the claimed exception or nil.
81+
static id ClaimPendingPolicyThrowIfEqual(v8::Isolate* isolate, id expected);
82+
// Clears any pending policy throw for the isolate (called during teardown).
83+
static void OnIsolateTeardown(v8::Isolate* isolate);
6184
static std::string GetErrorStackTrace(
6285
v8::Isolate* isolate, const v8::Local<v8::StackTrace>& stackTrace);
6386
static void ShowErrorModal(v8::Isolate* isolate, const std::string& title,

‎NativeScript/runtime/NativeScriptException.mm‎

Lines changed: 130 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
#include <limits>
1212
#include <mutex>
1313
#include <sstream>
14+
#include <unordered_map>
1415
#include "ArgConverter.h"
1516
#include "Caches.h"
1617
#include "DataWrapper.h"
@@ -43,6 +44,22 @@
4344
static PendingErrorDisplay gPendingErrorDisplay;
4445
static uint64_t gNextErrorTicket = 1;
4546

47+
// Per-isolate uncaughtErrorPolicy "throw" handoff slot. The NSException lifetime
48+
// is managed by ARC (__strong) — fine in this .mm. Heap-allocated and never
49+
// destroyed: ~Runtime reaches this registry via OnIsolateTeardown, and a global
50+
// Runtime can be destructed during __cxa_finalize at process exit — after this
51+
// TU's file-scope statics would have been destroyed. Locking a destroyed
52+
// std::mutex throws std::system_error out of the noexcept destructor chain and
53+
// terminates the process, so the registry must outlive all static destructors.
54+
static std::mutex& PolicyThrowMutex() {
55+
static std::mutex* mutex = new std::mutex();
56+
return *mutex;
57+
}
58+
static std::unordered_map<v8::Isolate*, NSException * __strong>& PendingPolicyThrows() {
59+
static auto* map = new std::unordered_map<v8::Isolate*, NSException * __strong>();
60+
return *map;
61+
}
62+
4663
} // namespace
4764

4865
namespace tns {
@@ -122,15 +139,47 @@ static void ConsiderStackCandidate(PendingErrorDisplay& state, v8::Isolate* isol
122139
} else {
123140
messageString = tns::ToString(isolate, error);
124141
}
142+
// Derive the combined stack ONCE, with the full fallback chain ReportFatalTail
143+
// uses (stackOverride → GetSmartStackTrace → v8::Message stack → the error's
144+
// own stack), so the `error` event listeners and the fatal tail observe the
145+
// same string.
125146
std::string stackForEvent = stackOverride;
126147
if (stackForEvent.empty()) {
127148
stackForEvent = tns::GetSmartStackTrace(isolate, nullptr, error);
128149
}
150+
if (stackForEvent.empty()) {
151+
if (!message.IsEmpty()) {
152+
stackForEvent = GetErrorStackTrace(isolate, message->GetStackTrace());
153+
} else {
154+
// Rejections/reportError carry no v8::Message; fall back to the error's
155+
// own stack.
156+
stackForEvent = GetErrorStackTrace(isolate, Exception::GetStackTrace(error));
157+
}
158+
}
159+
160+
// Android sets a combined `stackTrace` property on the error object BEFORE
161+
// dispatching the `error` event, so listeners can read `e.error.stackTrace`
162+
// (not only `e.error.stack`). Mirror that here (object errors only; guarded /
163+
// non-fatal on failure — same pattern ReportFatalTail uses). ReportFatalTail
164+
// sets it again with the same value (idempotent) for callers that reach it
165+
// directly (the nativeReportFatal handshake).
166+
if (error->IsObject()) {
167+
Local<Context> context = isolate->GetCurrentContext();
168+
bool stackTraceSet = error.As<Object>()
169+
->Set(context, tns::ToV8String(isolate, "stackTrace"),
170+
tns::ToV8String(isolate, stackForEvent))
171+
.FromMaybe(false);
172+
if (!stackTraceSet) {
173+
Log(@"Warning: Failed to set stackTrace property on error object");
174+
}
175+
}
176+
129177
if (ErrorEvents::DispatchError(isolate, error, messageString, stackForEvent)) {
130178
return;
131179
}
132180

133-
ReportFatalTail(isolate, error, message, stackOverride, logPrefix);
181+
// Pass the already-derived stack down so ReportFatalTail does not re-derive it.
182+
ReportFatalTail(isolate, error, message, stackForEvent, logPrefix);
134183
}
135184

136185
void NativeScriptException::ReportUnhandledRejection(Isolate* isolate, Local<Promise> promise,
@@ -164,6 +213,38 @@ static void ScheduleDeferredThrow(Isolate* isolate, NSException* e) {
164213
CFRunLoopWakeUp(loop);
165214
}
166215

216+
void NativeScriptException::DepositPendingPolicyThrow(Isolate* isolate, id exception) {
217+
std::lock_guard<std::mutex> lock(PolicyThrowMutex());
218+
PendingPolicyThrows()[isolate] = (NSException*)exception; // overwrites any prior entry
219+
}
220+
221+
id NativeScriptException::ClaimPendingPolicyThrow(Isolate* isolate) {
222+
std::lock_guard<std::mutex> lock(PolicyThrowMutex());
223+
auto it = PendingPolicyThrows().find(isolate);
224+
if (it == PendingPolicyThrows().end()) {
225+
return nil;
226+
}
227+
NSException* e = it->second;
228+
PendingPolicyThrows().erase(it);
229+
return e;
230+
}
231+
232+
id NativeScriptException::ClaimPendingPolicyThrowIfEqual(Isolate* isolate, id expected) {
233+
std::lock_guard<std::mutex> lock(PolicyThrowMutex());
234+
auto it = PendingPolicyThrows().find(isolate);
235+
if (it == PendingPolicyThrows().end() || it->second != expected) {
236+
return nil;
237+
}
238+
NSException* e = it->second;
239+
PendingPolicyThrows().erase(it);
240+
return e;
241+
}
242+
243+
void NativeScriptException::OnIsolateTeardown(Isolate* isolate) {
244+
std::lock_guard<std::mutex> lock(PolicyThrowMutex());
245+
PendingPolicyThrows().erase(isolate);
246+
}
247+
167248
void NativeScriptException::ReportFatalTail(Isolate* isolate, Local<Value> error,
168249
Local<v8::Message> message,
169250
const std::string& stackOverride,
@@ -293,9 +374,14 @@ static void ScheduleDeferredThrow(Isolate* isolate, NSException* e) {
293374
// uncaughtErrorPolicy — the cross-platform uncaught-error contract:
294375
// "report" (default): report and keep the app running;
295376
// "throw": after reporting, rethrow the unprevented error
296-
// natively (deferred to a clean scope-free frame). This
297-
// is a throw, not a crash guarantee — the app terminates
298-
// only if no native handler catches it.
377+
// natively. If a JS→native boundary originated the error
378+
// it is rethrown synchronously there (catchable by a
379+
// native @try/@catch around that call, matching Android);
380+
// otherwise (loop-originated: timers, microtasks,
381+
// rejections) it is thrown from a clean, scope-free frame
382+
// on the runtime loop. Either way it is a throw, not a
383+
// crash guarantee — the app terminates only if nothing
384+
// catches it.
299385
// The deprecated discardUncaughtJsExceptions flag suppresses the throw (an
300386
// explicit "keep the app alive").
301387
if (!isDiscarded) {
@@ -315,7 +401,32 @@ static void ScheduleDeferredThrow(Isolate* isolate, NSException* e) {
315401
if (!stackTrace.empty()) {
316402
tns::SetJSStackOnException(fatal, tns::ToNSString(stackTrace));
317403
}
318-
ScheduleDeferredThrow(isolate, fatal);
404+
// Claim-slot handoff: deposit the exception and schedule a FALLBACK
405+
// clean-frame throw. A JS→native boundary that originated this error
406+
// claims the slot while still under its V8 scopes and @throws `fatal`
407+
// synchronously after scope teardown, so a native @try/@catch around the
408+
// boundary can catch it (Android parity). Loop-originated errors (timers,
409+
// microtasks, rejections) are never claimed at a boundary, so the fallback
410+
// throws them from a clean, scope-free frame exactly as before.
411+
//
412+
// The fallback claims by pointer IDENTITY (ClaimPendingPolicyThrowIfEqual):
413+
// reporting is serialized under the isolate lock, but if a newer error is
414+
// deposited and then claimed (by its boundary or its own fallback) before
415+
// this block runs, the identity check fails and this now-stale block is a
416+
// no-op — it can never throw a different (newer) error than the one it was
417+
// scheduled for.
418+
DepositPendingPolicyThrow(isolate, fatal);
419+
Runtime* rt = Runtime::GetRuntime(isolate);
420+
CFRunLoopRef loop = rt != nullptr ? rt->RuntimeLoop() : nullptr;
421+
if (loop != nullptr) {
422+
CFRunLoopPerformBlock(loop, kCFRunLoopCommonModes, ^{
423+
id e = ClaimPendingPolicyThrowIfEqual(isolate, fatal);
424+
if (e != nil) {
425+
@throw e;
426+
}
427+
});
428+
CFRunLoopWakeUp(loop);
429+
}
319430
} else if (policy != "report") {
320431
static std::once_flag warnedPolicy;
321432
std::call_once(warnedPolicy, [&policy]() {
@@ -477,6 +588,20 @@ static bool GiveWorkerOnErrorAChance(Isolate* isolate, Local<Context> context,
477588
NativeScriptException::GetErrorStackTrace(isolate_, Exception::GetStackTrace(reason));
478589
}
479590

591+
// Android parity (Fix A): populate `reason.stackTrace` BEFORE dispatching
592+
// the unhandledrejection event so listeners can read `e.reason.stackTrace`
593+
// (object reasons only; guarded / non-fatal). Covers both the worker and
594+
// main dispatch branches below.
595+
if (reason->IsObject() && !stack.empty()) {
596+
bool ok = reason.As<Object>()
597+
->Set(context, tns::ToV8String(isolate_, "stackTrace"),
598+
tns::ToV8String(isolate_, stack))
599+
.FromMaybe(false);
600+
if (!ok) {
601+
Log(@"Warning: Failed to set stackTrace property on rejection reason");
602+
}
603+
}
604+
480605
if (isWorker) {
481606
// Dispatch the rejection event on the worker's own global first;
482607
// preventDefault() there fully handles it. Only when unprevented fall

‎NativeScript/runtime/Runtime.mm‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,10 @@ void DisposeIsolateWhenPossible(Isolate* isolate) {
193193
rejectionObserver_ = nullptr;
194194
}
195195

196+
// Drop any pending uncaughtErrorPolicy "throw" deposit for this isolate so a
197+
// stale entry can't outlive the isolate it keys on.
198+
NativeScriptException::OnIsolateTeardown(isolate_);
199+
196200
auto currentIsolate = this->isolate_;
197201
{
198202
// make sure we remove the isolate from the list of active isolates first

‎TestRunner/app/tests/ErrorEventsTests.js‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,10 @@ describe("WHATWG error events", function () {
8686
expect(received.error).toBe(err);
8787
expect(uncaught.length).toBe(1);
8888
expect(uncaught[0]).toBe(err);
89+
// Fix A: an Error reported (unprevented) through the pipeline ends up
90+
// with a combined `stackTrace` string on the error object.
91+
expect(typeof err.stackTrace).toBe("string");
92+
expect(err.stackTrace.length).toBeGreaterThan(0);
8993
done();
9094
});
9195

@@ -95,6 +99,39 @@ describe("WHATWG error events", function () {
9599
}).toThrowError(TypeError);
96100
});
97101

102+
it("an error reported through a native boundary has a combined stackTrace set BEFORE the error event fires", function (done) {
103+
// Reading an overridden getter from native routes the JS throw through
104+
// the C++ boundary reporter (ReportToJsHandlersAndLog), which now sets
105+
// `error.stackTrace` before dispatching the cancelable `error` event —
106+
// matching Android — so the listener observes it during dispatch (not
107+
// only `error.stack`).
108+
const marker = "boundary-stacktrace-marker";
109+
let sawEvent = false;
110+
let stackTraceInListener = null;
111+
onGlobal("error", function (e) {
112+
if (e.error && e.error.message === marker) {
113+
sawEvent = true;
114+
stackTraceInListener = e.error.stackTrace;
115+
e.preventDefault();
116+
}
117+
});
118+
119+
const Derived = TNSDerivedInterface.extend({
120+
get baseProperty() {
121+
throw new Error(marker);
122+
}
123+
});
124+
const instance = Derived.alloc().init();
125+
TNSTestNativeCallbacks.objectValueForKeyKey(instance, "baseProperty");
126+
127+
pollUntil(function () { return sawEvent; }, function () {
128+
expect(sawEvent).toBe(true);
129+
expect(typeof stackTraceInListener).toBe("string");
130+
expect(stackTraceInListener.length).toBeGreaterThan(0);
131+
done();
132+
});
133+
});
134+
98135
it("unhandledrejection listener receives reason and promise; preventDefault suppresses the shim", function (done) {
99136
const reason = new Error("rejected");
100137
let received = null;
@@ -111,6 +148,11 @@ describe("WHATWG error events", function () {
111148
expect(received.type).toBe("unhandledrejection");
112149
expect(received.reason).toBe(reason);
113150
expect(typeof received.promise.then).toBe("function");
151+
// Fix A / Android parity: the rejection drain sets a combined
152+
// `stackTrace` on the (object) reason BEFORE dispatching the event,
153+
// so a listener sees it (not only `reason.stack`).
154+
expect(typeof received.reason.stackTrace).toBe("string");
155+
expect(received.reason.stackTrace.length).toBeGreaterThan(0);
114156
afterQuietTurns(function () {
115157
expect(uncaught.length).toBe(0);
116158
done();

‎TestRunner/app/tests/EscapeExceptionTests.js‎

Lines changed: 39 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -250,11 +250,43 @@ describe("interop.escapeException and boundary hardening", function () {
250250
});
251251
});
252252

253-
// manual: uncaughtErrorPolicy "throw" is not exercised automatically because a
254-
// real crash would kill the test runner. To smoke it manually, set
255-
// { "uncaughtErrorPolicy": "throw" } in the app config and throw an
256-
// unprevented error (or `throw interop.escapeException(err)` from an `error`
257-
// listener) — the app must terminate with a NativeScriptFatalJSException
258-
// crash report scheduled on the runtime loop. Default-off behavior (report +
259-
// continue) is covered by every other suite here.
253+
// manual: uncaughtErrorPolicy "throw" is not exercised automatically because
254+
// its terminal case (a real crash) would kill the test runner, and the policy
255+
// cannot be toggled at runtime (GetAppConfigValue caches, and flipping it
256+
// would break every other spec). The whole suite staying green under the
257+
// default "report" policy is itself the pin that the claim-slot machinery is
258+
// inert when the policy is off.
259+
//
260+
// To smoke it manually, set { "uncaughtErrorPolicy": "throw" } in the app
261+
// config, then:
262+
//
263+
// 1. BOUNDARY-ORIGINATED, reported SYNCHRONOUSLY (catchable — Android parity):
264+
// boundaries that report inside their own frame via HandleBoundaryException
265+
// — overridden property getters/setters and DictionaryAdapter reads invoked
266+
// from native — rethrow the NSException at that boundary after scope
267+
// teardown. A native @try/@catch wrapping that native access catches it.
268+
// (The existing "throwing overridden getter"/"throwing DictionaryAdapter"
269+
// specs above drive exactly these boundaries; under "throw" the read would
270+
// surface a NativeScriptFatalJSException instead of a nil default.)
271+
//
272+
// IMPORTANT LIMITATION (verified empirically): the native→JS block /
273+
// overridden-method boundary (ArgConverter::MethodCallback, e.g.
274+
// invokeBlockCatchingException) does NOT get a synchronous rethrow. It uses
275+
// tc.ReThrow(), so an unbranded plain Error is surfaced to V8 as a pending
276+
// exception and is caught in JS up-stack (or reported later at an uncaught
277+
// Invoke), never during the block's own frame. A native @try/@catch around
278+
// invokeBlockCatchingException therefore does NOT catch a plain Error under
279+
// "throw" — it still surfaces to JS. Use interop.escapeException(err) to
280+
// force a synchronous native @throw at that boundary regardless of policy.
281+
//
282+
// 2. LOOP-ORIGINATED (deferred clean-frame throw): an uncaught error in a
283+
// setTimeout callback has no originating boundary to claim it, so it is
284+
// thrown from a clean, scope-free frame on the runtime loop and terminates
285+
// the app (nothing catches it) with a NativeScriptFatalJSException crash
286+
// report:
287+
//
288+
// setTimeout(() => { throw new Error("policy-throw-timer"); }, 0);
289+
//
290+
// Default-off behavior (report + continue) is covered by every other suite
291+
// here.
260292
});

0 commit comments

Comments
 (0)