From cd53084d104d3b6eb8eb33330ed1e2a28eeb4d3f Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Fri, 6 Oct 2023 09:12:34 +0200 Subject: [PATCH 1/3] fix: Crash in SentryTracer when cancelling timer The SentryTracer starts the timer for the deadline on the main thread and invalidates the timer, not necessarily on the main thread, but invalidate has to be called on the same thread on which the timer was started. This is fixed by starting and invalidating the NSTimer on the main thread. Fixes GH-3320 --- Sources/Sentry/SentryTracer.m | 15 +++++-- .../Transaction/SentryTracerTests.swift | 41 +++++++++++++++++++ 2 files changed, 53 insertions(+), 3 deletions(-) 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() From 509a8b045e88863a87ec5063574a259e4f48d468 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Fri, 6 Oct 2023 09:16:09 +0200 Subject: [PATCH 2/3] add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 643ca3d5459..325eba05006 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 unnecessaries build settings (#3325) +- Crash in SentryTracer when cancelling timer #3333 ## 8.13.0 From 5326485eedf75c2d37fbc252cc35cfa18cebe34c Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Fri, 6 Oct 2023 09:48:49 +0200 Subject: [PATCH 3/3] fix failing test --- SentryTestUtils/TestSentryDispatchQueueWrapper.swift | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) 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() }