getsentry / getsentry/sentry-react-native

Make getNewScreenTimeToDisplay race-free by moving it to the native store/pull model

Open
#6,723 1 comment 0 reactions 1 assignee Claimed by @antonis View on GitHub
Improvement React-Native
Dominant language
TypeScript
Stars
1.8k
Forks
366
Avg merge
1d 9h
Merged PRs (30d)
89

Description

### Background

#6722 fixes the fatal `JavaCallback was already settled` crash from `getNewScreenTimeToDisplay` under rapid navigation on Android (#6709). That fix makes losing the race **survivable** (crash → dropped measurement) but does not close the race window itself.

### Why the window is intrinsic to the current design

`RNSentryTimeToDisplay.getTimeToDisplay` schedules a deferred `Choreographer` frame callback that resolves a live `Promise` *after* the frame renders. The producer (native frame callback) and consumer (JS promise / backing `JavaCallback`) have independent lifetimes:

- Native gets **no teardown signal** when a navigation is superseded, so it can't cancel the pending frame callback.
- `com.facebook.react.bridge.Promise` exposes **no validity query**, and even a check would be TOCTOU.
- The deferred settle **is** the measurement (time to first frame after navigation), so we can't resolve early to dodge the race.

The only atomic option at that layer is attempt-and-catch — which is what #6722 does.

### Proposed race-free design

Move `getNewScreenTimeToDisplay` to the **store/pull model that the other TTID paths already use**: the frame callback writes the timestamp into the native `screenIdToRenderDuration` map (via `putTimeToDisplayFor`), and JS **pops** it later via `popTimeToDisplayFor` — same pattern as `ttid-`, `ttfd-`, and `ttid-navigation-` (see `timeToDisplayIntegration.ts`). A stored value has no long-lived bridge callback, so there is no "already settled" failure mode at all.

### Scope / considerations

- Bridge spec change (`NativeRNSentry.ts`) → **breaking**, needs a `@deprecated` migration path.
- Must be implemented on **both** platforms (Android + iOS — the iOS variant of the settle-after-teardown risk was deferred in #6722).
- Changes the fallback contract in `sentryeventemitterfallback.ts`, which currently races the native promise against `requestAnimationFrame`; a pull model changes that timing and needs care.

### Acceptance criteria

- [ ] `getNewScreenTimeToDisplay`'s deferred measurement no longer relies on resolving a long-lived promise from a frame callback.
- [ ] No behavioral regression in TTID measurements on either architecture (New + Old) or platform (iOS + Android).
- [ ] Deprecation path for the bridge API change.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.