Skip to content

Commit 9689475

Browse files
committed
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.
1 parent 4201154 commit 9689475

4 files changed

Lines changed: 108 additions & 7 deletions

File tree

packages/core/src/js/turbomodule/turboModuleTracker.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,8 +99,16 @@ export function pushTurboModuleCall(args: {
9999
scope: args.scope ?? getCurrentScope(),
100100
};
101101

102+
// Atomic push: if `syncToScope` throws (e.g. a scope-sync hook calls into a
103+
// native bridge that rejects with `_NativeClientError`), roll back the stack
104+
// push so we don't leak a frame.
102105
stack.push(call);
103-
syncToScope(call);
106+
try {
107+
syncToScope(call);
108+
} catch (e) {
109+
stack.pop();
110+
throw e;
111+
}
104112
return call.callId;
105113
}
106114

packages/core/src/js/turbomodule/wrapTurboModule.ts

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -74,32 +74,42 @@ export function wrapTurboModule<T extends object>(
7474
const originalFn = original as (...a: unknown[]) => unknown;
7575

7676
const wrapper = function sentryTurboModuleWrapper(this: unknown, ...args: unknown[]): unknown {
77+
// The instrumentation must never break the user's call — every tracker
78+
// interaction is isolated so a failure inside Sentry only drops the
79+
// attribution data, never the real TurboModule invocation.
80+
//
7781
// We don't know yet whether `original` is sync or async — start optimistic
7882
// as sync, relabel to 'async' if the result turns out to be thenable.
79-
const callId = pushTurboModuleCall({ name, method: key, kind: 'sync' });
83+
let callId: number | undefined;
84+
try {
85+
callId = pushTurboModuleCall({ name, method: key, kind: 'sync' });
86+
} catch (e) {
87+
logger.warn(`[TurboModuleTracker] push failed for ${name}.${key}: ${String(e)}`);
88+
}
89+
8090
let result: unknown;
8191
try {
8292
result = originalFn.apply(this, args);
8393
} catch (e) {
84-
popTurboModuleCall(callId);
94+
safePop(callId, name, key);
8595
throw e;
8696
}
8797

8898
if (isThenable(result)) {
89-
relabelTurboModuleCallKind(callId, 'async');
99+
safeRelabel(callId, 'async', name, key);
90100
return (result as Promise<unknown>).then(
91101
value => {
92-
popTurboModuleCall(callId);
102+
safePop(callId, name, key);
93103
return value;
94104
},
95105
err => {
96-
popTurboModuleCall(callId);
106+
safePop(callId, name, key);
97107
throw err;
98108
},
99109
);
100110
}
101111

102-
popTurboModuleCall(callId);
112+
safePop(callId, name, key);
103113
return result;
104114
};
105115

@@ -153,6 +163,28 @@ function collectMethodNames(module: object): string[] {
153163
return Array.from(seen);
154164
}
155165

166+
function safePop(callId: number | undefined, name: string, method: string): void {
167+
if (callId === undefined) {
168+
return;
169+
}
170+
try {
171+
popTurboModuleCall(callId);
172+
} catch (e) {
173+
logger.warn(`[TurboModuleTracker] pop failed for ${name}.${method}: ${String(e)}`);
174+
}
175+
}
176+
177+
function safeRelabel(callId: number | undefined, kind: 'sync' | 'async', name: string, method: string): void {
178+
if (callId === undefined) {
179+
return;
180+
}
181+
try {
182+
relabelTurboModuleCallKind(callId, kind);
183+
} catch (e) {
184+
logger.warn(`[TurboModuleTracker] relabel failed for ${name}.${method}: ${String(e)}`);
185+
}
186+
}
187+
156188
function isThenable(value: unknown): value is PromiseLike<unknown> {
157189
if (!value || (typeof value !== 'object' && typeof value !== 'function')) {
158190
return false;

packages/core/test/turbomodule/turboModuleTracker.test.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,20 @@ describe('turboModuleTracker', () => {
219219
expect(relabelTurboModuleCallKind(9999, 'async')).toBe(false);
220220
});
221221

222+
it('rolls back the stack push if syncToScope throws (atomic push)', () => {
223+
const failingScope = new Scope();
224+
jest.spyOn(failingScope, 'setContext').mockImplementation(() => {
225+
throw new Error('native bridge boom');
226+
});
227+
228+
const before = getTurboModuleCallStack().length;
229+
expect(() =>
230+
pushTurboModuleCall({ name: 'X', method: 'y', kind: 'sync', scope: failingScope }),
231+
).toThrow('native bridge boom');
232+
// Stack is unchanged — the failed push didn't leak a frame.
233+
expect(getTurboModuleCallStack().length).toBe(before);
234+
});
235+
222236
it('assigns monotonically increasing call ids', () => {
223237
const a = pushTurboModuleCall({ name: 'M', method: 'a', kind: 'sync', scope });
224238
const b = pushTurboModuleCall({ name: 'M', method: 'b', kind: 'sync', scope });

packages/core/test/turbomodule/wrapTurboModule.test.ts

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -214,6 +214,53 @@ describe('wrapTurboModule', () => {
214214
);
215215
});
216216

217+
it('still calls the original method when the tracker push throws (native bridge error)', () => {
218+
const warnSpy = jest.spyOn(require('@sentry/core').logger, 'warn').mockImplementation(() => undefined);
219+
// Simulate a scope-sync hook that calls into a native bridge which throws.
220+
jest.spyOn(scope, 'setContext').mockImplementation(() => {
221+
throw new Error('NATIVE.setContext boom');
222+
});
223+
224+
const originalFn = jest.fn(() => 'real-result');
225+
const module = { doStuff: originalFn };
226+
227+
wrapTurboModule('Mod', module);
228+
229+
// The user's call must still complete with the original return value
230+
// — Sentry's instrumentation can never break the wrapped TurboModule.
231+
const result = module.doStuff();
232+
233+
expect(result).toBe('real-result');
234+
expect(originalFn).toHaveBeenCalledTimes(1);
235+
expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('push failed for Mod.doStuff'));
236+
// And the failed push left no leaked frame on the tracker.
237+
expect(getTurboModuleCallStack()).toEqual([]);
238+
});
239+
240+
it('still calls the original method when the tracker pop throws', () => {
241+
const warnSpy = jest.spyOn(require('@sentry/core').logger, 'warn').mockImplementation(() => undefined);
242+
243+
const originalFn = jest.fn(() => 42);
244+
const module = { doStuff: originalFn };
245+
246+
wrapTurboModule('Mod', module);
247+
248+
// Make `setContext` throw only on the pop's restore/clear path. We do this
249+
// by letting push succeed, then breaking setContext before the pop fires.
250+
let setContextCalls = 0;
251+
jest.spyOn(scope, 'setContext').mockImplementation(() => {
252+
setContextCalls++;
253+
if (setContextCalls > 1) {
254+
throw new Error('clear boom');
255+
}
256+
return scope;
257+
});
258+
259+
expect(() => module.doStuff()).not.toThrow();
260+
expect(originalFn).toHaveBeenCalled();
261+
expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('pop failed for Mod.doStuff'));
262+
});
263+
217264
it('does not leak a tracker frame when the result has a throwing `then` getter', () => {
218265
const trap = Object.defineProperty({}, 'then', {
219266
get() {

0 commit comments

Comments
 (0)