fix: Guard FramesTracker start and stop

Profiling starts the frames tracker if it's not running, but the frames tracker again subscribes to the didBecomeActive and willResignActive notifications. To fix this, the start operation is a NoOp now if the frames tracker has already started. The same applies to stop.
getsentry · Aug 1, 2024 · e4c5e03 · e4c5e03
1 parent 61249fb
commit e4c5e03
Show file tree

Hide file tree

Showing 4 changed files with 61 additions and 14 deletions.
diff --git a/SentryTestUtils/TestDisplayLinkWrapper.swift b/SentryTestUtils/TestDisplayLinkWrapper.swift
@@ -39,11 +39,14 @@ public class TestDisplayLinkWrapper: SentryDisplayLinkWrapper {
         fastestFrozenFrameDuration = frozenFrameThreshold + timeEpsilon
     }
 
+    public var ignoreLinkInvocations = false
     public var linkInvocations = Invocations<Void>()
     public override func link(withTarget target: Any, selector sel: Selector) {
-        linkInvocations.record(Void())
-        self.target = target as AnyObject
-        self.selector = sel
+        if ignoreLinkInvocations == false {
+            linkInvocations.record(Void())
+            self.target = target as AnyObject
+            self.selector = sel
+        }
     }
 
     public override var timestamp: CFTimeInterval {

diff --git a/SentryTestUtils/TestNSNotificationCenterWrapper.swift b/SentryTestUtils/TestNSNotificationCenterWrapper.swift
@@ -4,14 +4,17 @@ import Sentry
 @objcMembers public class TestNSNotificationCenterWrapper: SentryNSNotificationCenterWrapper {
 
     public var ignoreRemoveObserver = false
+    public var ignoreAddObserver = false
 
     public var addObserverInvocations = Invocations<(observer: WeakReference<NSObject>, selector: Selector, name: NSNotification.Name)>()
     public var addObserverInvocationsCount: Int {
         return addObserverInvocations.count
     }
 
     public override func addObserver(_ observer: NSObject, selector aSelector: Selector, name aName: NSNotification.Name) {
-        addObserverInvocations.record((WeakReference(value: observer), aSelector, aName))
+        if ignoreAddObserver == false {
+            addObserverInvocations.record((WeakReference(value: observer), aSelector, aName))
+        }
     }
 
     public var removeObserverWithNameInvocations = Invocations< NSNotification.Name>()

diff --git a/Sources/Sentry/SentryFramesTracker.m b/Sources/Sentry/SentryFramesTracker.m
@@ -31,6 +31,8 @@
 @interface
 SentryFramesTracker ()
 
+@property (nonatomic, assign, readonly) BOOL isStarted;
+
 @property (nonatomic, strong, readonly) SentryDisplayLinkWrapper *displayLinkWrapper;
 @property (nonatomic, strong, readonly) SentryCurrentDateProvider *dateProvider;
 @property (nonatomic, strong, readonly) SentryDispatchQueueWrapper *dispatchQueueWrapper;
@@ -71,6 +73,7 @@ - (instancetype)initWithDisplayLinkWrapper:(SentryDisplayLinkWrapper *)displayLi
 {
     if (self = [super init]) {
         _isRunning = NO;
+        _isStarted = NO;
         _displayLinkWrapper = displayLinkWrapper;
         _dateProvider = dateProvider;
         _dispatchQueueWrapper = dispatchQueueWrapper;
@@ -141,6 +144,12 @@ - (void)resetProfilingTimestampsInternal
 
 - (void)start
 {
+    if (_isStarted) {
+        return;
+    }
+
+    _isStarted = YES;
+
     [self.notificationCenter
         addObserver:self
            selector:@selector(didBecomeActive)
@@ -331,6 +340,12 @@ - (void)pause
 
 - (void)stop
 {
+    if (!_isStarted) {
+        return;
+    }
+
+    _isStarted = NO;
+
     [self pause];
     [self.delayedFramesTracker resetDelayedFramesTimeStamps];
 

diff --git a/Tests/SentryTests/Integrations/Performance/FramesTracking/SentryFramesTrackerTests.swift b/Tests/SentryTests/Integrations/Performance/FramesTracking/SentryFramesTrackerTests.swift
@@ -4,38 +4,38 @@ import XCTest
 
 #if os(iOS) || os(tvOS) || targetEnvironment(macCatalyst)
 class SentryFramesTrackerTests: XCTestCase {
-
+    
     private class Fixture {
-
+        
         var displayLinkWrapper: TestDisplayLinkWrapper
         var queue: DispatchQueue
         var dateProvider = TestCurrentDateProvider()
         var notificationCenter = TestNSNotificationCenterWrapper()
         let keepDelayedFramesDuration = 10.0
 
         let slowestSlowFrameDelay: Double
-
+        
         init() {
             displayLinkWrapper = TestDisplayLinkWrapper(dateProvider: dateProvider)
             queue = DispatchQueue(label: "SentryFramesTrackerTests", qos: .background, attributes: [.concurrent])
 
             slowestSlowFrameDelay = (displayLinkWrapper.slowestSlowFrameDuration - slowFrameThreshold(displayLinkWrapper.currentFrameRate.rawValue))
         }
-
+        
         lazy var sut: SentryFramesTracker = SentryFramesTracker(displayLinkWrapper: displayLinkWrapper, dateProvider: dateProvider, dispatchQueueWrapper: SentryDispatchQueueWrapper(), notificationCenter: notificationCenter, keepDelayedFramesDuration: keepDelayedFramesDuration)
     }
-
+    
     private var fixture: Fixture!
-
+    
     override func setUp() {
         super.setUp()
         fixture = Fixture()
     }
-
+    
     func testIsNotRunning_WhenNotStarted() {
         XCTAssertFalse(self.fixture.sut.isRunning)
     }
-
+    
     func testIsRunning_WhenStarted() {
         let sut = fixture.sut
         sut.start()
@@ -49,7 +49,15 @@ class SentryFramesTrackerTests: XCTestCase {
 
         XCTAssertEqual(self.fixture.displayLinkWrapper.linkInvocations.count, 1)
     }
-
+
+    func testStartTwice_SubscribesOnceToNotifications() {
+        let sut = fixture.sut
+        sut.start()
+        sut.start()
+
+        XCTAssertEqual(self.fixture.notificationCenter.addObserverInvocationsCount, 2)
+    }
+
     func testIsNotRunning_WhenStopped() {
         let sut = fixture.sut
         sut.start()
@@ -58,6 +66,15 @@ class SentryFramesTrackerTests: XCTestCase {
         XCTAssertFalse(self.fixture.sut.isRunning)
     }
 
+    func testWhenStoppedTwice_OnlyRemovesOnceFromNotifications() {
+        let sut = fixture.sut
+        sut.start()
+        sut.stop()
+        sut.stop()
+
+        XCTAssertEqual(self.fixture.notificationCenter.removeObserverWithNameInvocationsCount, 2)
+    }
+
     func testKeepFrames_WhenStopped() throws {
         let sut = fixture.sut
         sut.start()
@@ -587,7 +604,16 @@ class SentryFramesTrackerTests: XCTestCase {
 
     func testDealloc_CallsStop() {
         func sutIsDeallocatedAfterCallingMe() {
-            _ = SentryFramesTracker(displayLinkWrapper: fixture.displayLinkWrapper, dateProvider: fixture.dateProvider, dispatchQueueWrapper: SentryDispatchQueueWrapper(), notificationCenter: TestNSNotificationCenterWrapper(), keepDelayedFramesDuration: 0)
+
+            let notificationCenter = TestNSNotificationCenterWrapper()
+            notificationCenter.ignoreAddObserver = true
+
+            let displayLinkWrapper = fixture.displayLinkWrapper
+            displayLinkWrapper.ignoreLinkInvocations = true
+
+            let sut = SentryFramesTracker(displayLinkWrapper: displayLinkWrapper, dateProvider: fixture.dateProvider, dispatchQueueWrapper: SentryDispatchQueueWrapper(), notificationCenter: notificationCenter, keepDelayedFramesDuration: 0)
+
+            sut.start()
         }
         sutIsDeallocatedAfterCallingMe()