getsentry / getsentry/sentry-react-native

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

Abierto
#6,723 1 comentario 0 reacciones 1 asignado Reclamado por @antonis Ver en GitHub
Improvement React-Native
Lenguaje dominante
TypeScript
Estrellas
1.8k
Forks
366
Merge medio
1 d 6 h
PR fusionados (30 d)
94

Descripción

### 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.

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.