feat: web-compliant error handling#1985
Open
edusperoni wants to merge 7 commits into
Open
Conversation
…r events) Ports the error model from NativeScript/ios#409 to the Android runtime. Unhandled promise rejection tracking: - Register SetPromiseRejectCallback (previously rejections vanished with no log, no callback). Rejections are tracked per-isolate and drained once per looper turn via a LooperTasks task, so a handler attached in the same turn always cancels the report. - Unhandled ones report through the existing __onUncaughtError hook with an always-on "Unhandled promise rejection:" logcat entry; late-attached handlers fire `rejectionhandled` (as a task, per spec, carrying the original reason; already-reported promises are held weakly). - Worker isolates dispatch `unhandledrejection` on the worker global first, then fall through to the existing worker onerror channel. WHATWG error events on globalThis: - Events.{h,cpp}: generic Event/EventTarget constructors, the addEventListener/removeEventListener/dispatchEvent mixin, and the internal backing target (stashed natively for future consumers). - ErrorEvents.{h,cpp}: ErrorEvent/PromiseRejectionEvent, reportError() and the native dispatch closures, layered on the generic primitives via a one-shot _installListenerErrorReporter handoff. Dispatch closures are captured at init so events keep firing even if app code overwrites globalThis.dispatchEvent. - NativeScriptException keeps the reporting machinery and the rejection tracker, mirroring the iOS file layout. preventDefault() semantics: - Uncaught errors dispatch a cancelable `error` ErrorEvent before the __onUncaughtError/__onDiscardedError hooks; preventDefault() fully handles the report. passExceptionToJsNative now returns that handled flag and NativeScriptUncaughtExceptionHandler skips the error activity and the default (crashing) handler when set. - Back-compat: unprevented errors behave exactly as before. Not ported (already native to Android): interop.escapeException - a JS throw in a method override already surfaces as a real com.tns.NativeScriptException to the Java caller, and caught Java exceptions already carry error.nativeException. crashOnUncaughtJsExceptions is likewise unnecessary - Android crashes by default on truly-uncaught exceptions. Testing: 30 new jasmine specs (WHATWG events/EventTarget semantics, reportError, unhandled rejections incl. rejectionhandled); full suite green on an API 35 arm64 emulator with no changes to existing suites.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe runtime adds WHATWG-style event primitives, native error-event dispatch, unhandled promise rejection tracking, configurable uncaught-error handling, Java exception escaping with JavaScript stack preservation, and corresponding tests and documentation. ChangesRuntime Error Handling
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Java
participant Runtime
participant ErrorEvents
participant EventTarget
Java->>Runtime: pass uncaught exception
Runtime->>ErrorEvents: dispatch error event
ErrorEvents->>EventTarget: notify error listeners
EventTarget-->>ErrorEvents: return preventDefault result
ErrorEvents-->>Runtime: return handled status
Runtime-->>Java: return handling result
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test-app/app/src/main/assets/app/tests/testErrorEvents.js (1)
38-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared
onGlobal/afterQuietTurns/pollUntiltest helpers.Both new spec files reimplement the same global-listener bookkeeping and quiet-turn waiting logic, with
afterQuietTurnsusing different (25ms vs 20ms) delays in each file. Moving these into a shared test-utility module would remove duplication and prevent timing drift between suites.
test-app/app/src/main/assets/app/tests/testErrorEvents.js#L38-L48: extractonGlobalandafterQuietTurnsinto a shared helper module imported by both spec files.test-app/app/src/main/assets/app/tests/testUnhandledRejections.js#L30-L58: replace the localonGlobal/afterQuietTurns(and optionallypollUntil) with the shared helper module, reconciling the differing timing constants.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-app/app/src/main/assets/app/tests/testErrorEvents.js` around lines 38 - 48, Extract the duplicated onGlobal and afterQuietTurns helpers into a shared test utility module, using one reconciled quiet-turn delay for both suites. In test-app/app/src/main/assets/app/tests/testErrorEvents.js:38-48, replace the local implementations with imports from that module; in test-app/app/src/main/assets/app/tests/testUnhandledRejections.js:30-58, likewise remove the local onGlobal/afterQuietTurns implementations and optionally reuse pollUntil through the shared module. Update both suites to use the shared helper consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test-app/app/src/main/assets/app/tests/testErrorEvents.js`:
- Around line 38-48: Extract the duplicated onGlobal and afterQuietTurns helpers
into a shared test utility module, using one reconciled quiet-turn delay for
both suites. In test-app/app/src/main/assets/app/tests/testErrorEvents.js:38-48,
replace the local implementations with imports from that module; in
test-app/app/src/main/assets/app/tests/testUnhandledRejections.js:30-58,
likewise remove the local onGlobal/afterQuietTurns implementations and
optionally reuse pollUntil through the shared module. Update both suites to use
the shared helper consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 51a14569-4f23-4a62-8ac5-f6f380f301bd
📒 Files selected for processing (15)
test-app/app/src/main/assets/app/mainpage.jstest-app/app/src/main/assets/app/tests/testErrorEvents.jstest-app/app/src/main/assets/app/tests/testUnhandledRejections.jstest-app/app/src/main/java/com/tns/NativeScriptUncaughtExceptionHandler.javatest-app/runtime/CMakeLists.txttest-app/runtime/src/main/cpp/ErrorEvents.cpptest-app/runtime/src/main/cpp/ErrorEvents.htest-app/runtime/src/main/cpp/Events.cpptest-app/runtime/src/main/cpp/Events.htest-app/runtime/src/main/cpp/NativeScriptException.cpptest-app/runtime/src/main/cpp/NativeScriptException.htest-app/runtime/src/main/cpp/Runtime.cpptest-app/runtime/src/main/cpp/Runtime.htest-app/runtime/src/main/cpp/com_tns_Runtime.cpptest-app/runtime/src/main/java/com/tns/Runtime.java
…tive callers) Ports the identity-preservation half of iOS's interop.escapeException (NativeScript/ios#409). The escape direction itself is already Android's default - a JS throw in a native-invoked override surfaces to the Java caller as a real com.tns.NativeScriptException - but a rethrown *Java* exception lost its identity: ReThrowToJava wrapped it, so the caller's catch of the concrete type (e.g. IOException) never matched. Now, mirroring iOS: - New `interop` global (per isolate, workers included) with interop.escapeException(x): returns a fresh JS Error branded via a private symbol whose payload carries the original Java throwable when x is/carries one (nativeException), or {name, message, stack} synthesis info otherwise. Idempotent; TypeError with no arguments. - At the JS->Java boundary, a branded error carrying a Java throwable is rethrown UNWRAPPED (env.Throw of the original object), so a native `catch (IOException e)` above the caller matches and Throwable identity is preserved. JNI does not enforce checked-exception declarations, so any Throwable propagates. - Branded errors bypass discardUncaughtJsExceptions (which only handles com.tns.NativeScriptException) - an explicit forward request, matching iOS semantics where branded escapes ignore the discard flag. - Unbranded throws keep today's semantics exactly (wrapper with the original as cause); branded plain JS errors keep the default NativeScriptException escape shape. Testing: 7 new specs (brand semantics + boundary round-trips through a new EscapeExceptionTest fixture, asserting the caller catches the original java.io.IOException by identity, and that unbranded/plain behavior is unchanged). Full suite green on an API 35 arm64 emulator.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test-app/runtime/src/main/cpp/NativeScriptException.cpp (1)
472-560: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
draining_can get stucktrueforever if a non-NativeScriptExceptionescapesDrain().Both loops only catch
NativeScriptException&, anddraining_ = false(line 559) plusPruneReportedOutstanding()(line 557) only run if nothing throws past the catch blocks. IfErrorEvents::DispatchRejectionHandled/DispatchUnhandledRejection,GetStackTraceOfValue,ToDetailString,GiveWorkerOnErrorAChance,PassUncaughtExceptionFromWorkerToParent, orPruneReportedOutstanding()throws anything other thanNativeScriptException(e.g. a JNI/std exception), it propagates out ofDrain()uncaught, leavingdraining_permanentlytrue. SinceDrain()early-returns wheneverdraining_is true (line 474-476), this silently and permanently disables all futureunhandledrejection/rejectionhandleddispatch for the isolate.Use an RAII guard so
draining_is always reset regardless of how the function exits, and widen the catches so no exception type can escape this loop:🛡️ Proposed fix
void PromiseRejectionTracker::Drain() { drainScheduled_ = false; if (draining_) { return; } draining_ = true; + struct DrainGuard { + bool& flag; + ~DrainGuard() { flag = false; } + } drainGuard{draining_}; auto isolate = runtime_->GetIsolate(); auto context = isolate->GetCurrentContext(); ... } catch (NativeScriptException& ex) { DEBUG_WRITE_FORCE( "PromiseRejectionTracker: exception while firing rejectionhandled: %s", ex.GetErrorMessage().c_str()); + } catch (...) { + DEBUG_WRITE_FORCE( + "PromiseRejectionTracker: unknown exception while firing rejectionhandled"); } ... } catch (NativeScriptException& ex) { DEBUG_WRITE_FORCE( "PromiseRejectionTracker: exception while reporting rejection: %s", ex.GetErrorMessage().c_str()); + } catch (...) { + DEBUG_WRITE_FORCE( + "PromiseRejectionTracker: unknown exception while reporting rejection"); } } PruneReportedOutstanding(); - - draining_ = false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-app/runtime/src/main/cpp/NativeScriptException.cpp` around lines 472 - 560, Update PromiseRejectionTracker::Drain to use an RAII guard that resets draining_ on every exit, including exceptions from PruneReportedOutstanding. Widen both loop-level catches, including the rejectionhandled dispatch and unhandled rejection reporting paths, so non-NativeScriptException failures cannot escape Drain; preserve the existing diagnostic logging and dispatch behavior.
🧹 Nitpick comments (1)
test-app/runtime/src/main/cpp/Interop.cpp (1)
34-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
NativeScriptException::THROWABLE_CLASSfor the throwableIsAssignableFromcheck.
GetWrappedJavaThrowableandInterop::ExtractEscapedJavaExceptionboth callenv.FindClass("java/lang/Throwable"), whileTryGetJavaThrowableObjectalready uses the cachedNativeScriptException::THROWABLE_CLASSfor the same check. Reusing the cached reference avoids duplicate JNI lookups and keeps a single throwable class reference in use.Also applies to: 179-216
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-app/runtime/src/main/cpp/Interop.cpp` around lines 34 - 65, Update GetWrappedJavaThrowable and Interop::ExtractEscapedJavaException to use the cached NativeScriptException::THROWABLE_CLASS reference for their IsAssignableFrom checks instead of calling env.FindClass("java/lang/Throwable"). Preserve the existing throwable detection behavior and apply the same reuse consistently in both methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test-app/runtime/src/main/cpp/NativeScriptException.cpp`:
- Around line 472-560: Update PromiseRejectionTracker::Drain to use an RAII
guard that resets draining_ on every exit, including exceptions from
PruneReportedOutstanding. Widen both loop-level catches, including the
rejectionhandled dispatch and unhandled rejection reporting paths, so
non-NativeScriptException failures cannot escape Drain; preserve the existing
diagnostic logging and dispatch behavior.
---
Nitpick comments:
In `@test-app/runtime/src/main/cpp/Interop.cpp`:
- Around line 34-65: Update GetWrappedJavaThrowable and
Interop::ExtractEscapedJavaException to use the cached
NativeScriptException::THROWABLE_CLASS reference for their IsAssignableFrom
checks instead of calling env.FindClass("java/lang/Throwable"). Preserve the
existing throwable detection behavior and apply the same reuse consistently in
both methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 106e6980-5592-4b35-92f3-1a57c6bc9883
📒 Files selected for processing (8)
test-app/app/src/main/assets/app/mainpage.jstest-app/app/src/main/assets/app/tests/testEscapeException.jstest-app/app/src/main/java/com/tns/tests/EscapeExceptionTest.javatest-app/runtime/CMakeLists.txttest-app/runtime/src/main/cpp/Interop.cpptest-app/runtime/src/main/cpp/Interop.htest-app/runtime/src/main/cpp/NativeScriptException.cpptest-app/runtime/src/main/cpp/Runtime.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- test-app/runtime/CMakeLists.txt
- test-app/runtime/src/main/cpp/Runtime.cpp
Companion to the iOS runtime's JS-stack DX work on NativeScript/ios#409, using the JVM primitive iOS doesn't have: Throwable.addSuppressed. - New com.tns.JavaScriptStackTrace: a never-thrown Throwable carrying a JS stack, with StackTraceElements synthesized from the V8 frames (its own Java capture is suppressed). Attached generically via attach() so future exception paths can adopt it; SDK integrations read the raw stacks through getJavaScriptStack()/getEscapeSiteStack(). - Original-throwable escapes: the boundary attaches the carrier as a suppressed exception before rethrowing, so the JS journey renders automatically in printStackTrace(), logcat fatal logs and crash reporters ("Suppressed: com.tns.JavaScriptStackTrace: ...") while the original object's identity, class, stack and cause chain stay untouched. Idempotent: the same throwable escaping twice through nested overrides gets one carrier. - Escape-site capture: escapeException() now records the stack of its own call site (the branded Error's natural V8 stack, grabbed before the origin stack overwrites it) alongside the origin stack. For non-Error values it is the only stack available and was previously lost. - Synthesized escapes: the com.tns.NativeScriptException thrown for a branded error with no underlying Java throwable gets its stack trace replaced with the synthesized JS frames, so crash reporters group these by where they actually happened in JS instead of bucketing everything under the identical JNI boundary machinery. Testing: 3 new specs (suppressed carrier contents + escape-site accessor, duplicate-carrier guard across nested escapes, non-Error escape-site fallback) plus JS-frame assertions on the synthesized path. Full suite green on an API 35 arm64 emulator.
Documents the web-compliant error model: global error/unhandledrejection/ rejectionhandled events and reportError, Java exception round-tripping (error.nativeException), interop.escapeException and the com.tns.JavaScriptStackTrace carrier, configuration flags with the terminal-path decision table, and crash-reporter integration on both the JS and Java sides. Adds a docs/ index and links it from the README, mirroring the docs added on the iOS PR (NativeScript/ios#409).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/error-handling.md`:
- Line 43: Update the cancelable `error` and `unhandledrejection` documentation
to state that `preventDefault()` suppresses downstream hooks, error activity,
and process crashes, but does not necessarily suppress native pre-dispatch
logging such as `System.err` or logger output.
- Around line 141-143: Update the Java example around the suppressed-exception
loop to declare and initialize caught before calling caught.getSuppressed(), or
move the loop into the preceding catch block where caught is already in scope,
ensuring the copied example compiles.
- Around line 129-136: Update the fenced code block containing the Java
stack-trace example to declare the text language identifier, changing the
opening fence to use text while preserving the stack-trace content unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3945e59d-d2cd-4ec0-b4d8-e907a05f318c
📒 Files selected for processing (3)
README.mddocs/README.mddocs/error-handling.md
Comment on lines
+141
to
+143
| for (Throwable suppressed : caught.getSuppressed()) { | ||
| if (suppressed instanceof com.tns.JavaScriptStackTrace) { | ||
| com.tns.JavaScriptStackTrace jsTrace = (com.tns.JavaScriptStackTrace) suppressed; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Define caught before using it.
The Java example calls caught.getSuppressed() without declaring caught, so copied code does not compile. Use a declared exception variable or place the loop inside the preceding catch block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/error-handling.md` around lines 141 - 143, Update the Java example
around the suppressed-exception loop to declare and initialize caught before
calling caught.getSuppressed(), or move the loop into the preceding catch block
where caught is already in scope, ensuring the copied example compiles.
Adopts the web's model as the default: an erroring page doesn't crash the browser, and an uncaught JS error no longer crashes the app. This is the Android half of the unified NativeScript 9.1 error-handling spec. Containment: - An uncaught throw in a NATIVE-initiated callback (the OS invoking an overridden method or interface implementation, a posted Runnable, a timer, __runOnMainThread, a frame callback) is contained at the boundary: reported through the WHATWG pipeline (cancelable `error` event -> __onUncaughtError -> logcat) and the native caller resumes with the return type's default value. preventDefault() suppresses the report entirely. - A JS-INITIATED chain (JS -> Java -> JS callback throws) keeps propagating to the outer JS catch with the original JS error object - correct JavaScript semantics, tracked via a per-isolate JS->Java call depth around CallJavaMethod/RegisterInstance. Containment applies only at an outermost native-initiated boundary. - Contained primitive-returning overrides yield the type's default (dispatchCallJSMethodNative substitutes it before unboxing), fixing the NPE the old discard path produced. - Branded interop.escapeException throws are never contained; module/ script evaluation (bootstrap) is not contained. uncaughtErrorPolicy (app package.json, root level): - "report" (default): report and continue, never crash. - "throw": unprevented errors are thrown to the native layer as real Java exceptions - com.tns.NativeScriptException with the JS frames as its stack trace - restoring the pre-9.1 behavior (typically a crash, via the thread's uncaught-exception path, which performs the single report). Unhandled rejections are thrown from a clean Java frame posted to the runtime's looper. Named for the mechanism, not a guaranteed crash: native catch-alls above the boundary can still intercept. discardUncaughtJsExceptions is deprecated (logcat warning): `true` keeps its legacy quiet routing (__onDiscardedError, no log) on top of the new default; `false` - which used to mean "crash" - is ignored. Branded escapes are now exempt from the discard handling (escapedFromJs marker). Testing: 5 new specs (containment + app liveness with event/hook identity, preventDefault suppression, timer containment, primitive default instead of NPE, JS-initiated chain identity) plus the direct-throw escape specs; "throw" policy is a documented manual smoke. Full suite green on an API 35 arm64 emulator - every pre-existing suite passes unchanged thanks to the JS-initiated-chain rule.
… 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.
Separates native-layer deaths from the WHATWG `error` event, completing the invariant that error/unhandledrejection only fire while the failure is still containable (the app fully alive, preventDefault() honest). - New cancelable `nativeuncaughterror` ErrorEvent, dispatched synchronously from the uncaught-exception handler for everything that reaches it un-marked: pure-native Java crashes, uncaught interop.escapeException forwards, bootstrap failures. The handler classifies nothing - the simple rule IS the spec. e.error carries the wrapped original Throwable via nativeException. preventDefault() is best-effort: it skips the error activity and the default (killing) handler - meaningful for background-thread crashes (this is what keeps the new spec's test runner alive). The legacy __onUncaughtError hook still fires for unprevented native crashes (deprecated back-compat - it is what Application.uncaughtErrorEvent has received for years). This also removes the blanket-preventDefault footgun: a generic `error` listener can no longer accidentally spare a process whose crashed thread is already gone. - Main-runtime fallback: crashes on threads with no runtime of their own (plugin/executor threads) previously got NO JS reporting at all - Runtime.isInitialized() and getCurrentRuntime() are thread-local, so the handler silently skipped them. The handler now resolves current -> main (new Runtime.getMainRuntime(), set when the workerId-0 runtime initializes) and enters the main isolate cross-thread (the JNI layer takes the v8::Locker). - Already-reported fast path: an uncaughtErrorPolicy "throw" exception (isReportedToJs) now short-circuits the handler before any work - no JS roundtrip and no eager stack rendering; the message strings are built lazily, only when the logger, debug print or error activity actually consumes them. Testing: new spec crashes a fresh runtime-less Java thread and asserts nativeuncaughterror arrives via the main-runtime fallback carrying the original Throwable, `error` never fires (invariant pin), the prevented report skips the legacy hook, and the process survives. Full suite green on an API 35 arm64 emulator: 605 specs, 0 failures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Ports the web-compliant error model from NativeScript/ios#409 to the Android runtime: unhandled promise rejection tracking, WHATWG error events, and
interop.escapeException— with the same file layout as the iOS implementation (Events.{h,cpp}for the generic primitives,ErrorEvents.{h,cpp}for the error layer, reporting machinery and the rejection tracker inNativeScriptException).Unhandled promise rejection tracking
The runtime never registered
SetPromiseRejectCallback, so unhandled rejections vanished with no log, no callback, no crash. Now:LooperTaskstask — the Android analogue of the iOS runloop observer), so a handler attached in the same turn always cancels the report.__onUncaughtErrorhook, prefixedUnhandled promise rejection:in logcat.rejectionhandled(as a task, per spec, carrying the original reason; already-reported promises are held weakly so GC'd ones drop out).unhandledrejectionon the worker global first, then flow through the existing worker error channel (worker-globalonerror→ parentWorker.onerror).WHATWG error events on globalThis
Spec-shaped
Event,EventTarget,ErrorEvent,PromiseRejectionEventconstructors,addEventListener/removeEventListener/dispatchEventonglobalThis, and globalreportError()— same surface as the iOS runtime, workers and Deno.errorErrorEvent; unhandled rejections a cancelableunhandledrejectionPromiseRejectionEvent.preventDefault()= fully handled (no__onUncaughtError/__onDiscardedErrorhook, no crash):passExceptionToJsNativenow returns the handled flag, andNativeScriptUncaughtExceptionHandlerskips the error activity and the default (crashing) handler when it is set.globalThis.dispatchEvent.__onUncaughtError/__onDiscardedErrorkeep working, so current core needs zero changes.interop.escapeExceptionNew
interopglobal (mirroring iOS) withinterop.escapeException(x). The escape direction is already Android's default — a JS throw in a native-invoked override surfaces to the Java caller as a realcom.tns.NativeScriptException— but a rethrown Java exception lost its identity: the boundary wrapped it, so a nativecatchof the concrete type never matched. Now:Throwableidentity; branding is idempotent,TypeErrorwith no arguments.discardUncaughtJsExceptions(an explicit forward request, matching iOS where branded escapes ignore the discard flag).com.tns.JavaScriptStackTrace(never thrown; API kept generic so other exception paths can adopt it) is attached to the escaped original viaThrowable.addSuppressed, with the JS frames synthesized as realStackTraceElements — so the JS journey renders automatically inprintStackTrace(), logcat fatal logs and crash reporters (Suppressed: com.tns.JavaScriptStackTrace: ...) while the original object's identity, class, stack and cause chain stay untouched. Idempotent across nested re-escapes.escapeException()also records its own call-site stack (previously lost; for non-Error values it's the only stack there is), exposed with the origin stack viagetJavaScriptStack()/getEscapeSiteStack()for crash-SDK integrations.com.tns.NativeScriptExceptionas before, but with its stack trace replaced by the synthesized JS frames — so crash reporters group them by where they actually happened in JS instead of bucketing everything under the identical JNI boundary machinery. Unbranded throws keep today's semantics exactly.crashOnUncaughtJsExceptions— Android already crashes by default on truly-uncaught exceptions (discardUncaughtJsExceptionsandpreventDefault()are the opt-outs).Containment by default +
uncaughtErrorPolicy(the 9.1 model)Adopts the web's model as the default: an erroring page doesn't crash the browser, and an uncaught JS error no longer crashes the app.
Runnable, a timer,__runOnMainThread, a frame callback) is contained at the boundary: reported through the pipeline (cancelableerrorevent →__onUncaughtError→ logcat) and the native caller resumes with the return type's default value (primitives get0/false— the dispatch layer substitutes before unboxing, fixing the NPE the old discard path produced).preventDefault()suppresses the report entirely — and since the report now happens synchronously at the boundary, it is fully meaningful on every thread.catchwith the original JS error object — correct JS semantics, tracked via a per-isolate JS→Java call depth. Containment applies only at an outermost native-initiated boundary.uncaughtErrorPolicy(apppackage.json, root):"report"(default) or"throw"— the full report (cancelable event → hook → log) runs first at the decision point (preventDefault()fully contains the error even under"throw", matching iOS), then the error is thrown to the native layer as a real Java exception with the JS frames as its stack trace, markedisReportedToJs()so the uncaught-exception path never reports the same failure twice — one event per failure on both platforms. Restores the pre-9.1 behavior (typically a crash; named for the mechanism, not a guaranteed outcome). Unhandled rejections under"throw"are thrown from a clean Java frame on the runtime's looper.error.stackTrace/reason.stackTraceare populated before the events dispatch, matching iOS's pre-dispatch fix.discardUncaughtJsExceptionsis deprecated (logcat warning):truekeeps its legacy quiet routing (__onDiscardedError) on top of the new default;false— which used to mean "crash" — is ignored rather than repurposed. Branded escapes are now exempt from discard handling.discardUncaughtJsExceptions: trueplus the event layer;uncaughtErrorPolicy: "throw"≡ the old default with better crash-reporter grouping.nativeuncaughterror— the native-layer death notificationNative crashes get their own event, completing the invariant that
error/unhandledrejectionfire only while the failure is still containable (preventDefault()honest, app fully alive). Everything reaching the uncaught-exception handler un-marked — pure-native Java crashes, uncaughtescapeExceptionforwards, bootstrap failures — dispatches a cancelablenativeuncaughterror(synchronously, on the crashing thread, entering the isolate under thev8::Locker), withe.error.nativeExceptioncarrying the originalThrowable.preventDefault()is best-effort (skips the error activity and the killing handler — meaningful for background threads; ignored on iOS where termination is unavoidable). Crashes on threads with no runtime of their own — previously invisible to JS entirely, sincegetCurrentRuntime()/isInitialized()are thread-local — now fall back to the newRuntime.getMainRuntime(). Already-reported"throw"-policy exceptions short-circuit the handler with zero work. Legacy__onUncaughtErrorkeeps firing for unprevented native crashes (back-compat).Docs: the full model — events,
escapeException,JavaScriptStackTrace, config decision table, crash-reporter integration — is documented indocs/error-handling.md, mirroring the docs on the iOS PR.Behavior changes (default config): uncaught JS errors in native-initiated callbacks no longer crash the app — they are reported and contained (set
uncaughtErrorPolicy: "throw"for the old behavior); a throwing override hands its Java caller the return type's default value, so null-hostile callers may fail downstream (the real error is logged first;interop.escapeExceptionis the out for contracts that need the exception). Unhandled rejections are now reported (previously silent). JS-initiated chains, module/script bootstrap errors,preventDefault()andescapeExceptionsemantics are as described above.Related Pull Requests
Events/ErrorEventssplit)Does your pull request have unit tests?
Yes — 40 new jasmine specs:
testErrorEvents.js(ErrorEvent/PromiseRejectionEvent constructors, EventTarget semantics,reportError,preventDefault()suppressing the__onUncaughtError/__onDiscardedErrorhooks, dispatch resilience toglobalThis.dispatchEventbeing overwritten),testUnhandledRejections.js(reporting ofPromise.reject/async throws/.thenthrows, same-turn.catchcancelling the report,unhandledrejection+preventDefault(),rejectionhandledfor late handlers), andtestEscapeException.js(brand semantics plus JS→Java boundary round-trips through a newEscapeExceptionTestfixture: the native caller catches the originaljava.io.IOExceptionby identity, the suppressedJavaScriptStackTracecarrier renders frames pointing at the spec file, duplicate carriers are not stacked across nested re-escapes, synthesized escapes carry JS frames, non-Error escapes fall back to the escape-site stack, and unbranded behavior is unchanged). A newtestUncaughtErrorPolicy.jssuite covers containment (app liveness + event/hook with thrown-value identity),preventDefault()suppression, timer containment, the primitive-default return, and JS-initiated-chain identity;uncaughtErrorPolicy: "throw"is a documented manual smoke (a real crash would kill the runner). Full suite green on an API 35 arm64 emulator — every pre-existing suite passes unchanged under the new default thanks to the JS-initiated-chain rule.Summary by CodeRabbit
error,unhandledrejection,rejectionhandled) with correct dispatch, cancellation, and listeneroptionsbehavior.interop.escapeExceptionfor JS→Java exception escaping with preserved identity and stack context.uncaughtErrorPolicyapp configuration ("report"/"throw").preventDefault()consumption and deliversrejectionhandledreliably.interop.escapeException, and policy behavior indocs/error-handling.md.