![[]](https://github.githubassets.com/assets/platform-4ce65ae0bf273ce2.png){kind=link}
![[]](https://github.githubassets.com/assets/platform-dark-ac89fbf373737ed3.png){kind=link}
![[]](https://github.githubassets.com/assets/copilot-e2b89335a66dba00.png){kind=link}
![[]](https://github.githubassets.com/assets/copilot-dark-2b28403b6080e898.png){kind=link}
![[]](https://github.githubassets.com/assets/changelog-fd687bff072883b0.png){kind=link}
![[]](https://github.githubassets.com/assets/changelog-dark-b1c25ae971bd641b.png){kind=link}
![[]](https://github.githubassets.com/assets/universe-a98cd54821b27569.png){kind=link}
{{ message }}
Repository navigation
More items
fix: Crash in SentryFramesTracker.removeListener - #7155
Merged
denrase merged 17 commits intoJan 20, 2026
Merged
denrase merged 17 commits into
denrase merged 17 commits into
Conversation
Mutate listners in `SentryFramesTracker` only on main queue.
Copy link
Copy Markdown
denrase
marked this pull request as ready for review
January 12, 2026 12:50
denrase
requested review from
itaybre,
noahsmartin,
philippsuess and
philprime
as code owners
January 12, 2026 12:50
Copy link
Copy Markdown
Collaborator
Author
|
@philprime Due to the comment in the code, moved the last remaining lock to main thread dispatch instead. |
Copy link
Copy Markdown
Codecov Report All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #7155 +/- ##
=============================================
+ Coverage 85.062% 85.091% +0.029%
=============================================
Files 468 468
Lines 28305 28320 +15
Branches 12532 12547 +15
=============================================
+ Hits 24077 24098 +21
+ Misses 4183 4181 -2
+ Partials 45 41 -4
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
[@philprime]
philprime
left a comment
Copy link
Copy Markdown
Member
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?
Comment thread
CHANGELOG.md
Resolved
Copy link
Copy Markdown
Contributor
Performance metrics
Baseline results on branch: mainStartup times
App size
Previous results on branch: fix/frames-tracker-remove-listner-crashStartup times
App size
|
Copy link
Copy Markdown
Collaborator
Author
|
@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. |
SentryFramesTracker.removeListener(_:)
Jan 12, 2026
Copy link
Copy Markdown
Contributor
Semver Impact of This PRPatch (bug fixes) Changelog PreviewThis is how your changes will appear in the changelog. New FeaturesMetrics
Other
Bug Fixes
Build / dependencies / internalDeps
Other
Other
This preview updates automatically when you update the PR. |
Comment thread
CHANGELOG.md
Resolved
Comment thread
Sources/Swift/Core/Integrations/FramesTracking/SentryFramesTracker.swift
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.
Copy link
Copy Markdown
Collaborator
Author
|
@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()
}
}
}
}
}
} [Bildschirmfoto 2026-01-13 um 11 53 31]
|
![[Bildschirmfoto 2026-01-13 um 11 53 31]](https://private-user-images.githubusercontent.com/3984453/535055328-b4d4adef-e43e-4336-99a2-bece0a98a760.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3OTQ1ODIwNDksIm5iZiI6MTc5NDU4MTc0OSwicGF0aCI6Ii8zOTg0NDUzLzUzNTA1NTMyOC1iNGQ0YWRlZi1lNDNlLTQzMzYtOTlhMi1iZWNlMGE5OGE3NjAucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI2MTExMyUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNjExMTNUMTQ1NTQ5WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9NGNkYWZjMDY5YjBmNzQzM2UwM2JhYmZkZDY3YmI5OGZlMmFiOTUyODI1MDg0NjI1YWZkNTA3Y2RmODkzMjIyOSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmcmVzcG9uc2UtY29udGVudC10eXBlPWltYWdlJTJGcG5nIn0.KO3CzADVTLlgr9rB1F9goqNAVEcumGqwqM7WYGvRc4M){kind=link}
Comment thread
Sources/Swift/Core/Integrations/FramesTracking/SentryFramesTracker.swift
Resolved
[@philprime]
philprime
left a comment
Copy link
Copy Markdown
Member
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
Comment thread
Sources/Swift/Core/Integrations/FramesTracking/SentryFramesTracker.swift
Resolved
SentryFramesTracker.removeListener(_:)
fix: Crash in SentryFramesTracker.removeListener(_:)
Jan 13, 2026
SentryFramesTracker.removeListener(_:)
fix: Crash in SentryFramesTracker.removeListener
Jan 13, 2026
[@philippsuess]
philippsuess
left a comment
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Honestly, I'm unsure about the dispatchSyncOnMainQueue in deinit.
Comment thread
Sources/Swift/Core/Integrations/FramesTracking/SentryFramesTracker.swift
Resolved
Comment thread
Sources/Swift/Core/Integrations/FramesTracking/SentryFramesTracker.swift
Resolved
Comment thread
Sources/Swift/Core/Integrations/FramesTracking/SentryFramesTracker.swift
Resolved
Comment thread
Sources/Swift/Core/Integrations/FramesTracking/SentryFramesTracker.swift
Resolved
[@philippsuess]
philippsuess
left a comment
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
LGTM, thanks
Hide details
View details
1 of 7 tasks
itaybre
added a commit
that referenced
this pull request
Apr 28, 2026
* fix: race in SentryFramesTracker listeners Replace NSHashTable.weakObjects() with a Swift array of weakly-boxed listeners. NSHashTable's internal bucket slots can be zeroed by the ObjC runtime on the thread that deallocates a stored listener while another thread is iterating the table, causing NSConcreteHashTable removeItem: to objc_msgSend a freed entry. Ensuring all mutations run on the main thread (fixed in #7155) is not sufficient: listener objects can still be deallocated on any thread, and the weak-zeroing races with the table's iteration. Reading our own weak-boxed listeners uses objc_loadWeak and is safe against concurrent deallocation. * fix: add changelog entry for SentryFramesTracker listener race (#7839) * Use WeakReference * Fix tests * Simplify listenersCount calculation in SentryFramesTracker
4 tasks done
itaybre
added a commit
that referenced
this pull request
May 29, 2026
* fix: FramesTracker listener-capture deinit race Both `addListener` and `removeListener` strongly captured `listener` in the closure passed to `dispatchAsyncOnMainQueueIfNotMainThread`. When a caller invoked either method from inside its own `init` / `deinit` on a background thread, the dispatched main-queue block tried to retain a freed (or mid-dealloc) object once it reached main. Under iOS 26's hardened ARC this is fatal: `swift_unknownObjectRetain` `EXC_BREAKPOINT`. The `removeListener`-from-`deinit` shape is the documented motivating crash (lldb-confirmed in a Capacitor + WKWebView host); the `addListener` mirror is theoretical but the fix is symmetric and cheap. Capture `ObjectIdentifier(listener)` (a value type) and a pre-built `WeakReference(value: listener)` instead, so the closures never hold the listener directly. The main-queue hop no longer extends the listener's lifetime, and identity matching against still-alive entries (read via `WeakReference.value` / `objc_loadWeak`) keeps working. Adds `[weak self]` to match the lifetime-safety theme of the rest of the file (precedent: `SentryUIDeviceWrapper.start`). This is the third crash class in the same method area. #7155 fixed the original Obj-C `removeListener` crash; #7839 fixed the `NSConcreteHashTable` race that the comment on the `listeners` property records. The remaining race lived in the Swift closure-capture path, exposed when iOS 26 hardened ARC's retain-on-mid-dealloc check. Adds `testAddAndRemoveListener_DoNotRetainListenerAcrossMainQueueHop`, a deterministic regression test using `TestSentryDispatchQueueWrapper` to defer block execution. The test verifies the structural invariant ("closure must not strongly retain the listener") via a `weak` reference assertion, which fails loudly on the pre-fix code with a clear error message: XCTAssertNil failed: "<FrameTrackerListener: 0x...>" - Closures dispatched by addListener/removeListener must not retain the listener across the main-queue hop * Update CHANGELOG.md --------- Co-authored-by: Itay Brenner <itaybrenner@hotmail.com> Co-authored-by: Itay Brenner <itay.brenner@sentry.io>
itaybre
added a commit
that referenced
this pull request
Jun 1, 2026
* fix: FramesTracker listener-capture deinit race Both `addListener` and `removeListener` strongly captured `listener` in the closure passed to `dispatchAsyncOnMainQueueIfNotMainThread`. When a caller invoked either method from inside its own `init` / `deinit` on a background thread, the dispatched main-queue block tried to retain a freed (or mid-dealloc) object once it reached main. Under iOS 26's hardened ARC this is fatal: `swift_unknownObjectRetain` `EXC_BREAKPOINT`. The `removeListener`-from-`deinit` shape is the documented motivating crash (lldb-confirmed in a Capacitor + WKWebView host); the `addListener` mirror is theoretical but the fix is symmetric and cheap. Capture `ObjectIdentifier(listener)` (a value type) and a pre-built `WeakReference(value: listener)` instead, so the closures never hold the listener directly. The main-queue hop no longer extends the listener's lifetime, and identity matching against still-alive entries (read via `WeakReference.value` / `objc_loadWeak`) keeps working. Adds `[weak self]` to match the lifetime-safety theme of the rest of the file (precedent: `SentryUIDeviceWrapper.start`). This is the third crash class in the same method area. #7155 fixed the original Obj-C `removeListener` crash; #7839 fixed the `NSConcreteHashTable` race that the comment on the `listeners` property records. The remaining race lived in the Swift closure-capture path, exposed when iOS 26 hardened ARC's retain-on-mid-dealloc check. Adds `testAddAndRemoveListener_DoNotRetainListenerAcrossMainQueueHop`, a deterministic regression test using `TestSentryDispatchQueueWrapper` to defer block execution. The test verifies the structural invariant ("closure must not strongly retain the listener") via a `weak` reference assertion, which fails loudly on the pre-fix code with a clear error message: XCTAssertNil failed: "<FrameTrackerListener: 0x...>" - Closures dispatched by addListener/removeListener must not retain the listener across the main-queue hop * Update CHANGELOG.md --------- Co-authored-by: Itay Brenner <itaybrenner@hotmail.com> Co-authored-by: Itay Brenner <itay.brenner@sentry.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.