diff --git a/CHANGELOG.md b/CHANGELOG.md index 24c51f2395f..4a5de5fdcf4 100644 --- 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 unnecessary build settings (#3325) +- Crash in SentryTracer when cancelling timer (#3333) ## 8.13.0 diff --git a/SentryTestUtils/TestSentryDispatchQueueWrapper.swift b/SentryTestUtils/TestSentryDispatchQueueWrapper.swift index 44d22183cf9..c9573f84517 100644 --- 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() 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 index 8f327550109..ddb44a581f9 100644 --- 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)getActiveSpan diff --git a/Tests/SentryTests/Transaction/SentryTracerTests.swift b/Tests/SentryTests/Transaction/SentryTracerTests.swift index e8c347daaba..b187cd979d8 100644 --- 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()