Skip to content

Commit 10c006a

Browse files
committed
fix: single report under "throw" policy, pre-dispatch stackTrace (iOS parity)
Parity round with NativeScript/ios#409's claim-slot commit, closing the review's cross-platform divergences: - Single report per failure under uncaughtErrorPolicy: "throw": the full report (cancelable event -> hook -> log) now runs at the decision point BEFORE the native throw, for both the sync boundary and the rejection drain, and the thrown com.tns.NativeScriptException is marked reportedToJs (isolate-private brand carried to a Java field by ReThrowToJava; set directly for the rejection looper throw) so NativeScriptUncaughtExceptionHandler skips the post-mortem re-report. Previously a "throw"-policy rejection fired unhandledrejection AND a second post-mortem error event. The public NativeScriptException.isReportedToJs() lets custom Thread.UncaughtExceptionHandler implementations honor the same contract. - preventDefault() now beats the "throw" policy at the sync boundary too: the event dispatches before the propagation decision, so a prevented error is fully contained with the app alive (iOS behavior). The deprecated discard flag disables the throw, matching iOS. - error.stackTrace / reason.stackTrace (the combined legacy stack, a NativeScript extension) are populated BEFORE the error / unhandledrejection events dispatch - including the worker branch of the rejection drain - so listeners see the same shape the hooks do; pinned by new spec assertions mirroring the iOS ones. - Docs: unified "throw" sequencing and the cross-platform catchability note (Android sync at the method boundary; iOS sync only at own-frame-reporting boundaries, deferred for block/method callbacks and loop-originated errors; escapeException is the portable per-call interception tool), plus the Android-only native-crash event asymmetry and the interop feature-detection note. Full suite green on an API 35 arm64 emulator: 604 specs, 0 failures.
1 parent 029b155 commit 10c006a

7 files changed

Lines changed: 151 additions & 29 deletions

File tree

docs/error-handling.md

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,7 @@ throw interop.escapeException(new java.io.IOException("x"));
120120
- Otherwise a `com.tns.NativeScriptException` is thrown as usual, but with its stack trace replaced by frames synthesized from the JS stack, so crash reporters group it by where it actually happened in JS.
121121
- The `escapeException()` call site's stack is recorded too — for non-Error values (`escapeException("boom")`) it is the only stack available.
122122
- Branded escapes bypass `discardUncaughtJsExceptions` (an explicit forward request must reach the caller).
123+
- The `interop` global is new in this release with `escapeException` as its only member — shared code targeting older runtimes should feature-detect: `global.interop?.escapeException`.
123124

124125
## Native (Java) API
125126

@@ -174,7 +175,7 @@ One policy key in the app's `package.json` (root level) governs what happens to
174175
| `uncaughtErrorPolicy` | Effect |
175176
|---|---|
176177
| `"report"` (default) | Report (event → hook → log) and continue. Never crashes. |
177-
| `"throw"` | After the (cancelable) event, the error is thrown to the native layer as a real Java exception — `com.tns.NativeScriptException` with the JS frames as its stack trace — and reported through the thread's uncaught-exception path. This typically ends the process (the pre-9.1 default), though a native catch or a custom `UncaughtExceptionHandler` above the boundary can still intercept it, which is why the policy names the mechanism, not a guaranteed crash. Unhandled rejections are thrown from a clean frame on the runtime's looper. |
178+
| `"throw"` | The full report runs first, at the decision point — cancelable event (`preventDefault()` still fully contains the error, same as iOS), then hook and log — and the error is *then* thrown to the native layer as a real Java exception: `com.tns.NativeScriptException` with the JS frames as its stack trace, marked `isReportedToJs()` so the uncaught-exception path does not report the same failure twice. This typically ends the process (the pre-9.1 default), though the policy names the mechanism, not a guaranteed crash. Unhandled rejections are thrown from a clean frame on the runtime's looper. **Cross-platform note:** on Android the sync throw unwinds through the native caller at the method boundary, so a Java `try/catch` above it can intercept; on iOS the rethrow is synchronous (catchable) only at boundaries that report within their own frame (property accessors, adapter reads) — block/overridden-method callbacks and loop-originated errors fall back to a deferred clean-frame throw. Portable code that needs a native-interceptable exception for a *specific* call should use `interop.escapeException`, which behaves identically on both platforms at every boundary; the policy governs unprevented, already-reported errors only. |
178179

179180
Deprecated (kept for the transition, both emit a logcat warning):
180181

