feat: web-compliant error handling (unhandled rejections, error events, interop.escapeException)#409
Open
edusperoni wants to merge 4 commits into
Open
feat: web-compliant error handling (unhandled rejections, error events, interop.escapeException)#409edusperoni wants to merge 4 commits into
edusperoni wants to merge 4 commits into
Conversation
Register SetPromiseRejectCallback and track rejected promises without handlers in a per-isolate PromiseRejectionTracker (owned by Caches). A kCFRunLoopBeforeWaiting observer drains the pending list once per runloop turn and reports each rejection through the same __onUncaughtError/__onDiscardedError machinery as uncaught sync exceptions (log-prefixed 'Unhandled promise rejection:'). A handler attached before the drain cancels the report. Worker isolates give the worker-global onerror a chance first, then forward to the main isolate's worker.onerror like uncaught worker errors. OnUncaughtError's reporting tail is refactored into the shared ReportToJsHandlersAndLog helper (behavior preserved); the observer polls an atomic pending count since rejections can arrive from any thread holding the v8::Locker, and the drain catches NSExceptions so they cannot unwind through the observer's live V8 scopes.
Add a JS bootstrap (InitErrorEvents, evaluated for main and worker isolates right after PromiseProxy::Init) providing spec-shaped Event, EventTarget, ErrorEvent and PromiseRejectionEvent constructors, the EventTarget methods on globalThis, and a global reportError(). Uncaught exceptions now dispatch a cancelable 'error' ErrorEvent and unhandled rejections a cancelable 'unhandledrejection' PromiseRejectionEvent before reporting; preventDefault() fully handles the error (no __on* shim, no fatal log, no modal). Unprevented errors keep the existing behavior byte-for-byte, so NativeScript core's __onUncaughtError/__onDiscardedError hooks continue to work unchanged. A handler attached after reporting fires 'rejectionhandled' as a task on the next drain turn; already-reported promises are held phantom-weak so GC'd promises drop out. Native code dispatches through closures returned by the bootstrap IIFE (stashed as Persistents in Caches), so the events keep firing even if app code overwrites globalThis.dispatchEvent. Listener-thrown errors route to the fatal tail without recursive dispatch.
Preserve the original NSException when a native call throws: the JS error now carries name/message from the exception and the wrapped original as error.nativeException (re-enables the long-dormant Throw_ObjC_exceptions_to_JavaScript test everywhere - libffi 3.7.1 fixed simulator unwinding). Add interop.escapeException(x): returns a JS Error branded with an isolate-private symbol carrying either the original NSException or synthesis info. When a branded throw reaches a JS-to-native boundary (overridden methods and JS-backed blocks via ArgConverter's MethodCallback, property accessors, adapters), the boundary converts it to a real @throw into the native caller - always after every V8 scope has destructed (pendingThrow captured before the scopes, thrown after the inner block). Unbranded throws keep today's semantics: ReThrow to the message listener in MethodCallback, and report-once-plus-defined- default at the former tns::Assert abort sites (property getters/ setters, Array/Dictionary/FastEnumeration adapters no longer abort the process on a JS throw). Add opt-in crashOnUncaughtJsExceptions (default off): unprevented fatal errors schedule an uncaught @throw on the runtime loop from a clean, V8-scope-free frame so crash reporters capture a real native crash with the JS stack in userInfo. Branded escapes reaching the fatal tail (e.g. thrown from an error-event listener) always take that path.
🚥 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 |
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 `@NativeScript/runtime/ArgConverter.h`:
- Around line 68-85: Update GetEscapeExceptionBrand to validate the cache with
cache->IsValid() before calling v8::Private::New or creating any persistent
state. Preserve the documented empty-handle return for both null and invalid
caches, including the dummy cache returned after removal.
In `@NativeScript/runtime/MetadataBuilder.mm`:
- Around line 1166-1170: Update the fallback wrapper creation in the setter path
around self_ to retain the newly created ObjCDataWrapper and invoke
tns::DeleteWrapperIfUnused(...) after creating the JavaScript wrapper, mirroring
the getter cleanup. Preserve the cached-instance path and ensure cleanup applies
only when thiz is not already cached.
In `@NativeScript/runtime/NativeScriptException.h`:
- Around line 66-71: Update the rejection tracking state around
DispatchRejectionHandledEvent and reportedOutstanding_ so each reported promise
retains its original rejection reason alongside the promise. When moving entries
into pendingRejectionHandled_, carry that stored reason through and dispatch
rejectionhandled with it instead of allowing reason to become undefined.
🪄 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
Run ID: f59b87c7-4598-4217-955d-2ba7d862ab38
📒 Files selected for processing (25)
NativeScript/runtime/ArgConverter.hNativeScript/runtime/ArgConverter.mmNativeScript/runtime/ArrayAdapter.mmNativeScript/runtime/Caches.cppNativeScript/runtime/Caches.hNativeScript/runtime/ClassBuilder.mmNativeScript/runtime/DataWrapper.hNativeScript/runtime/DictionaryAdapter.mmNativeScript/runtime/FastEnumerationAdapter.mmNativeScript/runtime/Interop.hNativeScript/runtime/Interop.mmNativeScript/runtime/InteropTypes.mmNativeScript/runtime/MetadataBuilder.mmNativeScript/runtime/NativeScriptException.hNativeScript/runtime/NativeScriptException.mmNativeScript/runtime/Runtime.hNativeScript/runtime/Runtime.mmNativeScript/runtime/WorkerWrapper.mmTestFixtures/TNSTestNativeCallbacks.hTestFixtures/TNSTestNativeCallbacks.mTestRunner/app/tests/ErrorEventsTests.jsTestRunner/app/tests/EscapeExceptionTests.jsTestRunner/app/tests/MethodCallsTests.jsTestRunner/app/tests/Promises.jsTestRunner/app/tests/index.js
…jectionhandled reason)
- GetEscapeExceptionBrand now checks Caches::IsValid() — Caches::Get
returns a dummy cache (never null) after isolate disposal, so the
nullptr check could not honor the documented empty-handle contract.
- The property setter's fallback ObjCDataWrapper is now released via
DeleteWrapperIfUnused, mirroring the getter (pre-existing asymmetry).
- rejectionhandled now carries the original rejection reason:
reportedOutstanding_/pendingRejectionHandled_ retain {promise, reason}
pairs (promise phantom-weak, so a GC'd promise still drops the entry
and its reason), per PromiseRejectionEvent semantics.
Collaborator
Author
|
Addressed all three review findings in 0d1e372 (full suite re-run: 900/900 green):
|
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
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.
What this does
Adopts the WHATWG error model at the runtime level so
@nativescript/coreno longer needs bespoke error hooks, closes the silently-swallowed-rejections hole, and adds an explicit escape hatch for forwarding real native exceptions to native callers.1. Unhandled promise rejection tracking
The runtime never registered
SetPromiseRejectCallback, so unhandled rejections vanished with no log, no callback, no crash. Now:kCFRunLoopBeforeWaitingobserver).__onUncaughtError/__onDiscardedError, honoringdiscardUncaughtJsExceptions), prefixedUnhandled promise rejection:.onerror→ main isolate'sworker.onerror).2. WHATWG error events on globalThis
Spec-shaped
Event,EventTarget,ErrorEvent,PromiseRejectionEventconstructors,addEventListener/removeEventListener/dispatchEventonglobalThis, and globalreportError()— same surface as workers/Deno, nowindowneeded.errorErrorEvent; unhandled rejections a cancelableunhandledrejectionPromiseRejectionEvent.preventDefault()= fully handled (no shim, no fatal log, no error modal).rejectionhandled(as a task, per spec, carrying the original reason; already-reported promises held phantom-weak so GC'd ones drop out).globalThis.dispatchEvent.__onUncaughtError/__onDiscardedErrorkeep working, so current core needs zero changes (they're now deprecated shims; a follow-up core PR can migrateapplication-common.tstoglobalThis.addEventListener).3.
interop.escapeException+ boundary hardeningNSExceptionaserror.nativeException(name/message from the exception) — re-enables the long-dormantThrow_ObjC_exceptions_to_JavaScripttest, now on simulator too.throw interop.escapeException(x)from a JS override/block converts to a real@throwinto the native caller at the boundary — a parent native@try/@catchsees the originalNSException(or one synthesized from the JS error, with the JS stack inuserInfo). Throws happen strictly after every V8 scope has destructed; ObjC exceptions never unwind through live V8 frames.tns::Assertprocess-aborts on JS throws in property getters/setters and the Array/Dictionary/FastEnumeration adapters are gone — those boundaries now report through the uncaught path and return a defined default.crashOnUncaughtJsExceptionsconfig (default off): unprevented fatal errors schedule an uncaught@throwon the runtime loop from a clean frame, so crash reporters capture a real native crash with the JS stack attached.Before / after examples
Unhandled rejections
Error events and reportError
Catching native exceptions in JS
Forwarding exceptions to a native caller (
interop.escapeException)Boundary hardening
Opt-in crash on fatal errors
When enabled, an error that no listener
preventDefault()s crashes the app with a realNSException(JS stack in the reason anduserInfo["JavaScriptStack"]), thrown from a clean frame on the runtime loop so crash reporters capture it properly.Behavior changes (default config)
preventDefault(), the new config flag) or identical.Testing
ApiTestsNSError shape (code/domain/nativeException),ExceptionHandlingTests, Promise thread-affinity,TNS Workers.TNSTestNativeCallbacks) exercise catch-in-native round trips.crashOnUncaughtJsExceptionsmanual smoke (documented inEscapeExceptionTests.js).