Skip to content
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

Fix crash introduced with uint64 support #2664

Merged
merged 1 commit into from
Jan 30, 2023
Merged

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Jan 27, 2023

📜 Description

We should have added onUIntegerElement to SentryCrashReportFixer. So the SDK crashed while reading the crash report on the app start. This is fixed now by adding onUIntegerElement. Furthermore, I validated that all SentryCrashJSONDecodeCallbacks now have a mapping to onUIntegerElement.

Related to #2631 and #2642.

Fixes GH-2663

#skip-changelog

💡 Motivation and Context

The SDK crashed while reading the crash report on the app start

💚 How did you test it?

Unit tests and calling crash in the iOS-Swift sample on a real device.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

We forgot to add onUIntegerElement to SentryCrashReportFixer.
So the SDK crashed while reading the crash report on the app start.
This is fixed now by adding onUIntegerElement. Furthermore, I validated
that all SentryCrashJSONDecodeCallbacks now have a mapping to
onUIntegerElement.
@brustolin brustolin linked an issue Jan 27, 2023 that may be closed by this pull request
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions
Copy link

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1222.42 ms 1254.41 ms 31.99 ms
Size 20.76 KiB 420.22 KiB 399.46 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
d10ae0c 1250.02 ms 1253.74 ms 3.72 ms
3a31fc9 1237.35 ms 1249.02 ms 11.67 ms
b2f82fa 1237.78 ms 1256.02 ms 18.24 ms
15b8c61 1223.16 ms 1244.83 ms 21.67 ms
b2f82fa 1236.94 ms 1262.86 ms 25.92 ms
8cb9355 1225.23 ms 1231.22 ms 5.99 ms
56ec5d0 1194.39 ms 1236.94 ms 42.55 ms
dc0db9e 1222.10 ms 1240.90 ms 18.80 ms
e71cf92 1201.69 ms 1226.52 ms 24.83 ms
302ee8b 1194.02 ms 1223.34 ms 29.32 ms

App size

Revision Plain With Sentry Diff
d10ae0c 20.76 KiB 419.86 KiB 399.10 KiB
3a31fc9 20.76 KiB 414.45 KiB 393.69 KiB
b2f82fa 20.76 KiB 419.62 KiB 398.86 KiB
15b8c61 20.76 KiB 419.67 KiB 398.91 KiB
b2f82fa 20.76 KiB 419.62 KiB 398.86 KiB
8cb9355 20.76 KiB 419.70 KiB 398.95 KiB
56ec5d0 20.76 KiB 414.44 KiB 393.68 KiB
dc0db9e 20.76 KiB 419.62 KiB 398.86 KiB
e71cf92 20.76 KiB 419.85 KiB 399.10 KiB
302ee8b 20.76 KiB 419.62 KiB 398.87 KiB

@codecov
Copy link

codecov bot commented Jan 27, 2023

Codecov Report

Merging #2664 (8ce3854) into main (ecd9ecd) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2664      +/-   ##
==========================================
- Coverage   80.46%   80.40%   -0.06%     
==========================================
  Files         246      246              
  Lines       22862    22859       -3     
  Branches    10116    10106      -10     
==========================================
- Hits        18395    18380      -15     
- Misses       4002     4014      +12     
  Partials      465      465              
Impacted Files Coverage Δ
...h/Recording/Monitors/SentryCrashMonitor_AppState.c 80.13% <100.00%> (+2.31%) ⬆️
...ces/SentryCrash/Recording/SentryCrashReportFixer.c 81.87% <100.00%> (+0.54%) ⬆️
Sources/Sentry/SentryProfilingLogging.mm 0.00% <0.00%> (-51.86%) ⬇️
Sources/Sentry/include/SentryMachLogging.hpp 62.50% <0.00%> (-18.75%) ⬇️
Sources/Sentry/NSLocale+Sentry.m 90.00% <0.00%> (-10.00%) ⬇️
Sources/Sentry/include/SentryProfilingLogging.hpp 45.45% <0.00%> (-9.10%) ⬇️
Sources/Sentry/include/SentryLog.h 80.00% <0.00%> (-4.22%) ⬇️
Sources/Sentry/SentryMachLogging.cpp 2.51% <0.00%> (-1.01%) ⬇️
Sources/Sentry/SentryNetworkTracker.m 95.20% <0.00%> (-0.19%) ⬇️
Sources/Sentry/SentryCrashIntegration.m 98.66% <0.00%> (+0.03%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecd9ecd...8ce3854. Read the comment docs.

@philipphofmann philipphofmann merged commit 8526e93 into main Jan 30, 2023
@philipphofmann philipphofmann deleted the fix/crash-report-fixer branch January 30, 2023 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App don't open again after first crash
2 participants