fix: Synchronization issue in FramesTracker (#3571)

* fix: Synchronization issue in FramesTracker Synchronize the call to SentryFramesTracker.resetProfilingTimestamps by dispatching it to the main thread cause there is no guarantee that the SDK calls this method on the main thread. Previously, this issue led to crashes in SentryFramesTracker. Fixes GH-3511
getsentry · Jan 29, 2024 · eef4553 · eef4553
1 parent 10eb288
commit eef4553
Show file tree

Hide file tree

Showing 9 changed files with 44 additions and 7 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,7 @@
 
 - Move header reference out of "extern C" (#3538)
 - Clarify FramesTracker log message (#3570)
+- Fix synchronization issue in FramesTracker (#3571)
 
 ## 8.19.0
 

diff --git a/Sources/Sentry/SentryDependencyContainer.m b/Sources/Sentry/SentryDependencyContainer.m
@@ -275,6 +275,7 @@ - (SentryFramesTracker *)framesTracker
                 _framesTracker = [[SentryFramesTracker alloc]
                     initWithDisplayLinkWrapper:[[SentryDisplayLinkWrapper alloc] init]
                                   dateProvider:self.dateProvider
+                          dispatchQueueWrapper:self.dispatchQueueWrapper
                      keepDelayedFramesDuration:SENTRY_AUTO_TRANSACTION_MAX_DURATION];
             }
         }

diff --git a/Sources/Sentry/SentryFramesTracker.m b/Sources/Sentry/SentryFramesTracker.m
@@ -6,6 +6,7 @@
 #    import "SentryCurrentDateProvider.h"
 #    import "SentryDelayedFrame.h"
 #    import "SentryDelayedFramesTracker.h"
+#    import "SentryDispatchQueueWrapper.h"
 #    import "SentryDisplayLinkWrapper.h"
 #    import "SentryLog.h"
 #    import "SentryProfiler.h"
@@ -28,6 +29,7 @@
 
 @property (nonatomic, strong, readonly) SentryDisplayLinkWrapper *displayLinkWrapper;
 @property (nonatomic, strong, readonly) SentryCurrentDateProvider *dateProvider;
+@property (nonatomic, strong, readonly) SentryDispatchQueueWrapper *dispatchQueueWrapper;
 @property (nonatomic, assign) CFTimeInterval previousFrameTimestamp;
 @property (nonatomic) uint64_t previousFrameSystemTimestamp;
 @property (nonatomic) uint64_t currentFrameRate;
@@ -58,12 +60,14 @@ @implementation SentryFramesTracker {
 
 - (instancetype)initWithDisplayLinkWrapper:(SentryDisplayLinkWrapper *)displayLinkWrapper
                               dateProvider:(SentryCurrentDateProvider *)dateProvider
+                      dispatchQueueWrapper:(SentryDispatchQueueWrapper *)dispatchQueueWrapper
                  keepDelayedFramesDuration:(CFTimeInterval)keepDelayedFramesDuration
 {
     if (self = [super init]) {
         _isRunning = NO;
         _displayLinkWrapper = displayLinkWrapper;
         _dateProvider = dateProvider;
+        _dispatchQueueWrapper = dispatchQueueWrapper;
         _delayedFramesTracker = [[SentryDelayedFramesTracker alloc]
             initWithKeepDelayedFramesDuration:keepDelayedFramesDuration
                                  dateProvider:dateProvider];
@@ -91,19 +95,27 @@ - (void)resetFrames
 
     self.previousFrameTimestamp = SentryPreviousFrameInitialValue;
 #    if SENTRY_TARGET_PROFILING_SUPPORTED
-    [self resetProfilingTimestamps];
+    [self resetProfilingTimestampsInternal];
 #    endif // SENTRY_TARGET_PROFILING_SUPPORTED
 
     [self.delayedFramesTracker resetDelayedFramesTimeStamps];
 }
 
 #    if SENTRY_TARGET_PROFILING_SUPPORTED
 - (void)resetProfilingTimestamps
+{
+    // The DisplayLink callback always runs on the main thread. We dispatch this to the main thread
+    // instead to avoid using locks in the DisplayLink callback.
+    [self.dispatchQueueWrapper dispatchOnMainQueue:^{ [self resetProfilingTimestampsInternal]; }];
+}
+
+- (void)resetProfilingTimestampsInternal
 {
     self.frozenFrameTimestamps = [SentryMutableFrameInfoTimeSeries array];
     self.slowFrameTimestamps = [SentryMutableFrameInfoTimeSeries array];
     self.frameRateTimestamps = [SentryMutableFrameInfoTimeSeries array];
 }
+
 #    endif // SENTRY_TARGET_PROFILING_SUPPORTED
 
 - (void)start

diff --git a/Sources/Sentry/include/SentryFramesTracker.h b/Sources/Sentry/include/SentryFramesTracker.h
@@ -6,6 +6,7 @@
 
 @class SentryOptions, SentryDisplayLinkWrapper, SentryScreenFrames;
 @class SentryCurrentDateProvider;
+@class SentryDispatchQueueWrapper;
 
 NS_ASSUME_NONNULL_BEGIN
 
@@ -24,6 +25,7 @@ NS_ASSUME_NONNULL_BEGIN
 
 - (instancetype)initWithDisplayLinkWrapper:(SentryDisplayLinkWrapper *)displayLinkWrapper
                               dateProvider:(SentryCurrentDateProvider *)dateProvider
+                      dispatchQueueWrapper:(SentryDispatchQueueWrapper *)dispatchQueueWrapper
                  keepDelayedFramesDuration:(CFTimeInterval)keepDelayedFramesDuration;
 
 @property (nonatomic, assign, readonly) SentryScreenFrames *currentFrames;

diff --git a/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift b/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift
@@ -36,7 +36,7 @@ class SentryProfilerSwiftTests: XCTestCase {
 
 #if !os(macOS)
         lazy var displayLinkWrapper = TestDisplayLinkWrapper(dateProvider: currentDateProvider)
-        lazy var framesTracker = SentryFramesTracker(displayLinkWrapper: displayLinkWrapper, dateProvider: currentDateProvider, keepDelayedFramesDuration: 0)
+        lazy var framesTracker = SentryFramesTracker(displayLinkWrapper: displayLinkWrapper, dateProvider: currentDateProvider, dispatchQueueWrapper: SentryDispatchQueueWrapper(), keepDelayedFramesDuration: 0)
 #endif
 
         init() {

diff --git a/Tests/SentryTests/Integrations/Performance/AppStartTracking/SentryAppStartTrackerTests.swift b/Tests/SentryTests/Integrations/Performance/AppStartTracking/SentryAppStartTrackerTests.swift
@@ -46,7 +46,7 @@ class SentryAppStartTrackerTests: NotificationCenterTestCase {
                 notificationCenterWrapper: SentryNSNotificationCenterWrapper()
             )
 
-            framesTracker = SentryFramesTracker(displayLinkWrapper: displayLinkWrapper, dateProvider: currentDate, keepDelayedFramesDuration: 0)
+            framesTracker = SentryFramesTracker(displayLinkWrapper: displayLinkWrapper, dateProvider: currentDate, dispatchQueueWrapper: SentryDispatchQueueWrapper(), keepDelayedFramesDuration: 0)
             framesTracker.start()
 
             runtimeInitTimestamp = SentryDependencyContainer.sharedInstance().dateProvider.date().addingTimeInterval(0.2)

diff --git a/Tests/SentryTests/Integrations/Performance/FramesTracking/SentryFramesTrackerTests.swift b/Tests/SentryTests/Integrations/Performance/FramesTracking/SentryFramesTrackerTests.swift
@@ -23,7 +23,7 @@ class SentryFramesTrackerTests: XCTestCase {
             slowestSlowFrameDelay = (displayLinkWrapper.slowestSlowFrameDuration - slowFrameThreshold(displayLinkWrapper.currentFrameRate.rawValue))
         }
 
-        lazy var sut: SentryFramesTracker = SentryFramesTracker(displayLinkWrapper: displayLinkWrapper, dateProvider: dateProvider, keepDelayedFramesDuration: keepDelayedFramesDuration)
+        lazy var sut: SentryFramesTracker = SentryFramesTracker(displayLinkWrapper: displayLinkWrapper, dateProvider: dateProvider, dispatchQueueWrapper: SentryDispatchQueueWrapper(), keepDelayedFramesDuration: keepDelayedFramesDuration)
     }
 
     private var fixture: Fixture!
@@ -601,13 +601,34 @@ class SentryFramesTrackerTests: XCTestCase {
 
     func testDealloc_CallsStop() {
         func sutIsDeallocatedAfterCallingMe() {
-            _ = SentryFramesTracker(displayLinkWrapper: fixture.displayLinkWrapper, dateProvider: fixture.dateProvider, keepDelayedFramesDuration: 0)
+            _ = SentryFramesTracker(displayLinkWrapper: fixture.displayLinkWrapper, dateProvider: fixture.dateProvider, dispatchQueueWrapper: SentryDispatchQueueWrapper(), keepDelayedFramesDuration: 0)
         }
         sutIsDeallocatedAfterCallingMe()
 
         XCTAssertEqual(1, fixture.displayLinkWrapper.invalidateInvocations.count)
     }
 
+#if os(iOS) || os(macOS) || targetEnvironment(macCatalyst)
+    func testResetProfilingTimestamps_FromBackgroundThread() {
+        let sut = fixture.sut
+        sut.start()
+
+        let queue = DispatchQueue(label: "reset profiling timestamps", attributes: [.initiallyInactive, .concurrent])
+
+        for _ in 0..<10_000 {
+            queue.async {
+                sut.resetProfilingTimestamps()
+            }
+        }
+
+        queue.activate()
+
+        for _ in 0..<1_000 {
+            self.fixture.displayLinkWrapper.normalFrame()
+        }
+    }
+#endif // os(iOS) || os(tvOS) || targetEnvironment(macCatalyst)
+
     private func givenMoreDelayedFramesThanTransactionMaxDuration(_ framesTracker: SentryFramesTracker) -> (UInt64, UInt, Double) {
         let displayLink = fixture.displayLinkWrapper
         displayLink.call()

diff --git a/...entryTests/Integrations/Performance/UIViewController/SentryTimeToDisplayTrackerTest.swift b/...entryTests/Integrations/Performance/UIViewController/SentryTimeToDisplayTrackerTest.swift
@@ -16,7 +16,7 @@ class SentryTimeToDisplayTrackerTest: XCTestCase {
         var framesTracker: SentryFramesTracker
 
         init() {
-            framesTracker = SentryFramesTracker(displayLinkWrapper: displayLinkWrapper, dateProvider: dateProvider, keepDelayedFramesDuration: 0)
+            framesTracker = SentryFramesTracker(displayLinkWrapper: displayLinkWrapper, dateProvider: dateProvider, dispatchQueueWrapper: SentryDispatchQueueWrapper(), keepDelayedFramesDuration: 0)
             SentryDependencyContainer.sharedInstance().framesTracker = framesTracker
             framesTracker.start()
         }

diff --git a/Tests/SentryTests/Transaction/SentrySpanTests.swift b/Tests/SentryTests/Transaction/SentrySpanTests.swift
@@ -541,7 +541,7 @@ class SentrySpanTests: XCTestCase {
 
     private func givenFramesTracker() -> (TestDisplayLinkWrapper, SentryFramesTracker) {
         let displayLinkWrapper = TestDisplayLinkWrapper(dateProvider: self.fixture.currentDateProvider)
-        let framesTracker = SentryFramesTracker(displayLinkWrapper: displayLinkWrapper, dateProvider: self.fixture.currentDateProvider, keepDelayedFramesDuration: 10)
+        let framesTracker = SentryFramesTracker(displayLinkWrapper: displayLinkWrapper, dateProvider: self.fixture.currentDateProvider, dispatchQueueWrapper: SentryDispatchQueueWrapper(), keepDelayedFramesDuration: 10)
         framesTracker.start()
         displayLinkWrapper.call()