-
-
Notifications
You must be signed in to change notification settings - Fork 359
fix(ios): Fix duplicate JS error reporting on iOS with New Architecture #5532
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@sentry review |
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. |
Android (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1c38acd+dirty | 398.09 ms | 409.18 ms | 11.09 ms |
| b7aa1aa+dirty | 324.73 ms | 327.76 ms | 3.03 ms |
| 785ffb1 | 471.92 ms | 460.96 ms | -10.96 ms |
| fdbea8b+dirty | 494.72 ms | 529.06 ms | 34.34 ms |
| d1bfbde+dirty | 478.88 ms | 505.52 ms | 26.64 ms |
| ee69ed5+dirty | 409.44 ms | 441.30 ms | 31.86 ms |
| 0d6e618+dirty | 414.00 ms | 416.90 ms | 2.90 ms |
| f8d19f8+dirty | 422.98 ms | 421.98 ms | -1.00 ms |
| 8e653ac+dirty | 360.28 ms | 372.04 ms | 11.76 ms |
| 170d5ea+dirty | 407.92 ms | 422.49 ms | 14.57 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1c38acd+dirty | 43.75 MiB | 48.08 MiB | 4.33 MiB |
| b7aa1aa+dirty | 17.75 MiB | 19.75 MiB | 2.00 MiB |
| 785ffb1 | 17.75 MiB | 20.15 MiB | 2.41 MiB |
| fdbea8b+dirty | 43.75 MiB | 48.05 MiB | 4.29 MiB |
| d1bfbde+dirty | 43.75 MiB | 47.99 MiB | 4.24 MiB |
| ee69ed5+dirty | 43.75 MiB | 48.04 MiB | 4.29 MiB |
| 0d6e618+dirty | 43.75 MiB | 47.99 MiB | 4.24 MiB |
| f8d19f8+dirty | 43.75 MiB | 48.08 MiB | 4.33 MiB |
| 8e653ac+dirty | 17.75 MiB | 19.75 MiB | 2.00 MiB |
| 170d5ea+dirty | 17.75 MiB | 19.70 MiB | 1.95 MiB |
Android (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1c38acd+dirty | 411.84 ms | 470.18 ms | 58.34 ms |
| b7aa1aa+dirty | 281.02 ms | 317.53 ms | 36.51 ms |
| fdbea8b+dirty | 551.94 ms | 577.02 ms | 25.08 ms |
| a02e30b+dirty | 346.13 ms | 381.76 ms | 35.62 ms |
| c7f264b+dirty | 356.98 ms | 407.46 ms | 50.48 ms |
| d1bfbde+dirty | 438.90 ms | 494.82 ms | 55.92 ms |
| ee69ed5+dirty | 411.19 ms | 447.04 ms | 35.85 ms |
| 0d6e618+dirty | 369.02 ms | 387.69 ms | 18.67 ms |
| 46bd012+dirty | 333.76 ms | 359.24 ms | 25.48 ms |
| f8d19f8+dirty | 374.17 ms | 383.40 ms | 9.23 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1c38acd+dirty | 43.94 MiB | 48.91 MiB | 4.97 MiB |
| b7aa1aa+dirty | 7.15 MiB | 8.46 MiB | 1.31 MiB |
| fdbea8b+dirty | 43.94 MiB | 48.87 MiB | 4.93 MiB |
| a02e30b+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
| c7f264b+dirty | 7.15 MiB | 8.41 MiB | 1.26 MiB |
| d1bfbde+dirty | 43.94 MiB | 48.82 MiB | 4.88 MiB |
| ee69ed5+dirty | 43.94 MiB | 48.87 MiB | 4.93 MiB |
| 0d6e618+dirty | 43.94 MiB | 48.82 MiB | 4.88 MiB |
| 46bd012+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
| f8d19f8+dirty | 43.94 MiB | 48.91 MiB | 4.97 MiB |
iOS (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 46e3d54+dirty | 1213.17 ms | 1216.33 ms | 3.17 ms |
| 46bd012+dirty | 1231.78 ms | 1212.30 ms | -19.47 ms |
| eb07ba3+dirty | 1214.49 ms | 1221.59 ms | 7.10 ms |
| 0b64753+dirty | 1225.77 ms | 1232.98 ms | 7.21 ms |
| 6a70a7e+dirty | 1231.40 ms | 1239.49 ms | 8.09 ms |
| 3401245+dirty | 1216.70 ms | 1241.44 ms | 24.74 ms |
| 2104bb9+dirty | 1221.63 ms | 1214.73 ms | -6.91 ms |
| 3e0a5f9+dirty | 1233.65 ms | 1239.10 ms | 5.45 ms |
| 3bd3f0d+dirty | 1230.18 ms | 1243.41 ms | 13.22 ms |
| 955f2eb+dirty | 1225.78 ms | 1239.27 ms | 13.49 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 46e3d54+dirty | 3.41 MiB | 4.58 MiB | 1.17 MiB |
| 46bd012+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
| eb07ba3+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
| 0b64753+dirty | 3.19 MiB | 4.55 MiB | 1.36 MiB |
| 6a70a7e+dirty | 3.19 MiB | 4.54 MiB | 1.36 MiB |
| 3401245+dirty | 3.41 MiB | 4.58 MiB | 1.17 MiB |
| 2104bb9+dirty | 3.19 MiB | 4.57 MiB | 1.38 MiB |
| 3e0a5f9+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
| 3bd3f0d+dirty | 3.19 MiB | 4.55 MiB | 1.37 MiB |
| 955f2eb+dirty | 3.19 MiB | 4.55 MiB | 1.36 MiB |
iOS (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 46e3d54+dirty | 1216.40 ms | 1210.47 ms | -5.93 ms |
| 46bd012+dirty | 1220.49 ms | 1226.89 ms | 6.40 ms |
| eb07ba3+dirty | 1222.46 ms | 1220.37 ms | -2.08 ms |
| 0b64753+dirty | 1232.49 ms | 1226.96 ms | -5.53 ms |
| 6a70a7e+dirty | 1225.82 ms | 1230.79 ms | 4.98 ms |
| 3401245+dirty | 1222.60 ms | 1223.06 ms | 0.46 ms |
| 2104bb9+dirty | 1222.94 ms | 1221.16 ms | -1.77 ms |
| 3e0a5f9+dirty | 1226.94 ms | 1230.02 ms | 3.08 ms |
| 3bd3f0d+dirty | 1231.51 ms | 1229.10 ms | -2.41 ms |
| 955f2eb+dirty | 1235.06 ms | 1253.88 ms | 18.81 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 46e3d54+dirty | 3.41 MiB | 4.58 MiB | 1.17 MiB |
| 46bd012+dirty | 2.63 MiB | 3.81 MiB | 1.18 MiB |
| eb07ba3+dirty | 2.63 MiB | 3.81 MiB | 1.18 MiB |
| 0b64753+dirty | 2.63 MiB | 3.98 MiB | 1.35 MiB |
| 6a70a7e+dirty | 2.63 MiB | 3.98 MiB | 1.34 MiB |
| 3401245+dirty | 3.41 MiB | 4.58 MiB | 1.17 MiB |
| 2104bb9+dirty | 2.63 MiB | 4.00 MiB | 1.37 MiB |
| 3e0a5f9+dirty | 2.63 MiB | 3.81 MiB | 1.18 MiB |
| 3bd3f0d+dirty | 2.63 MiB | 3.99 MiB | 1.35 MiB |
| 955f2eb+dirty | 2.63 MiB | 3.98 MiB | 1.35 MiB |
|
|
||
| ### Fixes | ||
|
|
||
| - Fix duplicate error reporting on iOS with New Architecture ([#5532](https://github.com/getsentry/sentry-react-native/pull/5532)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we specify if this was fixed on a specific version of React Native onwards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this behavior is part of New Architecture's error handling, so the fix applies to all RN versions with New Architecture enabled (0.69+), not just a specific version
lucas-zimerman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the changelog question, LGTM!
📢 Type of change
📜 Description
Fix duplicate JS error reporting on iOS with New Architecture.
With New Architecture, React Native wraps JS errors in C++ exceptions. These exceptions are caught by the native crash handler and should be filtered out since the JS error is already reported by the JS error handler. The key indicator is "ExceptionsManager.reportException" in the exception value, which is React Native's mechanism for reporting JS errors to the native layer.
💡 Motivation and Context
Fixes #5529
💚 How did you test it?
CI
📝 Checklist
sendDefaultPIIis enabled🔮 Next steps