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
![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}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1d8f659. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| - Add `disableAutoUpload` option to Expo plugin to disable source map and debug symbol uploads ([#6195](https://github.com/getsentry/sentry-react-native/pull/6195)) | ||
| - Expose `pauseAppHangTracking` and `resumeAppHangTracking` APIs on iOS ([#6192](https://github.com/getsentry/sentry-react-native/pull/6192)) | ||
| - Better route and dynamic param extraction for Expo Router ([#6197](https://github.com/getsentry/sentry-react-native/pull/6197)) | ||
| - 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)) |
There was a problem hiding this comment.
This need to move in the unreleased section
| @@ -0,0 +1,135 @@ | |||
| import { logger } from '@sentry/react'; | |||
There was a problem hiding this comment.
I think we need to import from core like in other occurences
| import { logger } from '@sentry/react'; | |
| import { logger } from '@sentry/core'; |
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
left a comment
antonis
left a comment
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.
| spotlightIntegration, | ||
| stallTrackingIntegration, | ||
| timeToDisplayIntegration, | ||
| turboModuleContextIntegration, |
There was a problem hiding this comment.
popTurboModuleCall clears scope that still has active calls when scopes interleave
In popTurboModuleCall (turboModuleTracker.ts), after removing a frame, the code looks only at the new stack top: if its scope differs from the popped frame's scope it calls clearScope(popped.scope). It never checks whether deeper frames still hold that same scope. Whenever scopes interleave on the stack (e.g. [A@scope1, B@scope2] LIFO-popping B, or [A@scope1, B@scope1, C@scope2] out-of-order-popping B), an active lower call on popped.scope silently loses its turbo_module context/tags until that call itself finishes and re-syncs. This degrades the crash-time context the integration is designed to provide.
Evidence
popTurboModuleCallinpackages/core/src/js/turbomodule/turboModuleTracker.tslines ~140-160 checks onlystack[stack.length - 1]againstpopped.scope.- If the new top is on a different scope, it calls
clearScope(popped.scope)unconditionally viasetContext(CONTEXT_KEY, null)and resets the tag pair — even if a deeper frame instackstill holds that scope. pushTurboModuleCallpinsscope = args.scope ?? getCurrentScope(), so interleaving scopes on the stack is an explicitly supported case (the comment onInternalCall.scopecalls this out).popTurboModuleCallnever scansstackfor other frames withpopped.scope, and tests inturboModuleTracker.test.tsdo not cover multi-scope interleaving — only single-scope nesting.
Also found at 3 additional locations
packages/core/src/js/turbomodule/turboModuleTracker.ts:152-160packages/core/etc/sentry-react-native.api.md:357-359packages/core/etc/sentry-react-native.api.md:892
Identified by Warden code-review · AEP-X5X
| for (const key of methodNames) { | ||
| if (skip.has(key)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Unguarded getter access in wrapping loop can throw unexpectedly
Reading target[key] without a try/catch can invoke getter-defined properties that throw, causing wrapTurboModule to propagate an unexpected exception and abort wrapping of all remaining methods — the try/catch only guards the assignment on line 97, not this read.
Evidence
collectMethodNamesusesObject.getOwnPropertyNames, which includes getter-defined property names.- On line 60,
target[key]performs a plain property access — ifkeyrefers to a getter, it is invoked here. - The only try/catch in the loop (lines 96–101) wraps
target[key] = wrapper(the write), not this read. - An exception thrown by the getter would propagate out of
wrapTurboModule, breaking module initialization for all remaining keys. - No test covers a getter-bearing TurboModule; JSI HostObject proxies under the New Architecture can expose properties as accessors.
Suggested fix: Wrap the property read in a try/catch so a throwing getter is treated like a non-wrappable property.
| } | |
| let original: unknown; | |
| try { | |
| original = target[key]; | |
| } catch { | |
| // Getter threw — skip this property silently, same as a sealed write. | |
| continue; | |
| } |
Identified by Warden code-review · P95-YE7
2 participants
📢 Type of change
📜 Description
💡 Motivation and Context
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled🔮 Next steps