Turbo Modules crash time context#6227
Open
alwx wants to merge 12 commits into
Open
Conversation
Contributor
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog.
🤖 This preview updates automatically when you update the PR. |
alwx
changed the title
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
antonis
reviewed
Jun 3, 2026
antonis
reviewed
Jun 3, 2026
Addresses five review comments on the TurboModule crash-time context PR (#6227): - Pin the Scope on each tracker frame at push time. `popTurboModuleCall` and `relabelTurboModuleCallKind` now always operate against the scope captured at push, so an async call that spans a scope switch (`withScope`, isolation-scope swaps) clears the right scope instead of leaking stale `turbo_module` context onto the original. `pop` no longer takes a `scope` argument — it was a footgun. - Replace `setTag(key, undefined)` on clear with an empty-string sentinel. The native `setTag(key: string, value: string)` TurboModule spec requires a string; `undefined` would either be rejected or silently dropped at the bridge. `setContext(key, null)` remains the canonical "no active call" signal; the empty tags exist only to scrub stale `name`/`method` from the previous call. - Defer `wrappedModules.add(module)` until after at least one method has been successfully wrapped. Previously, a JSI HostObject whose methods materialise lazily was permanently locked out after the first (empty) discovery pass — subsequent calls short-circuited even though the module had since gained methods. - Strengthen the sealed-module test to spy on `pushTurboModuleCall` and assert exactly one push per real call. The previous assertion only checked the post-call stack was empty, which is also true under double-wrapping (each wrapper pushes and pops symmetrically). - Move the changelog entry from the already-released `## 8.13.0` section into `## Unreleased`.
antonis
reviewed
Jun 3, 2026
antonis
left a comment
antonis
left a comment
Contributor
There was a problem hiding this comment.
Thank you for your work on this Alex 🙇
Overall looks good. Left a few comments along with the agent feedback.
Let's also update the PR description and title for future reference.
alwx
changed the title
- Switch logger import to '@sentry/core' to match the convention used by the rest of the codebase (per @antonis review). - Guard the property read in wrapTurboModule's wrap loop. Some JSI HostObject proxies under the New Architecture expose methods as accessors, and a throwing getter would otherwise propagate out and abort wrapping of all remaining methods on the module. A throwing getter is now treated like a non-wrappable property and skipped. - Fix popTurboModuleCall scope handling when scopes interleave on the stack. Previously we only inspected the new top frame, so popping a call from a stack like [A@s1, B@s2, C@s1] would clear s1's turbo_module context even though A is still active on it. We now walk the stack for the newest remaining frame on popped.scope and re-sync onto it, only clearing the scope when no frames remain. Adds tests for the throwing-getter path and for both the preserve-on-interleave and clear-when-empty popTurboModuleCall cases.
Contributor
There was a problem hiding this comment.
turboModuleContext writes to getCurrentScope() which is not native-synced, so native crashes may not get turbo_module context
The integration's primary goal is attributing native crashes to TurboModule calls, but syncToScope writes to getCurrentScope() while enableSyncToNative (which forwards setContext/setTag to the native SDK) is only applied to getGlobalScope() and getIsolationScope(). If getCurrentScope() is not one of those natively-synced scopes, the contexts.turbo_module data stays JS-only and won't appear in sentry-cocoa / sentry-java crash reports. Consider writing to getIsolationScope() (which is always native-synced) or verifying that getCurrentScope() === getIsolationScope() holds in all targeted RN environments.
Evidence
sdk.tsx:77-78:enableSyncToNativeis explicitly called only ongetGlobalScope()andgetIsolationScope().turboModuleTracker.ts:99:pushTurboModuleCallresolvescall.scopeasargs.scope ?? getCurrentScope(), and the wrapper inwrapTurboModule.tsnever passes a scope, so it always falls back togetCurrentScope().scopeSync.ts:30-31, 80-81: only scopes that have been throughenableSyncToNativeforwardsetTag/setContexttoNATIVE.setTag/NATIVE.setContext; untouched scopes only update in-memory data.wrapper.ts:475-485, 562-584:NATIVE.setTagandNATIVE.setContextare the only bridge to the native SDK scope state read by sentry-cocoa/sentry-java at crash time.turboModuleTracker.tscomment acknowledges reliance on "existing scope mirroring" but doesn't guard againstgetCurrentScope()being an un-mirrored scope.
Identified by Warden code-review
Addresses three review comments on #6227: - Skip all scope-sync methods on RNSentry (`setContext`, `setTag`, `setExtra`, `setUser`, `addBreadcrumb`, `clearBreadcrumbs`, `setAttribute`, `setAttributes`, `removeAttribute`) in addition to `addListener` / `removeListeners`. Without this the tracker recurses infinitely on every TurboModule call: `pushTurboModuleCall` -> `scope.setContext('turbo_module', ...)` -> `NATIVE.setContext` -> `RNSentry.setContext` (wrapped) -> `pushTurboModuleCall` -> ... . - Wrap the `.then` read in `isThenable` in try/catch. A user-defined throwing `then` getter on a method's return value would otherwise escape the wrapper and leak the in-flight tracker frame. - Warn when methods are discovered but none could be wrapped (e.g. `Object.freeze`d legacy `NativeModules.RNSentry` on the old architecture, or read-only accessors). Previously this was a silent no-op with no obvious cause for the missing crash context. Tests added: scope-sync methods are not wrapped on RNSentry, throwing `then` getter doesn't leak a frame, frozen module triggers the warning.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c8d960b. Configure here.
antonis
reviewed
Jun 8, 2026
antonis
left a comment
antonis
left a comment
Contributor
There was a problem hiding this comment.
Thank you for iterating on the feedback Alex 🙇
Apart from the Lint/Api checks LGTM.
Let's also update the empty PR description and title for future reference.
Addresses two unresolved review comments on #6227: - Native SDKs (sentry-cocoa, sentry-java) share a single scope, but JS has many Scope objects (global, isolation, withScope, ...). When a cross-scope pop runs `clearScope` on the popped scope, the scope-sync hook overwrites the native `turbo_module` context with null \u2014 even if the global stack top still lives on a different JS scope with an in-flight wrapped call. A crash following such a pop would land in native without the active TurboModule attribution. Fix: after updating the popped scope, re-sync the global stack top via its own scope so the native scope ends up holding the correct active call context. Dedup the write when the popped scope's new top *is* the global top. - Strengthen the `crash`-is-wrapped assertion in the recursion-guard test. `expect(fakeModule.crash).not.toBe(jest.fn())` was trivially true (every `jest.fn()` call returns a new reference), so it would have passed even if the integration didn't wrap `crash` at all. Capture the original mock before `setupOnce` and check the wrapper replaced it (and lacks the `_isMockFunction` marker).
Addresses the unresolved Warden finding on #6227. Previously, the wrapper called `pushTurboModuleCall` *outside* its `try` block. `pushTurboModuleCall` synchronously writes to the pinned scope, and `scopeSync.ts`'s hooked `setContext` calls into `NATIVE.setContext`, which throws `_NativeClientError` when the RNSentry module isn't loaded (and any other native bridge error along that path). A throw there would abort the wrapper before `originalFn.apply` was reached \u2014 silently dropping the user's real TurboModule call (including critical ones like `captureEnvelope`) and surfacing an unexpected error to the caller. It also left the just- pushed frame on the tracker stack with no matching pop, leaking the `turbo_module` context. Two-layer fix: - `pushTurboModuleCall` is now atomic. If `syncToScope` throws, the stack push is rolled back so the tracker stack stays consistent. - The wrapper isolates every tracker interaction (push, pop, relabel) in its own try/catch with a `logger.warn` on failure. The user's call is always executed and its result/exception is always returned to the caller. The worst case is loss of the `turbo_module` attribution for that call \u2014 never a broken user invocation. Tests added: atomic-push rollback on `syncToScope` throw; wrapper still calls the original method (and returns its result) when push or pop throws.
The committed API report had drifted from the format the workspace's pinned TypeScript 5.9.3 produces \u2014 earlier regenerations had resolved `tsc` via the system PATH (TS 4.9.5), which emits `number | undefined` where 5.9.3 emits `number`, single-quoted vs double-quoted string literals, and a different inlined form for `startIdleNavigationSpan`'s options. CI's `api-report:check` (which always uses the workspace's TS) caught the discrepancy. Regenerate against the correct TS version so CI passes. The actual public API surface is unchanged \u2014 only the textual representation in the report file.
Contributor
iOS (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 882f8ae+dirty | 3840.30 ms | 1224.41 ms | -2615.88 ms |
| f3215d3+dirty | 3842.73 ms | 1219.33 ms | -2623.40 ms |
| df5d108+dirty | 1225.90 ms | 1220.14 ms | -5.76 ms |
| 44c8b3f+dirty | 3823.85 ms | 1207.66 ms | -2616.19 ms |
| 5257d80+dirty | 3854.39 ms | 1234.28 ms | -2620.11 ms |
| 6176a94+dirty | 3836.50 ms | 1217.64 ms | -2618.86 ms |
| 5748023+dirty | 3840.49 ms | 1227.43 ms | -2613.05 ms |
| c823bb5+dirty | 3843.44 ms | 1225.70 ms | -2617.74 ms |
| b0d3373+dirty | 3831.75 ms | 1227.29 ms | -2604.46 ms |
| d038a14+dirty | 3845.71 ms | 1228.11 ms | -2617.59 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 882f8ae+dirty | 5.15 MiB | 6.70 MiB | 1.54 MiB |
| f3215d3+dirty | 5.15 MiB | 6.67 MiB | 1.52 MiB |
| df5d108+dirty | 3.38 MiB | 4.73 MiB | 1.35 MiB |
| 44c8b3f+dirty | 5.15 MiB | 6.66 MiB | 1.51 MiB |
| 5257d80+dirty | 5.15 MiB | 6.69 MiB | 1.54 MiB |
| 6176a94+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| 5748023+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| c823bb5+dirty | 5.15 MiB | 6.69 MiB | 1.53 MiB |
| b0d3373+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| d038a14+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
📲 Install BuildsAndroid
|
Contributor
Android (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| c151573+dirty | 530.34 ms | 559.43 ms | 29.09 ms |
| 8929511+dirty | 405.33 ms | 452.16 ms | 46.83 ms |
| 5c1e987+dirty | 423.52 ms | 471.64 ms | 48.12 ms |
| 7a89652+dirty | 537.76 ms | 567.84 ms | 30.08 ms |
| 6177334+dirty | 408.16 ms | 441.14 ms | 32.98 ms |
| 41d6254+dirty | 424.45 ms | 474.34 ms | 49.89 ms |
| 5125c43+dirty | 497.18 ms | 543.78 ms | 46.60 ms |
| 853723c+dirty | 405.54 ms | 440.08 ms | 34.54 ms |
| 3ce5254+dirty | 410.57 ms | 448.48 ms | 37.91 ms |
| b0d3373+dirty | 557.66 ms | 579.42 ms | 21.76 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| c151573+dirty | 48.30 MiB | 53.54 MiB | 5.24 MiB |
| 8929511+dirty | 43.75 MiB | 48.16 MiB | 4.41 MiB |
| 5c1e987+dirty | 43.75 MiB | 48.08 MiB | 4.33 MiB |
| 7a89652+dirty | 48.30 MiB | 53.60 MiB | 5.30 MiB |
| 6177334+dirty | 48.30 MiB | 53.54 MiB | 5.23 MiB |
| 41d6254+dirty | 48.30 MiB | 53.60 MiB | 5.30 MiB |
| 5125c43+dirty | 48.30 MiB | 53.54 MiB | 5.24 MiB |
| 853723c+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
| 3ce5254+dirty | 43.75 MiB | 48.12 MiB | 4.37 MiB |
| b0d3373+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
Contributor
iOS (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 882f8ae+dirty | 3842.51 ms | 1230.40 ms | -2612.11 ms |
| f3215d3+dirty | 3846.08 ms | 1231.85 ms | -2614.23 ms |
| df5d108+dirty | 1207.34 ms | 1210.50 ms | 3.16 ms |
| 44c8b3f+dirty | 3849.24 ms | 1209.94 ms | -2639.31 ms |
| 5257d80+dirty | 3845.40 ms | 1226.21 ms | -2619.19 ms |
| 6176a94+dirty | 3854.15 ms | 1221.77 ms | -2632.38 ms |
| 5748023+dirty | 3844.74 ms | 1225.49 ms | -2619.26 ms |
| c823bb5+dirty | 3845.16 ms | 1210.33 ms | -2634.83 ms |
| b0d3373+dirty | 3842.49 ms | 1218.49 ms | -2624.00 ms |
| d038a14+dirty | 3831.11 ms | 1216.30 ms | -2614.81 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 882f8ae+dirty | 5.15 MiB | 6.70 MiB | 1.54 MiB |
| f3215d3+dirty | 5.15 MiB | 6.67 MiB | 1.52 MiB |
| df5d108+dirty | 3.38 MiB | 4.73 MiB | 1.35 MiB |
| 44c8b3f+dirty | 5.15 MiB | 6.66 MiB | 1.51 MiB |
| 5257d80+dirty | 5.15 MiB | 6.69 MiB | 1.54 MiB |
| 6176a94+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| 5748023+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| c823bb5+dirty | 5.15 MiB | 6.69 MiB | 1.53 MiB |
| b0d3373+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| d038a14+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
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
2 participants
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.
📢 Type of change
📜 Description
Fixes #6163
📝 Checklist
sendDefaultPIIis enabled🔮 Next steps