ref: Properly teardown frames tracker (#3545)

philipphofmann · web-flow · commit 1734d1b5afa9 · 2024-01-05T14:18:38.000+01:00
The teardown of the SentryFramesTracker didn't work properly in tests.
The clearTestState method continuously initialized a new frames tracker
cause it accessed the property dependency container. This is fixed now
by calling SentryFramesTracker.stop directly in
SentryDependencyContainer.reset which is also called in clearTestState.
Furthermore, the SentryFramesTracker.stop now resets all frames and
removes all observers.
diff --git a/Sentry.xcodeproj/project.pbxproj b/Sentry.xcodeproj/project.pbxproj
@@ -79,6 +79,7 @@
 		620379DD2AFE1432005AC0C1 /* SentryBuildAppStartSpans.m in Sources */ = {isa = PBXBuildFile; fileRef = 620379DC2AFE1432005AC0C1 /* SentryBuildAppStartSpans.m */; };
 		622C08D829E546F4002571D4 /* SentryTraceOrigins.h in Headers */ = {isa = PBXBuildFile; fileRef = 622C08D729E546F4002571D4 /* SentryTraceOrigins.h */; };
 		622C08DB29E554B9002571D4 /* SentrySpanContext+Private.h in Headers */ = {isa = PBXBuildFile; fileRef = 622C08D929E554B9002571D4 /* SentrySpanContext+Private.h */; };
+		62375FB92B47F9F000CC55F1 /* SentryDependencyContainerTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 62375FB82B47F9F000CC55F1 /* SentryDependencyContainerTests.swift */; };
 		623C45B02A651D8200D9E88B /* SentryCoreDataTracker+Test.m in Sources */ = {isa = PBXBuildFile; fileRef = 623C45AF2A651D8200D9E88B /* SentryCoreDataTracker+Test.m */; };
 		627E7589299F6FE40085504D /* SentryInternalDefines.h in Headers */ = {isa = PBXBuildFile; fileRef = 627E7588299F6FE40085504D /* SentryInternalDefines.h */; };
 		62862B1C2B1DDBC8009B16E3 /* SentryDelayedFrame.h in Headers */ = {isa = PBXBuildFile; fileRef = 62862B1B2B1DDBC8009B16E3 /* SentryDelayedFrame.h */; };
@@ -92,9 +93,9 @@
 		62A456E52B0370E0003F19A1 /* SentryUIEventTrackerTransactionMode.m in Sources */ = {isa = PBXBuildFile; fileRef = 62A456E42B0370E0003F19A1 /* SentryUIEventTrackerTransactionMode.m */; };
 		62B86CFC29F052BB008F3947 /* SentryTestLogConfig.m in Sources */ = {isa = PBXBuildFile; fileRef = 62B86CFB29F052BB008F3947 /* SentryTestLogConfig.m */; };
 		62C25C862B075F4900C68CBD /* TestOptions.swift in Sources */ = {isa = PBXBuildFile; fileRef = 62C25C852B075F4900C68CBD /* TestOptions.swift */; };
-		62C3168B2B1F865A000D7031 /* SentryTimeSwiftTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 62C3168A2B1F865A000D7031 /* SentryTimeSwiftTests.swift */; };
 		62C316812B1F2E93000D7031 /* SentryDelayedFramesTracker.h in Headers */ = {isa = PBXBuildFile; fileRef = 62C316802B1F2E93000D7031 /* SentryDelayedFramesTracker.h */; };
 		62C316832B1F2EA1000D7031 /* SentryDelayedFramesTracker.m in Sources */ = {isa = PBXBuildFile; fileRef = 62C316822B1F2EA1000D7031 /* SentryDelayedFramesTracker.m */; };
+		62C3168B2B1F865A000D7031 /* SentryTimeSwiftTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 62C3168A2B1F865A000D7031 /* SentryTimeSwiftTests.swift */; };
 		62E081A929ED4260000F69FC /* SentryBreadcrumbDelegate.h in Headers */ = {isa = PBXBuildFile; fileRef = 62E081A829ED4260000F69FC /* SentryBreadcrumbDelegate.h */; };
 		62E081AB29ED4322000F69FC /* SentryBreadcrumbTestDelegate.swift in Sources */ = {isa = PBXBuildFile; fileRef = 62E081AA29ED4322000F69FC /* SentryBreadcrumbTestDelegate.swift */; };
 		62F226B729A37C120038080D /* SentryBooleanSerialization.m in Sources */ = {isa = PBXBuildFile; fileRef = 62F226B629A37C120038080D /* SentryBooleanSerialization.m */; };
