From 2384e6d10dc5a356180c66b6a3dbe6ee6e5958d8 Mon Sep 17 00:00:00 2001
From: Philipp Hofmann <philipp.hofmann@sentry.io>
Date: Wed, 3 Apr 2024 12:57:18 +0200
Subject: [PATCH] fix: Crash in hasUnfinishedChildSpansToWaitFor (#3821)

Fix a crash in SentryTracer hasUnfinishedChildSpansToWaitFor by not
setting the shouldIgnoreWaitForChildrenCallback to nil when finishing
the tracer.

Fixes GH-3781
---
 CHANGELOG.md                                  |  1 +
 Sources/Sentry/SentryTracer.m                 |  4 --
 .../Transaction/SentryTracerTests.swift       | 42 ++++++++++++++++++-
 3 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 15614d95e09..8b9f765e3b1 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -17,6 +17,7 @@ more about how to use the Metrics API.
 - Fixed certain views getting loaded twice when adding a child view controller (#3753)
 - Fix NSInvalidArgumentException for `NSError sentryErrorWithDomain` (#3819)
 - Again fix runtime error when including Sentry as a static lib (#3820)
+- Fix crash in hasUnfinishedChildSpansToWaitFor (#3821)
 
 ## 8.22.4
 
diff --git a/Sources/Sentry/SentryTracer.m b/Sources/Sentry/SentryTracer.m
index 0d3627f92a0..b8714fb649b 100644
--- a/Sources/Sentry/SentryTracer.m
+++ b/Sources/Sentry/SentryTracer.m
@@ -547,10 +547,6 @@ - (void)finishInternal
         self.finishCallback = nil;
     }
 
-    if (self.shouldIgnoreWaitForChildrenCallback) {
-        self.shouldIgnoreWaitForChildrenCallback = nil;
-    }
-
     // Prewarming can execute code up to viewDidLoad of a UIViewController, and keep the app in the
     // background. This can lead to auto-generated transactions lasting for minutes or even hours.
     // Therefore, we drop transactions lasting longer than SENTRY_AUTO_TRANSACTION_MAX_DURATION.
diff --git a/Tests/SentryTests/Transaction/SentryTracerTests.swift b/Tests/SentryTests/Transaction/SentryTracerTests.swift
index cc0192b17fa..8a8237e1d89 100644
--- a/Tests/SentryTests/Transaction/SentryTracerTests.swift
+++ b/Tests/SentryTests/Transaction/SentryTracerTests.swift
@@ -178,7 +178,47 @@ class SentryTracerTests: XCTestCase {
         let span = try XCTUnwrap(spans.first, "Expected first span not to be nil")
         expect(span["timestamp"] as? TimeInterval) == tracerTimestamp.timeIntervalSince1970
         
-        expect(sut.shouldIgnoreWaitForChildrenCallback) == nil
+        expect(sut.shouldIgnoreWaitForChildrenCallback).toNot(beNil(), description: "We must not set the callback to nil because when iterating over the child spans in hasUnfinishedChildSpansToWaitFor this could lead to a crash when shouldIgnoreWaitForChildrenCallback is nil.")
+    }
+    
+    /// Reproduces a crash in hasUnfinishedChildSpansToWaitFor; see https://github.com/getsentry/sentry-cocoa/issues/3781
+    /// We used to set the shouldIgnoreWaitForChildrenCallback to nil in finishInternal, which can lead
+    /// to a crash when spans keep finishing while finishInternal is executed because
+    /// shouldIgnoreWaitForChildrenCallback could be then nil in hasUnfinishedChildSpansToWaitFor.
+    func testFinish_ShouldIgnoreWaitForChildrenCallback_DoesNotCrash() throws {
+        
+        for _ in 0..<5 {
+            let sut = fixture.getSut()
+            
+            let dispatchQueue = DispatchQueue(label: "test", attributes: [.concurrent, .initiallyInactive])
+            
+            let expectation = expectation(description: "call everything")
+            expectation.expectedFulfillmentCount = 11
+            
+            sut.shouldIgnoreWaitForChildrenCallback = { _ in
+                return true
+            }
+            
+            for _ in 0..<1_000 {
+                let child = sut.startChild(operation: self.fixture.transactionOperation)
+                child.finish()
+            }
+            
+            dispatchQueue.async {
+                for _ in 0..<10 {
+                    let child = sut.startChild(operation: self.fixture.transactionOperation)
+                    child.finish()
+                    expectation.fulfill()
+                }
+            }
+            dispatchQueue.async {
+                sut.finish()
+                expectation.fulfill()
+            }
+            
+            dispatchQueue.activate()
+            wait(for: [expectation], timeout: 1.0)
+        }
     }
 
     func testDeadlineTimer_FinishesTransactionAndChildren() {