@@ -189,8 +190,9 @@ Terminal-path decision table:
189190
|---|---|---|
190191
| uncaught error, default (`"report"`) | `__onUncaughtError` | no |
191192
| uncaught error, `"report"` + `discardUncaughtJsExceptions: true` | `__onDiscardedError` | no |
192-
| uncaught error, listener called `preventDefault()` | none | no |
193-
| uncaught error / unhandled rejection, `"throw"`, unprevented | `__onUncaughtError` (via the uncaught-exception path) | yes, normally |
193+
| uncaught error, listener called `preventDefault()` (either policy) | none | no |
194+
| uncaught error / unhandled rejection, `"throw"`, unprevented | `__onUncaughtError` (at the decision point, before the throw) | yes, normally |
195+
| uncaught error / unhandled rejection, `"throw"` + `discardUncaughtJsExceptions: true` | `__onDiscardedError` | no (discard disables the throw, matching iOS) |
194196
| unhandled rejection / `reportError`, `"report"`, unprevented | `__onUncaughtError` | no |
195197
| unhandled rejection / `reportError`, `preventDefault()` | none | no |
196198

@@ -220,7 +222,9 @@ Java side — for exceptions that never pass through the JS event layer (escaped
220222

221223
- **Containment is boundary-outermost.** The runtime tracks the depth of in-flight JS→Java calls; a throw with JS frames waiting below the boundary propagates (so `try { javaApi.call(cb) } catch` works, with the original JS error object restored across the crossing), and is contained only at an outermost native-initiated entry. `interop.escapeException` and `uncaughtErrorPolicy: "throw"` are the two ways an error crosses that outermost boundary.
222224
- **Contained callbacks return type defaults.** A throwing overridden method hands its Java caller `null` for reference types and `0`/`false` for primitives (the runtime substitutes the default before unboxing, so no `NullPointerException` from the binding). The error is loudly reported *before* the caller resumes, so logcat shows the real failure ahead of any downstream symptom. If the Java contract genuinely needs the exception, use `interop.escapeException`.
223-
- Every error is reported exactly once: either the containment point, the rejection drain (once per looper turn, scheduled on the runtime's `ALooper`), `reportError`, or — under `"throw"` — the thread's uncaught-exception path. Never two of them for the same error.
225+
- Every error is reported exactly once, at its decision point: the containment boundary, the rejection drain (once per looper turn, scheduled on the runtime's `ALooper`), or `reportError`. Under `"throw"` the report still happens at the decision point and the thrown `NativeScriptException` carries `isReportedToJs()`, which the uncaught-exception handler honors by not reporting again — one event per failure on both platforms.
226+
- The legacy `stackTrace` property (combined JS + Java frames, a NativeScript extension) is set on the error/reason **before** the event dispatches, so listeners and hooks see the same shape. The standard `e.error.stack` is always there for spec-shaped code.
224227
- A rejection that gets a handler before the end-of-turn drain is never reported (and produces no `rejectionhandled` either).
228+
- **Android-only asymmetry:** the `error` event also fires for *pure-native* Java uncaught exceptions — the app-level `NativeScriptUncaughtExceptionHandler` reports any crashing `Throwable` through `passUncaughtExceptionToJs`, with no iOS analogue. `preventDefault()` there suppresses the error activity and the default (killing) handler, but the throwing thread has already unwound — on the main thread the looper is gone, so realistic recovery is limited to background threads.
225229
- Module/script evaluation (`runModule`/`runScript`, app bootstrap) is not contained — an app whose main module fails to load still fails loudly.
226230
- Worker isolates run the same machinery: each worker has its own tracker, drain, event layer and containment.

test-app/app/src/main/assets/app/tests/testUncaughtErrorPolicy.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,10 @@ describe("uncaughtErrorPolicy (default: report)", function () {
7373
expect(received).not.toBeNull();
7474
// The event carries the actual thrown value.
7575
expect(received.error).toBe(reason);
76+
// The combined `stackTrace` string is populated BEFORE dispatch,
77+
// so listeners can read it (not only e.error.stack).
78+
expect(typeof received.error.stackTrace).toBe("string");
79+
expect(received.error.stackTrace.length).toBeGreaterThan(0);
7680
// Unprevented, so the legacy hook fired too.
7781
expect(uncaughtSeen(reason)).toBe(true);
7882
// And the app is still running.

test-app/app/src/main/assets/app/tests/testUnhandledRejections.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,10 @@ describe("unhandled promise rejections", function () {
132132
expect(received.type).toBe("unhandledrejection");
133133
expect(received.reason).toBe(reason);
134134
expect(typeof received.promise.then).toBe("function");
135+
// The drain sets a combined `stackTrace` on the (object) reason
136+
// BEFORE dispatching the event, so a listener sees it.
137+
expect(typeof received.reason.stackTrace).toBe("string");
138+
expect(received.reason.stackTrace.length).toBeGreaterThan(0);
135139
afterQuietTurns(function () {
136140
expect(reportedSeen(reason)).toBe(false);
137141
done();

test-app/app/src/main/java/com/tns/NativeScriptUncaughtExceptionHandler.java

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,12 @@ public void uncaughtException(Thread thread, Throwable ex) {
2424

2525
boolean handledByJs = false;
2626

27-
if (Runtime.isInitialized()) {
27+
// An uncaughtErrorPolicy: "throw" exception was already fully reported
28+
// to JS (event + hook + log) at the throw decision point - reporting
29+
// it again here would double-dispatch the same failure.
30+
boolean alreadyReportedToJs = ex instanceof NativeScriptException && ((NativeScriptException) ex).isReportedToJs();
31+
32+
if (Runtime.isInitialized() && !alreadyReportedToJs) {
2833
try {
2934
if (Util.isDebuggableApp(context)) {
3035
System.err.println(errorMessage);

test-app/runtime/src/main/cpp/NativeScriptException.cpp

Lines changed: 108 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,10 @@ using namespace std;
1818
using namespace tns;
1919
using namespace v8;
2020

21+
/* Defined below, next to ContainUncaughtCallbackException. */
22+
static void MarkReportedToJs(Isolate* isolate, Local<Value> error);
23+
static bool IsMarkedReportedToJs(Isolate* isolate, Local<Value> error);
24+
2125
NativeScriptException::NativeScriptException(JEnv& env)
2226
: m_javascriptException(nullptr) {
2327
m_javaException = JniLocalRef(env.ExceptionOccurred());
@@ -157,6 +161,16 @@ void NativeScriptException::ReThrowToJava() {
157161
env.GetFieldID(NATIVESCRIPTEXCEPTION_CLASS, "escapedFromJs", "Z");
158162
env.SetBooleanField(ex, escapedFromJsField, JNI_TRUE);
159163
}
164+
165+
// A `uncaughtErrorPolicy: "throw"` propagation whose report already ran
166+
// at the boundary: carry the mark so the post-mortem uncaught path does
167+
// not report the same failure a second time. `ex` is always a
168+
// com.tns.NativeScriptException at this point.
169+
if (ex != nullptr && IsMarkedReportedToJs(isolate, errObj)) {
170+
static jfieldID reportedToJsField =
171+
env.GetFieldID(NATIVESCRIPTEXCEPTION_CLASS, "reportedToJs", "Z");
172+
env.SetBooleanField(ex, reportedToJsField, JNI_TRUE);
173+
}
160174
} else if (!m_message.empty()) {
161175
JniLocalRef msg(env.NewStringUTF(m_message.c_str()));
162176
JniLocalRef stackTrace(env.NewStringUTF(m_stackTrace.c_str()));
@@ -324,27 +338,37 @@ void NativeScriptException::ReportUnhandledRejection(Isolate* isolate,
324338
Local<Promise> promise,
325339
Local<Value> reason,
326340
const string& stackTrace) {
341+
// Populate the legacy `stackTrace` property before dispatch so event
342+
// listeners see the same shape the hooks do (matches the sync path).
343+
if (reason->IsObject() && !stackTrace.empty()) {
344+
auto context = isolate->GetCurrentContext();
345+
reason.As<Object>()
346+
->Set(context, V8StringConstants::GetStackTrace(isolate),
347+
ArgConverter::ConvertToV8String(isolate, stackTrace))
348+
.FromMaybe(false);
349+
}
350+
327351
if (ErrorEvents::DispatchUnhandledRejection(isolate, promise, reason)) {
328352
return;
329353
}
330354

331355
auto runtime = GetRuntimeOrNull(isolate);
332-
if (runtime != nullptr &&
356+
bool discard =
357+
runtime != nullptr && runtime->GetDiscardUncaughtJsExceptions();
358+
ReportFatalTail(isolate, reason, stackTrace, "Unhandled promise rejection:",
359+
discard ? JNI_TRUE : JNI_FALSE);
360+
361+
// The report is complete (event + hook + log, exactly once). Under the
362+
// "throw" policy the rejection is additionally handed to the native layer;
363+
// the thrown exception is marked reported-to-JS so the post-mortem
364+
// uncaught path does not report it a second time. The deprecated discard
365+
// flag disables the throw, matching iOS.
366+
if (runtime != nullptr && !discard &&
333367
runtime->GetUncaughtErrorPolicy() == Runtime::UncaughtErrorPolicy::Throw) {
334-
// Hand the rejection to the native layer; the post-mortem uncaught path
335-
// performs the (single) report, exactly as for a sync uncaught error.
336368
string message =
337369
"Unhandled promise rejection: " + ToDetailString(isolate, reason);
338370
ThrowUncaughtJsErrorToJava(message, stackTrace);
339-
return;
340371
}
341-
342-
jboolean isDiscarded =
343-
(runtime != nullptr && runtime->GetDiscardUncaughtJsExceptions())
344-
? JNI_TRUE
345-
: JNI_FALSE;
346-
ReportFatalTail(isolate, reason, stackTrace, "Unhandled promise rejection:",
347-
isDiscarded);
348372
}
349373

350374
void NativeScriptException::ReportFatalTail(Isolate* isolate,
@@ -389,6 +413,38 @@ void NativeScriptException::ReportFatalTail(Isolate* isolate,
389413
}
390414
}
391415

416+
/*
417+
* Marks a JS error value whose report already ran (event + hook + log), via
418+
* an isolate-private symbol. ReThrowToJava carries the mark over to the Java
419+
* com.tns.NativeScriptException so the post-mortem uncaught-exception path
420+
* does not report the same failure a second time.
421+
*/
422+
static Local<Private> GetReportedToJsBrand(Isolate* isolate) {
423+
return Private::ForApi(
424+
isolate, ArgConverter::ConvertToV8String(isolate, "tns::reportedToJs"));
425+
}
426+
427+
static void MarkReportedToJs(Isolate* isolate, Local<Value> error) {
428+
if (!error->IsObject()) {
429+
return;
430+
}
431+
auto context = isolate->GetCurrentContext();
432+
error.As<Object>()
433+
->SetPrivate(context, GetReportedToJsBrand(isolate),
434+
v8::True(isolate))
435+
.FromMaybe(false);
436+
}
437+
438+
static bool IsMarkedReportedToJs(Isolate* isolate, Local<Value> error) {
439+
if (!error->IsObject()) {
440+
return false;
441+
}
442+
auto context = isolate->GetCurrentContext();
443+
return error.As<Object>()
444+
->HasPrivate(context, GetReportedToJsBrand(isolate))
445+
.FromMaybe(false);
446+
}
447+
392448
bool NativeScriptException::ContainUncaughtCallbackException(Isolate* isolate,
393449
v8::TryCatch& tc) {
394450
auto runtime = GetRuntimeOrNull(isolate);
@@ -409,7 +465,7 @@ bool NativeScriptException::ContainUncaughtCallbackException(Isolate* isolate,
409465
}
410466

411467
// Branded interop.escapeException: an explicit forward to the native
412-
// caller - never contained.
468+
// caller - never contained, never reported here.
413469
if (error->IsObject()) {
414470
JEnv env;
415471
Interop::EscapedExceptionInfo escapeInfo;
@@ -421,14 +477,10 @@ bool NativeScriptException::ContainUncaughtCallbackException(Isolate* isolate,
421477
}
422478
}
423479

424-
if (runtime->GetUncaughtErrorPolicy() == Runtime::UncaughtErrorPolicy::Throw) {
425-
// Pre-9.1 semantics: the exception is thrown to the Java caller and the
426-
// post-mortem uncaught-exception path performs the (single) report.
427-
return false;
428-
}
429-
430-
// Contain: report through the WHATWG pipeline (`error` event -> legacy
431-
// hook -> log) and let the caller resume with a default value.
480+
// Report through the WHATWG pipeline at the decision point, for both
481+
// policies (same sequence as iOS): `error` event first - preventDefault()
482+
// fully contains the error even under the "throw" policy - then the legacy
483+
// hook and log.
432484
string errorMessage;
433485
string stackTrace;
434486
auto message = tc.Message();
@@ -440,20 +492,42 @@ bool NativeScriptException::ContainUncaughtCallbackException(Isolate* isolate,
440492
stackTrace = GetStackTraceOfValue(isolate, error);
441493
}
442494

495+
// Populate the legacy `stackTrace` property before dispatch so event
496+
// listeners see the same shape the hooks do.
497+
if (error->IsObject() && !stackTrace.empty()) {
498+
auto context = isolate->GetCurrentContext();
499+
error.As<Object>()
500+
->Set(context, V8StringConstants::GetStackTrace(isolate),
501+
ArgConverter::ConvertToV8String(isolate, stackTrace))
502+
.FromMaybe(false);
503+
}
504+
505+
bool discard = runtime->GetDiscardUncaughtJsExceptions();
443506
{
444507
// A throwing listener or hook must not escape into the caller.
445508
v8::TryCatch reportTc(isolate);
446-
if (!ErrorEvents::DispatchError(isolate, error, errorMessage, stackTrace)) {
447-
jboolean isDiscarded =
448-
runtime->GetDiscardUncaughtJsExceptions() ? JNI_TRUE : JNI_FALSE;
449-
ReportFatalTail(isolate, error, stackTrace, "", isDiscarded);
509+
if (ErrorEvents::DispatchError(isolate, error, errorMessage, stackTrace)) {
510+
tc.Reset();
511+
return true;
450512
}
513+
ReportFatalTail(isolate, error, stackTrace, "",
514+
discard ? JNI_TRUE : JNI_FALSE);
451515
if (reportTc.HasCaught()) {
452516
DEBUG_WRITE_FORCE(
453517
"ContainUncaughtCallbackException: exception while reporting");
454518
}
455519
}
456520

521+
// The report is complete (exactly once). Under the "throw" policy the
522+
// exception is additionally propagated to the Java caller, marked so the
523+
// post-mortem uncaught path skips re-reporting. The deprecated discard
524+
// flag disables the throw, matching iOS.
525+
if (!discard &&
526+
runtime->GetUncaughtErrorPolicy() == Runtime::UncaughtErrorPolicy::Throw) {
527+
MarkReportedToJs(isolate, error);
528+
return false;
529+
}
530+
457531
tc.Reset();
458532
return true;
459533
}
@@ -637,6 +711,16 @@ void PromiseRejectionTracker::Drain() {
637711

638712
string stackTrace = GetStackTraceOfValue(isolate, reason);
639713

714+
// Populate the legacy `stackTrace` property before dispatch so event
715+
// listeners see it - covers both the worker and main branches below
716+
// (ReportUnhandledRejection's own set is idempotent).
717+
if (reason->IsObject() && !stackTrace.empty()) {
718+
reason.As<Object>()
719+
->Set(context, V8StringConstants::GetStackTrace(isolate),
720+
ArgConverter::ConvertToV8String(isolate, stackTrace))
721+
.FromMaybe(false);
722+
}
723+
640724
if (workerWrapper != nullptr) {
641725
// Dispatch the rejection event on the worker's own global first;
642726
// preventDefault() there fully handles it. Only when unprevented fall

test-app/runtime/src/main/java/com/tns/NativeScriptException.java

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,28 @@ public class NativeScriptException extends RuntimeException {
1212
* discardUncaughtJsExceptions handling.
1313
*/
1414
private boolean escapedFromJs = false;
15+
/*
16+
* Set (natively or by the runtime) when this exception's failure was
17+
* already reported to JS (error/unhandledrejection event, hooks, log) at
18+
* the uncaughtErrorPolicy: "throw" decision point. Custom
19+
* Thread.UncaughtExceptionHandler implementations should skip
20+
* Runtime.passUncaughtExceptionToJs for such exceptions so the failure
21+
* is not reported twice.
22+
*/
23+
private boolean reportedToJs = false;
1524

1625
boolean isEscapedFromJs() {
1726
return escapedFromJs;
1827
}
1928

29+
void markReportedToJs() {
30+
reportedToJs = true;
31+
}
32+
33+
public boolean isReportedToJs() {
34+
return reportedToJs;
35+
}
36+
2037
public NativeScriptException() {
2138
super();
2239
}

test-app/runtime/src/main/java/com/tns/Runtime.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1375,6 +1375,10 @@ private static Object defaultPrimitiveValue(Class<?> type) {
13751375
private static void throwUncaughtJsErrorOnCurrentThread(String message, String stackTrace) {
13761376
final NativeScriptException ex = new NativeScriptException(message, stackTrace, 0);
13771377
JavaScriptStackTrace.applyFrames(ex, stackTrace, null);
1378+
// The failure was already reported (event + hook + log) at the throw
1379+
// decision point - the uncaught-exception handler must not report it
1380+
// a second time.
1381+
ex.markReportedToJs();
13781382
android.os.Looper looper = android.os.Looper.myLooper();
13791383
if (looper == null) {
13801384
throw ex;

0 commit comments

Comments
 (0)