@@ -996,6 +997,7 @@
 		620379DC2AFE1432005AC0C1 /* SentryBuildAppStartSpans.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryBuildAppStartSpans.m; sourceTree = "<group>"; };
 		622C08D729E546F4002571D4 /* SentryTraceOrigins.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SentryTraceOrigins.h; path = include/SentryTraceOrigins.h; sourceTree = "<group>"; };
 		622C08D929E554B9002571D4 /* SentrySpanContext+Private.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = "SentrySpanContext+Private.h"; path = "include/SentrySpanContext+Private.h"; sourceTree = "<group>"; };
+		62375FB82B47F9F000CC55F1 /* SentryDependencyContainerTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryDependencyContainerTests.swift; sourceTree = "<group>"; };
 		623C45AE2A651C4500D9E88B /* SentryCoreDataTracker+Test.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = "SentryCoreDataTracker+Test.h"; sourceTree = "<group>"; };
 		623C45AF2A651D8200D9E88B /* SentryCoreDataTracker+Test.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = "SentryCoreDataTracker+Test.m"; sourceTree = "<group>"; };
 		627E7588299F6FE40085504D /* SentryInternalDefines.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SentryInternalDefines.h; path = include/SentryInternalDefines.h; sourceTree = "<group>"; };
@@ -1009,9 +1011,9 @@
 		62A456E42B0370E0003F19A1 /* SentryUIEventTrackerTransactionMode.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryUIEventTrackerTransactionMode.m; sourceTree = "<group>"; };
 		62B86CFB29F052BB008F3947 /* SentryTestLogConfig.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryTestLogConfig.m; sourceTree = "<group>"; };
 		62C25C852B075F4900C68CBD /* TestOptions.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TestOptions.swift; sourceTree = "<group>"; };
-		62C3168A2B1F865A000D7031 /* SentryTimeSwiftTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryTimeSwiftTests.swift; sourceTree = "<group>"; };
 		62C316802B1F2E93000D7031 /* SentryDelayedFramesTracker.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SentryDelayedFramesTracker.h; path = include/SentryDelayedFramesTracker.h; sourceTree = "<group>"; };
 		62C316822B1F2EA1000D7031 /* SentryDelayedFramesTracker.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryDelayedFramesTracker.m; sourceTree = "<group>"; };
+		62C3168A2B1F865A000D7031 /* SentryTimeSwiftTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryTimeSwiftTests.swift; sourceTree = "<group>"; };
 		62E081A829ED4260000F69FC /* SentryBreadcrumbDelegate.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SentryBreadcrumbDelegate.h; path = include/SentryBreadcrumbDelegate.h; sourceTree = "<group>"; };
 		62E081AA29ED4322000F69FC /* SentryBreadcrumbTestDelegate.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryBreadcrumbTestDelegate.swift; sourceTree = "<group>"; };
 		62F226B629A37C120038080D /* SentryBooleanSerialization.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryBooleanSerialization.m; sourceTree = "<group>"; };
@@ -2866,6 +2868,7 @@
 				8431EE5A29ADB8EA00D8DC56 /* SentryTimeTests.m */,
 				62C3168A2B1F865A000D7031 /* SentryTimeSwiftTests.swift */,
 				33042A1629DC2C4300C60085 /* SentryExtraContextProviderTests.swift */,
+				62375FB82B47F9F000CC55F1 /* SentryDependencyContainerTests.swift */,
 			);
 			path = Helper;
 			sourceTree = "<group>";
@@ -4460,6 +4463,7 @@
 				7B18DE4A28DA0C8B004845C6 /* SentryNSNotificationCenterWrapperTests.swift in Sources */,
 				7BEFB044270B0F630025F808 /* SentryTracerObjCTests.m in Sources */,
 				7B0002322477F0520035FEF1 /* SentrySessionTests.m in Sources */,
+				62375FB92B47F9F000CC55F1 /* SentryDependencyContainerTests.swift in Sources */,
 				7BC6EC08255C36DE0059822A /* SentryStacktraceTests.swift in Sources */,
 				D88817DD26D72BA500BF2251 /* SentryTraceStateTests.swift in Sources */,
 				7B26BBFB24C0A66D00A79CCC /* SentrySdkInfoNilTests.m in Sources */,
diff --git a/SentryTestUtils/ClearTestState.swift b/SentryTestUtils/ClearTestState.swift
@@ -26,9 +26,6 @@ class TestCleanup: NSObject {
         setTestDefaultLogLevel()
 
         #if os(iOS) || os(tvOS) || targetEnvironment(macCatalyst)
-        let framesTracker = SentryDependencyContainer.sharedInstance().framesTracker
-        framesTracker.stop()
-        framesTracker.resetFrames()
 
         setenv("ActivePrewarm", "0", 1)
         SentryAppStartTracker.load()
diff --git a/Sources/Sentry/SentryDependencyContainer.m b/Sources/Sentry/SentryDependencyContainer.m
@@ -63,12 +63,16 @@ + (instancetype)sharedInstance
 
 + (void)reset
 {
-#if !TARGET_OS_WATCH
     if (instance) {
+#if !TARGET_OS_WATCH
         [instance->_reachability removeAllObservers];
-    }
 #endif // !TARGET_OS_WATCH
 
+#if SENTRY_HAS_UIKIT
+        [instance->_framesTracker stop];
+#endif // SENTRY_HAS_UIKIT
+    }
+
     instance = [[SentryDependencyContainer alloc] init];
 }
 
