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 in SentryTracer when cancelling timer #3333

Merged
merged 7 commits into from
Oct 12, 2023
Merged

Conversation

philipphofmann
Copy link
Member

📜 Description

The SentryTracer starts the timer for the deadline on the main thread and invalidates the timer, not necessarily on the main thread, but invalidate has to be called on the same thread on which the timer was started. This is fixed by starting and invalidating the NSTimer on the main thread.

💡 Motivation and Context

Fixes GH-3320

💚 How did you test it?

It isn't straightforward to reproduce the EXC_BAD_ACCESS in the SentryTracer, cause violating calling invalidate on a different thread doesn't necessarily lead to crashes, see Apple Docs. Unit tests.

📝 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

The SentryTracer starts the timer for the deadline on the main thread
and invalidates the timer, not necessarily on the main thread, but
invalidate has to be called on the same thread on which the timer was
started. This is fixed by starting and invalidating the NSTimer on the
main thread.

Fixes GH-3320
@github-actions
Copy link

github-actions bot commented Oct 6, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1233.83 ms 1264.29 ms 30.46 ms
Size 22.85 KiB 411.37 KiB 388.52 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
d257eb9 1226.08 ms 1256.54 ms 30.46 ms
89bc37d 1228.20 ms 1257.10 ms 28.90 ms
1bbcb9c 1189.61 ms 1223.50 ms 33.89 ms
fd6a31c 1204.73 ms 1222.34 ms 17.61 ms
7bc3c0d 1212.35 ms 1228.94 ms 16.59 ms
5616e0a 1237.00 ms 1260.43 ms 23.43 ms
9d56232 1192.09 ms 1228.86 ms 36.77 ms
98713e6 1211.48 ms 1244.42 ms 32.94 ms
b385962 1195.85 ms 1221.63 ms 25.78 ms
4259afd 1234.04 ms 1256.76 ms 22.72 ms

App size

Revision Plain With Sentry Diff
d257eb9 20.76 KiB 433.23 KiB 412.47 KiB
89bc37d 20.76 KiB 401.53 KiB 380.77 KiB
1bbcb9c 20.76 KiB 426.10 KiB 405.34 KiB
fd6a31c 20.76 KiB 436.50 KiB 415.74 KiB
7bc3c0d 20.76 KiB 427.35 KiB 406.59 KiB
5616e0a 22.85 KiB 407.45 KiB 384.60 KiB
9d56232 20.76 KiB 425.80 KiB 405.04 KiB
98713e6 20.76 KiB 435.22 KiB 414.46 KiB
b385962 20.76 KiB 399.69 KiB 378.93 KiB
4259afd 20.76 KiB 419.70 KiB 398.94 KiB

Previous results on branch: fix/tracer-crash

Startup times

Revision Plain With Sentry Diff
4bfdca8 1226.92 ms 1246.14 ms 19.22 ms
b7fd8c4 1244.22 ms 1244.50 ms 0.28 ms
fc0ea36 1247.71 ms 1271.24 ms 23.53 ms
4750b1a 1256.69 ms 1278.35 ms 21.65 ms
a9d8255 1238.90 ms 1246.88 ms 7.98 ms

App size

Revision Plain With Sentry Diff
4bfdca8 22.85 KiB 409.07 KiB 386.22 KiB
b7fd8c4 22.85 KiB 409.07 KiB 386.22 KiB
fc0ea36 22.85 KiB 411.38 KiB 388.53 KiB
4750b1a 22.85 KiB 409.06 KiB 386.21 KiB
a9d8255 22.85 KiB 409.07 KiB 386.22 KiB

@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

Merging #3333 (0aeff0a) into main (1437c68) will increase coverage by 0.053%.
The diff coverage is 94.736%.

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3333       +/-   ##
=============================================
+ Coverage   89.251%   89.305%   +0.053%     
=============================================
  Files          502       502               
  Lines        54400     54487       +87     
  Branches     19538     19572       +34     
=============================================
+ Hits         48553     48660      +107     
+ Misses        4986      4852      -134     
- Partials       861       975      +114     
Files Coverage Δ
Sources/Sentry/SentryTracer.m 96.250% <100.000%> (-0.101%) ⬇️
...ts/SentryTests/Transaction/SentryTracerTests.swift 99.182% <93.750%> (-0.426%) ⬇️

... and 32 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 1437c68...0aeff0a. Read the comment docs.

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

Good catch. Looks basically right, I had one quick question. Ship when ready!

Sources/Sentry/SentryTracer.m Outdated Show resolved Hide resolved
@philipphofmann philipphofmann merged commit fb7eecf into main Oct 12, 2023
66 checks passed
@philipphofmann philipphofmann deleted the fix/tracer-crash branch October 12, 2023 08:13
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.

EXC_BAD_ACCESS in SentryTracer cancelDeadlineTimer
2 participants