fix: Race condition in ANRTrackerV1 (getsentry#5137)

Use an atomic_bool to fix race conditions for accessing reported in the
ANRTrackerV1.

Fixes getsentryGH-2508

Author: philipphofmann

CHANGELOG.md

@@ -6,6 +6,10 @@
 
 - Added ability to bring your own button for user feedback form display (#5107)
 
+### Fixes
+
+- Race condition in ANRTrackerV1 (#5137)
+
 ### Improvements
 
 - More logging for Session Replay video info (#5132)

Sentry.xcodeproj/project.pbxproj

@@ -89,6 +89,7 @@
 		6205CF262D549D8A001E6049 /* SentryDateCodableTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6205CF252D549D8A001E6049 /* SentryDateCodableTests.swift */; };
 		621AE74B2C626C230012E730 /* SentryANRTrackerV2.h in Headers */ = {isa = PBXBuildFile; fileRef = 621AE74A2C626C230012E730 /* SentryANRTrackerV2.h */; };
 		621AE74D2C626C510012E730 /* SentryANRTrackerV2.m in Sources */ = {isa = PBXBuildFile; fileRef = 621AE74C2C626C510012E730 /* SentryANRTrackerV2.m */; };
+		621D22012DBB7E09006F9C48 /* SentryANRTrackerV1IntegrationTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 621D22002DBB7E09006F9C48 /* SentryANRTrackerV1IntegrationTests.swift */; };
 		621D9F2F2B9B0320003D94DE /* SentryCurrentDateProvider.swift in Sources */ = {isa = PBXBuildFile; fileRef = 621D9F2E2B9B0320003D94DE /* SentryCurrentDateProvider.swift */; };
 		621F61F12BEA073A005E654F /* SentryEnabledFeaturesBuilder.swift in Sources */ = {isa = PBXBuildFile; fileRef = 621F61F02BEA073A005E654F /* SentryEnabledFeaturesBuilder.swift */; };
 		62212B872D520CB00062C2FA /* SentryEventCodable.swift in Sources */ = {isa = PBXBuildFile; fileRef = 62212B862D520CB00062C2FA /* SentryEventCodable.swift */; };
@@ -1173,6 +1174,7 @@
 		621AE74A2C626C230012E730 /* SentryANRTrackerV2.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SentryANRTrackerV2.h; path = include/SentryANRTrackerV2.h; sourceTree = "<group>"; };
 		621AE74C2C626C510012E730 /* SentryANRTrackerV2.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryANRTrackerV2.m; sourceTree = "<group>"; };
 		621AE74E2C626CF70012E730 /* SentryANRTrackerV2Tests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryANRTrackerV2Tests.swift; sourceTree = "<group>"; };
+		621D22002DBB7E09006F9C48 /* SentryANRTrackerV1IntegrationTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryANRTrackerV1IntegrationTests.swift; sourceTree = "<group>"; };
 		621D9F2E2B9B0320003D94DE /* SentryCurrentDateProvider.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryCurrentDateProvider.swift; sourceTree = "<group>"; };
 		621F61F02BEA073A005E654F /* SentryEnabledFeaturesBuilder.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryEnabledFeaturesBuilder.swift; sourceTree = "<group>"; };
 		62212B862D520CB00062C2FA /* SentryEventCodable.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryEventCodable.swift; sourceTree = "<group>"; };
@@ -3095,6 +3097,7 @@
 			isa = PBXGroup;
 			children = (
 				7B2A70D727D5F07F008B0D15 /* SentryANRTrackerV1Tests.swift */,
+				621D22002DBB7E09006F9C48 /* SentryANRTrackerV1IntegrationTests.swift */,
 				621AE74E2C626CF70012E730 /* SentryANRTrackerV2Tests.swift */,
 				7BFA69F527E0840400233199 /* SentryANRTrackingIntegrationTests.swift */,
 				623E16C22D6C57C000CF1625 /* SentryANRTypeTests.swift */,
@@ -5523,6 +5526,7 @@
 				84EB21962BF01CEA00EDDA28 /* SentryCrashInstallationTests.swift in Sources */,
 				7BFE7A0A27A1B6B000D2B66E /* SentryWatchdogTerminationsIntegrationTests.swift in Sources */,
 				D8292D7D2A39A027009872F7 /* UrlSanitizedTests.swift in Sources */,
+				621D22012DBB7E09006F9C48 /* SentryANRTrackerV1IntegrationTests.swift in Sources */,
 				7BA61CAF247BBF3C00C130A8 /* SentryDebugImageProviderTests.swift in Sources */,
 				7BB7E7C729267A28004BF96B /* EmptyIntegration.swift in Sources */,
 				7B965728268321CD00C66E25 /* SentryCrashScopeObserverTests.swift in Sources */,

Sources/Sentry/SentryANRTrackerV1.m

@@ -65,7 +65,7 @@ - (void)detectANRs
     }
 
     __block atomic_int ticksSinceUiUpdate = 0;
-    __block BOOL reported = NO;
+    __block atomic_bool reported = false;
 
     NSInteger reportThreshold = 5;
     NSTimeInterval sleepInterval = self.timeoutInterval / reportThreshold;
@@ -88,7 +88,8 @@ - (void)detectANRs
         [self.dispatchQueueWrapper dispatchAsyncOnMainQueue:^{
             atomic_store_explicit(&ticksSinceUiUpdate, 0, memory_order_relaxed);
 
-            if (reported) {
+            bool isReported = atomic_load_explicit(&reported, memory_order_relaxed);
+            if (isReported) {
                 SENTRY_LOG_WARN(@"ANR stopped.");
 
                 // The ANR stopped, don't block the main thread with calling ANRStopped listeners.
@@ -99,7 +100,7 @@ - (void)detectANRs
                 [self.dispatchQueueWrapper dispatchAsyncWithBlock:^{ [self ANRStopped]; }];
             }
 
-            reported = NO;
+            atomic_store_explicit(&reported, false, memory_order_relaxed);
         }];
 
         [self.threadWrapper sleepForTimeInterval:sleepInterval];
@@ -116,9 +117,12 @@ - (void)detectANRs
             continue;
         }
 
-        if (atomic_load_explicit(&ticksSinceUiUpdate, memory_order_relaxed) >= reportThreshold
-            && !reported) {
-            reported = YES;
+        bool isReported = atomic_load_explicit(&reported, memory_order_relaxed);
+        int currentTicks = atomic_load_explicit(&ticksSinceUiUpdate, memory_order_relaxed);
+
+        if (currentTicks >= reportThreshold && !isReported) {
+
+            atomic_store_explicit(&reported, true, memory_order_relaxed);
 
             if (![self.crashWrapper isApplicationInForeground]) {
                 SENTRY_LOG_DEBUG(@"Ignoring ANR because the app is in the background");

Tests/SentryTests/Integrations/ANR/SentryANRTrackerV1IntegrationTests.swift

@@ -0,0 +1,32 @@
+@testable import Sentry
+import XCTest
+
+#if os(iOS) || os(tvOS) || targetEnvironment(macCatalyst)
+
+final class SentryANRTrackerV1IntegrationTests: XCTestCase {
+
+    /// This uses an actual dispatch queue and a thread wrapper so the thread sanitizer can find threading problems in the implementation.
+    /// It has no assertions on purpose. To test the thread safety locally, enable the thread sanitizer in the test plan.
+    func testWithActualDispatchQueueAndThreadWrapper() {
+        let expectation = XCTestExpectation(description: "ANR Tracker")
+
+        let listener = SentryANRTrackerTestDelegate()
+
+        let anrTracker: SentryANRTracker = SentryANRTrackerV1(
+            timeoutInterval: 0.01,
+            crashWrapper: TestSentryCrashWrapper.sharedInstance(),
+            dispatchQueueWrapper: SentryDispatchQueueWrapper(),
+            threadWrapper: SentryThreadWrapper())
+
+        anrTracker.add(listener: listener)
+
+        DispatchQueue.global().asyncAfter(deadline: .now() + 2.0) {
+            anrTracker.clear()
+            expectation.fulfill()
+        }
+
+        wait(for: [expectation], timeout: 5.0)
+    }
+}
+
+#endif // os(iOS) || os(tvOS) || targetEnvironment(macCatalyst)

Tests/SentryTests/Integrations/ANR/SentryANRTrackerV1Tests.swift

@@ -208,7 +208,7 @@ class SentryANRTrackerV1Tests: XCTestCase, SentryANRTrackerDelegate {
         // and finish multiple times. Most importantly, the code covers every start with one finish.
         XCTAssertEqual(fixture.threadWrapper.threadStartedInvocations.count, fixture.threadWrapper.threadFinishedInvocations.count, "The number of started and finished threads should be equal, otherwise the ANR tracker could run.")
     }
-    
+
     // swiftlint:disable test_case_accessibility
     // Protocl implementation can't be private