Merge 5326485 into c2acec5

getsentry · Oct 6, 2023 · b7fd8c4 · b7fd8c4
2 parents c2acec5 + 5326485
commit b7fd8c4
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 7 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,7 @@
 - App hang with race condition for tick counter (#3290)
 - Remove "duplicate library" warning (#3312)
 - Remove unnecessaries build settings (#3325)
+- Crash in SentryTracer when cancelling timer #3333
 
 ## 8.13.0
 

diff --git a/SentryTestUtils/TestSentryDispatchQueueWrapper.swift b/SentryTestUtils/TestSentryDispatchQueueWrapper.swift
@@ -23,25 +23,27 @@ public class TestSentryDispatchQueueWrapper: SentryDispatchQueueWrapper {
         dispatchAsyncInvocations.invocations.last?()
     }
 
-    public var blockOnMainInvocations = Invocations<() -> Void>()
+    // Keeping track of the blocks can lead to memory leaks. Therefore,
+    // we only keep track of the number of invocations.
+    public var blockOnMainInvocations = Invocations<Void>()
     public var blockBeforeMainBlock: () -> Bool = { true }
 
     public override func dispatch(onMainQueue block: @escaping () -> Void) {
-        blockOnMainInvocations.record(block)
+        blockOnMainInvocations.record(Void())
         if blockBeforeMainBlock() {
             block()
         }
     }
 
     public override func dispatchAsync(onMainQueue block: @escaping () -> Void) {
-        blockOnMainInvocations.record(block)
+        blockOnMainInvocations.record(Void())
         if blockBeforeMainBlock() {
             block()
         }
     }
 
     public override func dispatchSync(onMainQueue block: @escaping () -> Void) {
-        blockOnMainInvocations.record(block)
+        blockOnMainInvocations.record(Void())
         if blockBeforeMainBlock() {
             block()
         }

diff --git a/Sources/Sentry/SentryTracer.m b/Sources/Sentry/SentryTracer.m
@@ -235,11 +235,16 @@ - (void)cancelIdleTimeout
 - (void)startDeadlineTimer
 {
     __weak SentryTracer *weakSelf = self;
-    [SentryThreadWrapper onMainThread:^{
+    [_configuration.dispatchQueueWrapper dispatchOnMainQueue:^{
         weakSelf.deadlineTimer = [weakSelf.configuration.timerFactory
             scheduledTimerWithTimeInterval:SENTRY_AUTO_TRANSACTION_DEADLINE
                                    repeats:NO
                                      block:^(NSTimer *_Nonnull timer) {
+                                         if (weakSelf == nil) {
+                                             SENTRY_LOG_DEBUG(@"WeakSelf is nil. Not calling "
+                                                              @"deadlineTimerFired.");
+                                             return;
+                                         }
                                          [weakSelf deadlineTimerFired];
                                      }];
     }];
@@ -267,8 +272,12 @@ - (void)deadlineTimerFired
 
 - (void)cancelDeadlineTimer
 {
-    [self.deadlineTimer invalidate];
-    self.deadlineTimer = nil;
+    // The timer must be invalidated from the thread on which the timer was installed, see
+    // https://developer.apple.com/documentation/foundation/nstimer/1415405-invalidate#1770468
+    [_configuration.dispatchQueueWrapper dispatchOnMainQueue:^{
+        [self.deadlineTimer invalidate];
+        self.deadlineTimer = nil;
+    }];
 }
 
 - (id<SentrySpan>)getActiveSpan

diff --git a/Tests/SentryTests/Transaction/SentryTracerTests.swift b/Tests/SentryTests/Transaction/SentryTracerTests.swift
@@ -93,6 +93,7 @@ class SentryTracerTests: XCTestCase {
                 customSamplingContext: [:],
                 configuration: SentryTracerConfiguration(block: {
                     $0.waitForChildren = waitForChildren
+                    $0.dispatchQueueWrapper = self.dispatchQueue
                     $0.timerFactory = self.timerFactory
                 }))
             return tracer
@@ -160,7 +161,9 @@ class SentryTracerTests: XCTestCase {
     }
 
     func testDeadlineTimer_FinishesTransactionAndChildren() {
+        fixture.dispatchQueue.blockBeforeMainBlock = { true }
         let sut = fixture.getSut()
+
         let child1 = sut.startChild(operation: fixture.transactionOperation)
         let child2 = sut.startChild(operation: fixture.transactionOperation)
         let child3 = sut.startChild(operation: fixture.transactionOperation)
@@ -176,6 +179,44 @@ class SentryTracerTests: XCTestCase {
         XCTAssertEqual(child2.status, .deadlineExceeded)
         XCTAssertEqual(child3.status, .ok)
     }
+
+    func testDeadlineTimer_StartedAndCancelledOnMainThread() {
+        fixture.dispatchQueue.blockBeforeMainBlock = { true }
+
+        let sut = fixture.getSut()
+        let child1 = sut.startChild(operation: fixture.transactionOperation)
+
+        fixture.timerFactory.fire()
+
+        XCTAssertEqual(sut.status, .deadlineExceeded)
+        XCTAssertEqual(child1.status, .deadlineExceeded)
+        XCTAssertEqual(2, fixture.dispatchQueue.blockOnMainInvocations.count, "The NSTimer must be started and cancelled on the main thread.")
+    }
+
+    func testDeadlineTimer_WhenCancelling_IsInvalidated() {
+        fixture.dispatchQueue.blockBeforeMainBlock = { true }
+
+        let sut = fixture.getSut()
+        let timer: Timer? = Dynamic(sut).deadlineTimer
+        _ = sut.startChild(operation: fixture.transactionOperation)
+
+        fixture.timerFactory.fire()
+
+        XCTAssertNil(Dynamic(sut).deadlineTimer.asObject, "DeadlineTimer should be nil.")
+        XCTAssertFalse(timer?.isValid ?? true)
+    }
+
+    func testDeadlineTimer_FiresAfterTracerDeallocated() {
+        fixture.dispatchQueue.blockBeforeMainBlock = { true }
+
+        // Added internal function so the tracer gets deallocated after executing this function.
+        func startTracer() {
+            _ = fixture.getSut()
+        }
+        startTracer()
+
+        fixture.timerFactory.fire()
+    }
 
     func testFramesofSpans_SetsDebugMeta() {
         let sut = fixture.getSut()