WIP: Turbo Modules crash time context#6227
Conversation
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
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Features
- Turbo Modules crash time context ([#6227](https://github.com/getsentry/sentry-react-native/pull/6227))If none of the above apply, you can opt out of this check by adding |
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
| const callId = pushTurboModuleCall({ name, method: key, kind: 'sync' }); | ||
| let result: unknown; | ||
| try { | ||
| result = originalFn.apply(this, args); | ||
| } catch (e) { | ||
| popTurboModuleCall(callId); | ||
| throw e; | ||
| } | ||
|
|
||
| if (isThenable(result)) { | ||
| // Re-record as async — clearer in the report. We just overwrite the | ||
| // existing tracker frame in place by popping + re-pushing with a fresh | ||
| // id would lose ordering, so instead we leave the stack frame alone | ||
| // and only relabel for the scope on completion (it's the *active* | ||
| // call's `kind` that ends up in `contexts.turbo_module`, and the | ||
| // outer perf-logger driven users can push with `kind: 'async'` | ||
| // directly when they know up front). | ||
| return (result as Promise<unknown>).then( |
There was a problem hiding this comment.
Async TurboModule calls always recorded as kind: 'sync' in crash context
Every method call pushes with kind: 'sync' (line 58), and when the result is thenable there is no mutation or re-push to change the stack frame's kind to 'async' — so async calls appear as kind: 'sync' in contexts.turbo_module for the entire duration of the async operation.
Evidence
pushTurboModuleCallat line 58 is always called withkind: 'sync'.syncToScopeis invoked insidepushTurboModuleCallimmediately, writingkind: 'sync'intocontexts.turbo_module.- The
isThenablebranch (line 65) only wraps.then/.catchto callpopTurboModuleCall; it never callspushTurboModuleCallagain withkind: 'async'or mutates the existing stack frame. popTurboModuleCallinturboModuleTracker.tsonly removes the frame and updates the scope to the new top — it does not relabel the kind.- The inline comment at lines 67-77 describes a relabeling that never materialises in the implementation.
Also found at 1 additional location
packages/core/etc/sentry-react-native.api.md:734
Identified by Warden code-review · EUK-DJP
| return result; | ||
| }; | ||
| } | ||
|
|
||
| try { | ||
| Object.defineProperty(module, WRAPPED_FLAG, { | ||
| value: true, | ||
| enumerable: false, | ||
| configurable: false, | ||
| writable: false, | ||
| }); | ||
| } catch (e) { |
There was a problem hiding this comment.
Sealed TurboModule proxies can be double-wrapped, causing duplicate tracker pushes per call
When Object.defineProperty throws for a sealed proxy, WRAPPED_FLAG is never set, so the guard at line 39 (if (maybeWrapped[WRAPPED_FLAG])) always passes on subsequent calls to wrapTurboModule, re-wrapping already-wrapped methods and pushing the same call twice onto the tracker stack.
Evidence
- The guard
if (maybeWrapped[WRAPPED_FLAG])at line 39 relies solely onObject.definePropertyhaving succeeded. - The
catchblock (lines 93-99) only logs a warning and does not apply any fallback to prevent future wrapping. - A second call to
wrapTurboModuleon the same sealed proxy will iterate all keys again and wrap the already-wrapped function references, so each real call pushescallIdtwice. - The inline comment in the catch block explicitly acknowledges "the per-call pushes would double up" without mitigating it.
- No test covers the sealed-proxy re-wrap scenario.
Also found at 1 additional location
packages/core/etc/sentry-react-native.api.md:799
Identified by Warden code-review · AF3-FXS
| const fakeModule = { | ||
| addListener: jest.fn(), | ||
| removeListeners: jest.fn(), | ||
| crash: jest.fn(), | ||
| }; |
There was a problem hiding this comment.
Object.keys in wrapTurboModule silently skips prototype methods on JSI TurboModule proxies
wrapTurboModule uses Object.keys(target) which only enumerates own enumerable properties; if a real JSI-backed TurboModule proxy exposes methods via its prototype chain (as some RN HostObject implementations do), nothing gets wrapped and the integration silently provides no crash context.
Evidence
wrapTurboModule.ts:45iteratesObject.keys(target)– only own, enumerable string keys.getRNSentryModule()inwrapper.ts:45returnsTurboModuleRegistry.get('RNSentry'), which is a JSI HostObject proxy in RN's TurboModule architecture; JSI HostObjects can surface methods through the prototype chain rather than as own properties.- The test at lines 21-25 creates a plain JS object literal (
{ addListener, removeListeners, crash }) where all methods are own enumerable properties, soObject.keysfinds them and the test passes regardless of how real proxies work. wrapTurboModule.test.tsalso exclusively uses plain object literals, leaving the prototype-method path untested.- If real module methods are not own properties,
Object.keysreturns nothing to wrap; no warning or error is emitted, so the failure is silent.
Also found at 1 additional location
packages/core/src/js/integrations/default.ts:45-45
Identified by Warden find-bugs · B3Z-ZZF
1 participant
📢 Type of change
📜 Description
💡 Motivation and Context
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled🔮 Next steps