From e4c5e035530d1f67f3be4f66f561944a632d8e92 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Thu, 1 Aug 2024 15:34:33 +0200 Subject: [PATCH] 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. --- SentryTestUtils/TestDisplayLinkWrapper.swift | 9 ++-- .../TestNSNotificationCenterWrapper.swift | 5 +- Sources/Sentry/SentryFramesTracker.m | 15 ++++++ .../SentryFramesTrackerTests.swift | 46 +++++++++++++++---- 4 files changed, 61 insertions(+), 14 deletions(-) diff --git a/SentryTestUtils/TestDisplayLinkWrapper.swift b/SentryTestUtils/TestDisplayLinkWrapper.swift index 3f71d3dc6d2..e0528bb4843 100644 --- 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() 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 index 79510b4873b..8a03b2c709e 100644 --- a/SentryTestUtils/TestNSNotificationCenterWrapper.swift +++ b/SentryTestUtils/TestNSNotificationCenterWrapper.swift @@ -4,6 +4,7 @@ import Sentry @objcMembers public class TestNSNotificationCenterWrapper: SentryNSNotificationCenterWrapper { public var ignoreRemoveObserver = false + public var ignoreAddObserver = false public var addObserverInvocations = Invocations<(observer: WeakReference, selector: Selector, name: NSNotification.Name)>() public var addObserverInvocationsCount: Int { @@ -11,7 +12,9 @@ import Sentry } 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 index b9b0123e257..6c5908bf16c 100644 --- 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 index 427387dda48..2c7774fd9b1 100644 --- a/Tests/SentryTests/Integrations/Performance/FramesTracking/SentryFramesTrackerTests.swift +++ b/Tests/SentryTests/Integrations/Performance/FramesTracking/SentryFramesTrackerTests.swift @@ -4,9 +4,9 @@ import XCTest #if os(iOS) || os(tvOS) || targetEnvironment(macCatalyst) class SentryFramesTrackerTests: XCTestCase { - + private class Fixture { - + var displayLinkWrapper: TestDisplayLinkWrapper var queue: DispatchQueue var dateProvider = TestCurrentDateProvider() @@ -14,28 +14,28 @@ class SentryFramesTrackerTests: XCTestCase { 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()