Skip to content

Commit 9474ead

Browse files
alwxalwx
authored
Turbo Modules crash time context (#6227)
* Turbo Modules crash time context * fixes * fix(turbomodule): address review feedback 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`. * fix(turbomodule): address remaining PR review feedback - 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. * fix(turbomodule): prevent scope-sync recursion and harden wrapper edges 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. * fix(turbomodule): keep native context in sync after cross-scope pop 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). * fix(turbomodule): never let instrumentation break user TurboModule calls 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. * Lint fixes * chore(api-report): regenerate with workspace TypeScript 5.9.3 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.
1 parent 7a89652 commit 9474ead

12 files changed

Lines changed: 1244 additions & 0 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
- Instrument Expo Router `push`, `replace`, `navigate`, `back`, and `dismiss` (in addition to `prefetch`) with breadcrumbs and spans, and tag the resulting idle navigation span with the initiating `navigation.method` ([#6221](https://github.com/getsentry/sentry-react-native/pull/6221))
1616
- Note: Expo Router span/breadcrumb attributes that may contain user identifiers (`route.href`, `route.params`, and concrete pathnames derived from string hrefs such as `/users/42`) are now gated behind `sendDefaultPii`. When `sendDefaultPii` is off (the default), prefetch spans for string hrefs use `route.name: 'unknown'` and omit `route.href`. Templated object hrefs (e.g. `{ pathname: '/users/[id]' }`) are unaffected.
1717
- Warn when Gradle resolves `sentry-android` to a version incompatible with the SDK ([#6238](https://github.com/getsentry/sentry-react-native/pull/6238))
18+
- Attach the active TurboModule method to native crash reports as `contexts.turbo_module` + `turbo_module.name` / `turbo_module.method` tags ([#6227](https://github.com/getsentry/sentry-react-native/pull/6227))
1819

1920
### Fixes
2021

‎packages/core/etc/sentry-react-native.api.md‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -354,6 +354,9 @@ export { functionToStringIntegration }
354354

355355
export { getActiveSpan }
356356

357+
// @public
358+
export function getActiveTurboModuleCall(): TurboModuleCall | undefined;
359+
357360
export { getClient }
358361

359362
// Warning: (ae-forgotten-export) The symbol "ReactNativeTracingIntegration" needs to be exported by the entry point index.d.ts
@@ -378,6 +381,9 @@ export function getReactNativeTracingIntegration(client: Client): ReactNativeTra
378381

379382
export { getRootSpan }
380383

384+
// @public
385+
export function getTurboModuleCallStack(): TurboModuleCall[];
386+
381387
// Warning: (ae-forgotten-export) The symbol "GlobalErrorBoundaryState" needs to be exported by the entry point index.d.ts
382388
//
383389
// @public
@@ -530,11 +536,22 @@ export { OpenAiOptions }
530536
// @public
531537
export function pauseAppHangTracking(): void;
532538

539+
// @public
540+
export function popTurboModuleCall(callId: number): void;
541+
533542
// @public
534543
export const primitiveTagIntegration: () => Integration;
535544

536545
export { Profiler }
537546

547+
// @public
548+
export function pushTurboModuleCall(args: {
549+
name: string;
550+
method: string;
551+
kind: 'sync' | 'async';
552+
scope?: Scope;
553+
}): number;
554+
538555
// Warning: (ae-forgotten-export) The symbol "ReactNativeClientOptions" needs to be exported by the entry point index.d.ts
539556
//
540557
// @public
@@ -808,6 +825,27 @@ export class TouchEventBoundary extends React_2.Component<TouchEventBoundaryProp
808825

809826
export { TransactionEvent }
810827

828+
// @public
829+
export interface TurboModuleCall {
830+
callId: number;
831+
kind: 'sync' | 'async';
832+
method: string;
833+
name: string;
834+
startedAtMs: number;
835+
}
836+
837+
// @public
838+
export const turboModuleContextIntegration: (options?: TurboModuleContextOptions) => Integration;
839+
840+
// @public (undocumented)
841+
export interface TurboModuleContextOptions {
842+
modules?: Array<{
843+
name: string;
844+
module: object | null | undefined;
845+
skipMethods?: ReadonlyArray<string>;
846+
}>;
847+
}
848+
811849
// @public (undocumented)
812850
export const Unmask: HostComponent<ViewProps> | React_2.ComponentType<ViewProps>;
813851

@@ -852,6 +890,11 @@ export function wrapExpoImage<T extends ExpoImage>(imageClass: T): T;
852890
// @public
853891
export function wrapExpoRouter<T extends ExpoRouter>(router: T): T;
854892

893+
// @public
894+
export function wrapTurboModule<T extends object>(name: string, module: T | null | undefined, options?: {
895+
skip?: ReadonlyArray<string>;
896+
}): T | null | undefined;
897+
855898
// Warnings were encountered during analysis:
856899
//
857900
// src/js/feedback/integration.ts:21:5 - (ae-forgotten-export) The symbol "ScreenshotButtonProps" needs to be exported by the entry point index.d.ts

‎packages/core/src/js/index.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,3 +157,12 @@ export { FeedbackForm as FeedbackWidget } from './feedback/FeedbackForm';
157157
export { showFeedbackForm as showFeedbackWidget } from './feedback/FeedbackFormManager';
158158

159159
export { getDataFromUri } from './wrapper';
160+
161+
export {
162+
getActiveTurboModuleCall,
163+
getTurboModuleCallStack,
164+
popTurboModuleCall,
165+
pushTurboModuleCall,
166+
wrapTurboModule,
167+
} from './turbomodule';
168+
export type { TurboModuleCall } from './turbomodule';

‎packages/core/src/js/integrations/default.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ import {
4141
spotlightIntegration,
4242
stallTrackingIntegration,
4343
timeToDisplayIntegration,
44+
turboModuleContextIntegration,
4445
userInteractionIntegration,
4546
viewHierarchyIntegration,
4647
} from './exports';
@@ -174,5 +175,10 @@ export function getDefaultIntegrations(options: ReactNativeClientOptions): Integ
174175

175176
integrations.push(primitiveTagIntegration());
176177

178+
if (options.enableNative) {
179+
// Attribute native crashes to the active TurboModule method (see #6163).
180+
integrations.push(turboModuleContextIntegration());
181+
}
182+
177183
return integrations;
178184
}

‎packages/core/src/js/integrations/exports.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ export { appRegistryIntegration } from './appRegistry';
2626
export { timeToDisplayIntegration } from '../tracing/integrations/timeToDisplayIntegration';
2727
export { breadcrumbsIntegration } from './breadcrumbs';
2828
export { primitiveTagIntegration } from './primitiveTagIntegration';
29+
export { turboModuleContextIntegration } from './turboModuleContext';
30+
export type { TurboModuleContextOptions } from './turboModuleContext';
2931
export { logEnricherIntegration } from './logEnricherIntegration';
3032
export { graphqlIntegration } from './graphql';
3133
export { supabaseIntegration } from './supabase';

‎packages/core/src/js/integrations/turboModuleContext.ts‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
import type { Integration } from '@sentry/core';
2+
3+
import { wrapTurboModule } from '../turbomodule';
4+
import { getRNSentryModule } from '../wrapper';
5+
6+
export const INTEGRATION_NAME = 'TurboModuleContext';
7+
8+
export interface TurboModuleContextOptions {
9+
/**
10+
* Additional TurboModules to track. Each entry's methods will be wrapped so
11+
* that any native crash happening inside a method call gets `contexts.turbo_module`
12+
* + `turbo_module.name` / `turbo_module.method` attached to the crash report.
13+
*
14+
* The built-in `RNSentry` TurboModule is always tracked.
15+
*/
16+
modules?: Array<{ name: string; module: object | null | undefined; skipMethods?: ReadonlyArray<string> }>;
17+
}
18+
19+
// Methods on RNSentry that must NOT be tracked:
20+
//
21+
// - `addListener` / `removeListeners` are RN event-emitter stubs that fire on
22+
// every subscriber registration — tracking them would just churn the scope.
23+
//
24+
// - The scope-sync methods (`setContext`, `setTag`, `setExtra`, `setUser`,
25+
// `addBreadcrumb`, `clearBreadcrumbs`, `setAttribute`, `setAttributes`,
26+
// `removeAttribute`) are called by our own `enableSyncToNative` hook every
27+
// time anything writes to a JS Scope. Tracking them would cause infinite
28+
// recursion: `pushTurboModuleCall` -> `scope.setContext` -> `NATIVE.setContext`
29+
// -> `RNSentry.setContext` (wrapped) -> `pushTurboModuleCall` -> ... .
30+
const RNSENTRY_SKIP = [
31+
'addListener',
32+
'removeListeners',
33+
'setContext',
34+
'setTag',
35+
'setExtra',
36+
'setUser',
37+
'addBreadcrumb',
38+
'clearBreadcrumbs',
39+
'setAttribute',
40+
'setAttributes',
41+
'removeAttribute',
42+
] as const;
43+
44+
/**
45+
* Attaches the currently-executing TurboModule method to the Sentry scope so
46+
* that native crashes can be attributed to the high-level RN module + method
47+
* (e.g. `RNSentry.captureEnvelope`) on top of the native stack trace.
48+
*
49+
* The active call is mirrored as `contexts.turbo_module` and the
50+
* `turbo_module.name` / `turbo_module.method` tags, both of which are already
51+
* synced to the native SDKs by the existing scope-sync hooks and therefore end
52+
* up in crash reports captured by sentry-cocoa / sentry-java.
53+
*
54+
* See https://github.com/getsentry/sentry-react-native/issues/6163.
55+
*/
56+
export const turboModuleContextIntegration = (options: TurboModuleContextOptions = {}): Integration => {
57+
return {
58+
name: INTEGRATION_NAME,
59+
setupOnce() {
60+
// Wrap the live RNSentry TurboModule. Other integrations import the same
61+
// instance by reference, so wrapping here transparently tracks every call
62+
// made from JS — including the SDK's own internal envelope/scope sync
63+
// calls, which are the most likely entry points for native crashes.
64+
wrapTurboModule('RNSentry', getRNSentryModule(), { skip: RNSENTRY_SKIP });
65+
66+
for (const entry of options.modules ?? []) {
67+
wrapTurboModule(entry.name, entry.module, { skip: entry.skipMethods });
68+
}
69+
},
70+
};
71+
};

‎packages/core/src/js/turbomodule/index.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
export {
2+
getActiveTurboModuleCall,
3+
getTurboModuleCallStack,
4+
popTurboModuleCall,
5+
pushTurboModuleCall,
6+
} from './turboModuleTracker';
7+
export type { TurboModuleCall } from './turboModuleTracker';
8+
export { wrapTurboModule } from './wrapTurboModule';

0 commit comments

Comments
 (0)