diff --git a/Sources/Sentry/SentryFramesTracker.m b/Sources/Sentry/SentryFramesTracker.m
@@ -83,7 +83,6 @@ - (void)setDisplayLinkWrapper:(SentryDisplayLinkWrapper *)displayLinkWrapper
     _displayLinkWrapper = displayLinkWrapper;
 }
 
-/** Internal for testing */
 - (void)resetFrames
 {
     _totalFrames = 0;
@@ -277,6 +276,9 @@ - (void)stop
     _isRunning = NO;
     [self.displayLinkWrapper invalidate];
     [self.delayedFramesTracker resetDelayedFramesTimeStamps];
+    @synchronized(self.listeners) {
+        [self.listeners removeAllObjects];
+    }
 }
 
 - (void)dealloc
diff --git a/Tests/SentryTests/Helper/SentryDependencyContainerTests.swift b/Tests/SentryTests/Helper/SentryDependencyContainerTests.swift
@@ -0,0 +1,15 @@
+import Nimble
+import XCTest
+
+#if os(iOS) || os(tvOS) || targetEnvironment(macCatalyst)
+final class SentryDependencyContainerTests: XCTestCase {
+    
+    func testReset_CallsFramesTrackerStop() throws {
+        let framesTracker = SentryDependencyContainer.sharedInstance().framesTracker
+        framesTracker.start()
+        SentryDependencyContainer.reset()
+        
+        expect(framesTracker.isRunning) == false
+    }
+}
+#endif
diff --git a/Tests/SentryTests/Integrations/Performance/FramesTracking/SentryFramesTrackerTests.swift b/Tests/SentryTests/Integrations/Performance/FramesTracking/SentryFramesTrackerTests.swift
@@ -69,6 +69,19 @@ class SentryFramesTrackerTests: XCTestCase {
         expect(self.fixture.sut.isRunning) == false
     }
     
+    func testKeepFrames_WhenStopped() throws {
+        let sut = fixture.sut
+        sut.start()
+        
+        let displayLink = fixture.displayLinkWrapper
+        displayLink.call()
+        displayLink.normalFrame()
+        
+        sut.stop()
+        
+        try assert(slow: 0, frozen: 0, total: 1)
+    }
+    
     func testStartAfterStopped_SubscribesTwiceToDisplayLink() {
         let sut = fixture.sut
         sut.start()
@@ -550,6 +563,22 @@ class SentryFramesTrackerTests: XCTestCase {
 
         expect(listener.newFrameInvocations.count) == 0
     }
+    
+    func testListenerNotCalledAfterCallingStop() {
+        let sut = fixture.sut
+        let listener1 = FrameTrackerListener()
+        let listener2 = FrameTrackerListener()
+        sut.start()
+        sut.add(listener1)
+        sut.stop()
+        sut.start()
+        sut.add(listener2)
+
+        fixture.displayLinkWrapper.normalFrame()
+
+        expect(listener1.newFrameInvocations.count) == 0
+        expect(listener2.newFrameInvocations.count) == 1
+    }
 
     func testReleasedListener() {
         let sut = fixture.sut
@@ -617,13 +646,13 @@ private extension SentryFramesTrackerTests {
     func assert(slow: UInt? = nil, frozen: UInt? = nil, total: UInt? = nil, frameRates: UInt? = nil) throws {
         let currentFrames = fixture.sut.currentFrames
         if let total = total {
-            XCTAssertEqual(total, currentFrames.total)
+            expect(currentFrames.total) == total
         }
         if let slow = slow {
-            XCTAssertEqual(slow, currentFrames.slow)
+            expect(currentFrames.slow) == slow
         }
         if let frozen = frozen {
-            XCTAssertEqual(frozen, currentFrames.frozen)
+            expect(currentFrames.frozen) == frozen
         }
 
 #if os(iOS) || os(macOS) || targetEnvironment(macCatalyst)

Original file line number	Diff line number	Diff line change
`@@ -83,7 +83,6 @@ - (void)setDisplayLinkWrapper:(SentryDisplayLinkWrapper *)displayLinkWrapper`
`83`	`83`	`_displayLinkWrapper = displayLinkWrapper;`
`84`	`84`	`}`
`85`	`85`
`86`		`-/** Internal for testing */`
`87`	`86`	`- (void)resetFrames`
`88`	`87`	`{`
`89`	`88`	`_totalFrames = 0;`
`@@ -277,6 +276,9 @@ - (void)stop`
`277`	`276`	`_isRunning = NO;`
`278`	`277`	`[self.displayLinkWrapper invalidate];`
`279`	`278`	`[self.delayedFramesTracker resetDelayedFramesTimeStamps];`
	`279`	`+ @synchronized(self.listeners) {`
	`280`	`+ [self.listeners removeAllObjects];`
	`281`	`+ }`
`280`	`282`	`}`
`281`	`283`
`282`	`284`	`- (void)dealloc`