fix: Crash in SentryFramesTracker.removeListener
#7155
Conversation
Mutate listners in `SentryFramesTracker` only on main queue.
denrase
requested review from
itaybre,
noahsmartin,
philipphofmann and
philprime
as code owners
denrase
changed the title
denrase
changed the title
|
@philprime Due to the comment in the code, moved the last remaining lock to main thread dispatch instead. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7155 +/- ##
=============================================
+ Coverage 84.745% 84.748% +0.002%
=============================================
Files 461 461
Lines 27815 27820 +5
Branches 12319 12327 +8
=============================================
+ Hits 23572 23577 +5
+ Misses 4202 3981 -221
- Partials 41 262 +221
... and 33 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
philprime
left a comment
philprime
left a comment
There was a problem hiding this comment.
LGTM, but just to be certain: Instead of using a lock we are going all-in on dispatch on the main thread, because we might be locking the main thread?
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 79e2bb8 | 1216.37 ms | 1242.42 ms | 26.05 ms |
| 83bf9af | 1213.30 ms | 1234.18 ms | 20.89 ms |
| 2e5230b | 1207.41 ms | 1240.41 ms | 33.00 ms |
| 9f7ef2b | 1213.53 ms | 1250.23 ms | 36.70 ms |
| 013fd4d | 1216.02 ms | 1242.16 ms | 26.14 ms |
| d29a425 | 1209.96 ms | 1239.00 ms | 29.04 ms |
| 2f4ddaa | 1227.26 ms | 1260.04 ms | 32.78 ms |
| adeec82 | 1220.43 ms | 1254.94 ms | 34.51 ms |
| 778dadf | 1207.69 ms | 1246.09 ms | 38.40 ms |
| 3bf0d3f | 1202.12 ms | 1237.23 ms | 35.11 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 79e2bb8 | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 83bf9af | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 2e5230b | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 9f7ef2b | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 013fd4d | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| d29a425 | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 2f4ddaa | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| adeec82 | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 778dadf | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 3bf0d3f | 24.14 KiB | 1.04 MiB | 1.02 MiB |
Previous results on branch: fix/frames-tracker-remove-listner-crash
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| fc7e87f | 1219.17 ms | 1239.91 ms | 20.74 ms |
| c6fdfe7 | 1219.85 ms | 1250.46 ms | 30.61 ms |
| af0d84f | 1207.13 ms | 1247.29 ms | 40.16 ms |
| f8aca10 | 1199.12 ms | 1220.77 ms | 21.64 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| fc7e87f | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| c6fdfe7 | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| af0d84f | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| f8aca10 | 24.14 KiB | 1.04 MiB | 1.02 MiB |
|
@philprime Not because of locking the main thread, but the code base was moving away from locks, with the reasoning that But there's the catch that I was not able to reproduce the crash from the issue, but basically it was possible to clear the listeners from a non-main thread, while adding/removing individual listeners was always being done on the main-thread. |
denrase
changed the title
SentryFramesTracker.removeListener(_:)
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛
Build / dependencies / internal 🔧Deps
Other
Other
🤖 This preview updates automatically when you update the PR. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Sources/Swift/Core/Integrations/FramesTracking/SentryFramesTracker.swift
Outdated
Show resolved
Hide resolved
- Updated `SentryFramesTracker` to ensure listeners are managed on the main queue, preventing potential crashes. - Made `isStarted` property private for better encapsulation. - Enhanced deinitialization to safely stop the tracker and avoid async dispatch issues.
|
@philprime Would be great if you could take another look. I tried reproducing again after the cursor bot comment and could get the frames tracker to crash using this code in the sample app. I added additional precautions to fix this, which did work. While I still could not reproduce the initial issue, this should offer more protection against potential races. @IBAction func useAfterFree(_ sender: UIButton) {
highlightButton(sender)
let framesTracker = SentryDependencyContainer.sharedInstance().framesTracker
class TestListener: NSObject, SentryFramesTrackerListener {
func framesTrackerHasNewFrame(_ newFrameDate: Date) {
// Access during callback to create more contention
}
}
var listeners: [TestListener] = []
for _ in 0..<50 {
let listener = TestListener()
listeners.append(listener)
framesTracker.addListener(listener)
}
for _ in 0..<20 {
DispatchQueue.global().async {
for listener in listeners {
for i in 0..<100 {
framesTracker.removeListener(listener)
if i % 2 == 0 {
framesTracker.stop()
} else {
framesTracker.start()
}
}
}
}
}
}
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Sources/Swift/Core/Integrations/FramesTracking/SentryFramesTracker.swift
Show resolved
Hide resolved
philprime
left a comment
philprime
left a comment
There was a problem hiding this comment.
LGTM, this also seems to be more consistent. Maybe it makes sense if @itaybre or @philipphofmann also gives this a second review
| private func willResignActive() { | ||
| pause() | ||
| } | ||
|
|
||
| private func unpause() { | ||
| guard !isRunning else { return } | ||
|
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
philipphofmann
changed the title
SentryFramesTracker.removeListener(_:)SentryFramesTracker.removeListener(_:)
philipphofmann
changed the title
SentryFramesTracker.removeListener(_:)SentryFramesTracker.removeListener
philipphofmann
left a comment
philipphofmann
left a comment
There was a problem hiding this comment.
Honestly, I'm unsure about the dispatchSyncOnMainQueue in deinit.
Sources/Swift/Core/Integrations/FramesTracking/SentryFramesTracker.swift
Show resolved
Hide resolved
|
|
||
| deinit { | ||
| // Avoid async dispatch with self capture on deinit. | ||
| dispatchQueueWrapper.dispatchSyncOnMainQueue { |
There was a problem hiding this comment.
h: I don't think it'a good idea to call dispatch_sync on a deinit method. Even though most of the time we're going to call this from the main thread and we won't run dispatch_sync from a BG thread, this can cause weird behaviour and hard-to-track bugs. To me, it seems this is a duct tape around a deeper problem of how we handle synchronization around the listeners. I think it would be better to come up with a different strategy. For example, I don't think we need to remove all listeners in deinit. The listeners array anyways is going to be deallocated. Maybe we should rather call a stop method that only removes the observers and calls pause, but not removing the observers.
There was a problem hiding this comment.
Yeah that makes sense, what we def need to do is invalidate the display link, as it retains the object is is reporting to. Will update, thx for the insights. 👍
| dispatchQueueWrapper.dispatchAsyncOnMainQueueIfNotMainThread { | ||
| self.startInternal() | ||
| } | ||
| } | ||
|
|
||
| private func startInternal() { |
There was a problem hiding this comment.
Can we double check that no one calling start expects data immediately?
| // Need to invalidate so DisplayLink is releasing this object. Calling this is thread-safe. | ||
| displayLinkWrapper.invalidate() |
There was a problem hiding this comment.
m: @denrase I think we should also remove the observers as we do in stopInternal. I think we could add a new method that calls displayLinkWrapper.invalidate() and also removes the observers. Then we can call it from here and stopInternal
📜 Description
Mutate listeners in
SentryFramesTrackeronly on main queue.Access to listeners was moved to main thread dispatch instead of locks, but one lock was probably forgotten. The reason why this was changed is added as a comment in code, so because of this we do not use the locking anymore.
If this issue should remain even after this fix, we can still additionally add locks/synchronisation around listeners access.
💡 Motivation and Context
Closes https://linear.app/getsentry/issue/COCOA-869/sentryframestracker-removelistener-crash
💚 How did you test it?
Unfortunately, I could not reproduce the crash, but the sync was only in one place, suggesting it was forgotten to move this to main thread access only.
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.