fix(android): Prevent fatal 'JavaCallback was already settled' crash from getNewScreenTimeToDisplay - #6722
Open
antonis wants to merge 4 commits into
Open
fix(android): Prevent fatal 'JavaCallback was already settled' crash from getNewScreenTimeToDisplay#6722antonis wants to merge 4 commits into
antonis wants to merge 4 commits into
Conversation
…from getNewScreenTimeToDisplay `RNSentryTimeToDisplay.getTimeToDisplay` settles the promise from a deferred Choreographer frame callback. Under rapid navigation the bridge callback backing the promise can already have been settled/invalidated by the runtime by the time the frame callback runs (on Expo bridgeless this surfaces as "JavaCallback was already settled"). Because the settle happened inside the frame callback, the throw was uncaught and reached the host app as a fatal crash. Route every settle through guarded helpers that settle at most once and swallow any settle failure (logging at WARNING), so a late/duplicate settle drops the time-to-display measurement instead of crashing the app. Shared impl, so this covers both the old and new architecture. Fixes #6709 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
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. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?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.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d16b9da. Configure here.
📲 Install BuildsAndroid
|
Contributor
Android (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| f3215d3+dirty | 396.53 ms | 436.66 ms | 40.13 ms |
| d0e3b3e+dirty | 443.19 ms | 480.00 ms | 36.81 ms |
| a5d243c+dirty | 499.20 ms | 525.62 ms | 26.42 ms |
| 7fd0012+dirty | 422.41 ms | 445.57 ms | 23.17 ms |
| 0a147b2+dirty | 442.80 ms | 522.24 ms | 79.44 ms |
| d7d54c6+dirty | 423.54 ms | 498.56 ms | 75.02 ms |
| c823bb5+dirty | 468.26 ms | 516.16 ms | 47.90 ms |
| 5789645+dirty | 502.66 ms | 594.19 ms | 91.53 ms |
| 40c9884+dirty | 453.76 ms | 478.98 ms | 25.22 ms |
| d771cd5+dirty | 432.29 ms | 464.38 ms | 32.09 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| f3215d3+dirty | 48.30 MiB | 53.49 MiB | 5.19 MiB |
| d0e3b3e+dirty | 49.74 MiB | 55.09 MiB | 5.34 MiB |
| a5d243c+dirty | 48.30 MiB | 53.54 MiB | 5.23 MiB |
| 7fd0012+dirty | 50.56 MiB | 56.46 MiB | 5.90 MiB |
| 0a147b2+dirty | 49.74 MiB | 55.08 MiB | 5.34 MiB |
| d7d54c6+dirty | 49.74 MiB | 55.09 MiB | 5.34 MiB |
| c823bb5+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
| 5789645+dirty | 49.74 MiB | 54.85 MiB | 5.11 MiB |
| 40c9884+dirty | 49.74 MiB | 55.09 MiB | 5.34 MiB |
| d771cd5+dirty | 49.74 MiB | 55.36 MiB | 5.62 MiB |
…lures
Switch the null-cause branch of rejectSafely from the deprecated
reject(String) overload to reject(code, message) so JS receives a
specific error code ("SentryTimeToDisplay") instead of the generic
"EUNSPECIFIED". Update the affected tests to match the two-arg overload.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Android (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 37a2091+dirty | 407.82 ms | 441.22 ms | 33.40 ms |
| f3215d3+dirty | 411.11 ms | 454.38 ms | 43.27 ms |
| bc8f61e+dirty | 442.98 ms | 506.56 ms | 63.58 ms |
| f9c1ed4+dirty | 431.00 ms | 466.22 ms | 35.22 ms |
| 7887847+dirty | 416.61 ms | 462.04 ms | 45.43 ms |
| 57e0069+dirty | 413.82 ms | 453.47 ms | 39.65 ms |
| a0a3177+dirty | 441.27 ms | 499.86 ms | 58.59 ms |
| 09a902f+dirty | 427.38 ms | 473.80 ms | 46.42 ms |
| a5d243c+dirty | 424.52 ms | 485.18 ms | 60.66 ms |
| 5a23c47+dirty | 423.85 ms | 466.47 ms | 42.62 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 37a2091+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
| f3215d3+dirty | 48.30 MiB | 53.49 MiB | 5.19 MiB |
| bc8f61e+dirty | 49.74 MiB | 55.09 MiB | 5.35 MiB |
| f9c1ed4+dirty | 49.74 MiB | 54.86 MiB | 5.12 MiB |
| 7887847+dirty | 49.74 MiB | 54.81 MiB | 5.07 MiB |
| 57e0069+dirty | 49.74 MiB | 54.85 MiB | 5.11 MiB |
| a0a3177+dirty | 49.74 MiB | 55.37 MiB | 5.63 MiB |
| 09a902f+dirty | 49.74 MiB | 54.81 MiB | 5.07 MiB |
| a5d243c+dirty | 48.30 MiB | 53.54 MiB | 5.23 MiB |
| 5a23c47+dirty | 49.74 MiB | 54.82 MiB | 5.07 MiB |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📢 Type of change
📜 Description
On Android,
RNSentryTimeToDisplay.getTimeToDisplay(reached from thegetNewScreenTimeToDisplayTurboModule method) settles the JS promise from a deferredChoreographerframe callback. Under rapid navigation the bridge callback backing that promise can already have been settled/invalidated by the runtime by the time the frame callback runs — on Expo bridgeless (new architecture) this surfaces asJavaCallback was already settled. Cannot invoke it again. Because the settle happens inside the frame callback, the throw was uncaught and reached the host app as a fatal crash.This routes every settle through guarded helpers (
resolveSafely/rejectSafely) that:AtomicBooleanguard), andWARNINGinstead of propagating.A late/duplicate settle now drops the time-to-display measurement (the expected degradation) instead of crashing the app. The implementation is shared between the old and new architecture, so the fix covers both.
💡 Motivation and Context
Fixes #6709
💚 How did you test it?
New Android unit tests in
RNSentryTimeToDisplayTest:📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps