WIP: Turbo Modules crash time context #6227
4 issues
code-review: Found 4 issues (2 medium, 2 low)
Medium
popTurboModuleCall clears scope that still has active calls when scopes interleave - `packages/core/src/js/integrations/default.ts:44`
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.
Also found at:
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
Unguarded getter access in wrapping loop can throw unexpectedly - `packages/core/src/js/turbomodule/wrapTurboModule.ts:60`
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.
Low
Low-level TurboModule push/pop helpers exposed in public package API - `packages/core/src/js/index.ts:160`
packages/core/src/js/turbomodule/index.ts re-exports pushTurboModuleCall and popTurboModuleCall, and packages/core/src/js/index.ts (lines 161–167) makes them part of the package's public surface. These are low-level helpers that mutate a module-level singleton stack (const stack: InternalCall[] in turboModuleTracker.ts) and rely on perfectly balanced LIFO usage to keep contexts.turbo_module accurate. The intended consumer entry point is wrapTurboModule, which already pairs push/pop with try/catch and promise settlement handling. Consider keeping push/pop internal (or prefixed _) so the public API is just wrapTurboModule + the read-only getActiveTurboModuleCall / getTurboModuleCallStack; this avoids committing to a low-level contract that is easy to misuse (unbalanced push leaves stale turbo_module context/tags on the scope until overwritten) and hard to evolve without a breaking change.
Scope and `getCurrentScope` mock set up but never asserted on - `packages/core/test/integrations/turboModuleContext.test.ts:9-14`
The scope object and getCurrentScope spy are created in beforeEach but no test asserts on scope state — consider either removing the dead setup or adding assertions that verify contexts.turbo_module is populated when wrapped methods are invoked.
⏱ 10m 18s · 1.3M in / 107.3k out · $3.97
Annotations
Check warning on line 44 in packages/core/src/js/integrations/default.ts
sentry-warden / warden: code-review
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.
Check warning on line 160 in packages/core/src/js/turbomodule/turboModuleTracker.ts
sentry-warden / warden: code-review
[AEP-X5X] popTurboModuleCall clears scope that still has active calls when scopes interleave (additional location)
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.
Check warning on line 359 in packages/core/etc/sentry-react-native.api.md
sentry-warden / warden: code-review
[AEP-X5X] popTurboModuleCall clears scope that still has active calls when scopes interleave (additional location)
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.
Check warning on line 892 in packages/core/etc/sentry-react-native.api.md
sentry-warden / warden: code-review
[AEP-X5X] popTurboModuleCall clears scope that still has active calls when scopes interleave (additional location)
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.
Check warning on line 60 in packages/core/src/js/turbomodule/wrapTurboModule.ts
sentry-warden / warden: code-review
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.