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: Rare battery breadcrumbs crash #3582

Merged
merged 3 commits into from
Jan 29, 2024

Conversation

philipphofmann
Copy link
Member

📜 Description

We see rare crashes in getBatteryStatus of SentrySystemEventBreadcrumbs. The object of an NSNotification can be nil, and we didn't check for that edge case. This PR fixes that problem and uses dictionaryWithCapacity to slightly reduce the memory footprint for getBatteryStatus. This change is not guaranteed to fix these rare crashes, but it won't do any harm.

Internal SDK crashes

💡 Motivation and Context

Came up while investigating #3341.

💚 How did you test it?

Unit testing.

📝 Checklist

You have to check all boxes before merging:

  • 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 change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

We see rare crashes in getBatteryStatus of SentrySystemEventBreadcrumbs.
The object of an NSNotification can be nil, and we didn't check for that
edge case. This PR fixes that problem and uses dictionaryWithCapacity to
slightly reduce the memory footprint for getBatteryStatus. This change
is not guaranteed to fix these rare crashes, but it won't do any harm.
Copy link

codecov bot commented Jan 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (eef4553) 89.285% compared to head (ce54466) 89.289%.
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3582       +/-   ##
=============================================
+ Coverage   89.285%   89.289%   +0.003%     
=============================================
  Files          529       529               
  Lines        57753     57764       +11     
  Branches     20670     20673        +3     
=============================================
+ Hits         51565     51577       +12     
- Misses        5277      5281        +4     
+ Partials       911       906        -5     
Files Coverage Δ
Sources/Sentry/SentrySystemEventBreadcrumbs.m 100.000% <100.000%> (ø)
...Breadcrumbs/SentrySystemEventBreadcrumbsTest.swift 98.062% <100.000%> (+0.070%) ⬆️

... and 14 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

Copy link

github-actions bot commented Jan 29, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1212.12 ms 1221.49 ms 9.37 ms
Size 21.58 KiB 418.14 KiB 396.56 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
e324230 1225.84 ms 1250.40 ms 24.57 ms
31208ed 1258.36 ms 1263.86 ms 5.50 ms
16450b8 1196.33 ms 1217.26 ms 20.93 ms
56ec5d0 1194.39 ms 1236.94 ms 42.55 ms
7cd187e 1229.02 ms 1233.06 ms 4.04 ms
407ff99 1190.89 ms 1237.18 ms 46.29 ms
d413317 1203.27 ms 1215.02 ms 11.75 ms
881a955 1242.24 ms 1247.85 ms 5.61 ms
98cca71 1226.66 ms 1242.94 ms 16.28 ms
b9a9ffd 1221.18 ms 1235.37 ms 14.19 ms

App size

Revision Plain With Sentry Diff
e324230 22.85 KiB 408.87 KiB 386.02 KiB
31208ed 20.76 KiB 435.26 KiB 414.50 KiB
16450b8 22.85 KiB 410.98 KiB 388.13 KiB
56ec5d0 20.76 KiB 414.44 KiB 393.68 KiB
7cd187e 20.76 KiB 401.65 KiB 380.89 KiB
407ff99 20.76 KiB 427.86 KiB 407.10 KiB
d413317 20.76 KiB 420.71 KiB 399.95 KiB
881a955 22.85 KiB 407.63 KiB 384.78 KiB
98cca71 22.85 KiB 411.14 KiB 388.29 KiB
b9a9ffd 21.58 KiB 418.43 KiB 396.85 KiB

Previous results on branch: fix/battery-notification-nil-check

Startup times

Revision Plain With Sentry Diff
33e820d 1210.41 ms 1231.75 ms 21.34 ms
ae790ff 1241.92 ms 1261.14 ms 19.22 ms

App size

Revision Plain With Sentry Diff
33e820d 21.58 KiB 417.94 KiB 396.36 KiB
ae790ff 21.58 KiB 418.00 KiB 396.42 KiB

Copy link
Member

@kahest kahest left a comment

Choose a reason for hiding this comment

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

LGTM, just one small suggestion

thx for the good PR description and the opportunistic refactor 👍

Sources/Sentry/SentrySystemEventBreadcrumbs.m Show resolved Hide resolved
@philipphofmann philipphofmann merged commit 30f8871 into main Jan 29, 2024
71 checks passed
@philipphofmann philipphofmann deleted the fix/battery-notification-nil-check branch January 29, 2024 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants