ref: Ignore more thread sanitizer warnings in code (#3923)

Ignore the thread sanitizer in the code where possible, which has the advantage of knowing a method is ignored when reading the code and not jumping to the ThreadSanitizer.sup file. Furthermore, use the ThreadSanitizer.sup file not only for tests but for the Run Xcode schemes.
getsentry · Apr 30, 2024 · 265f000 · 265f000
1 parent e887ddc
commit 265f000
Show file tree

Hide file tree

Showing 13 changed files with 99 additions and 53 deletions.
diff --git a/Sentry.xcodeproj/project.pbxproj b/Sentry.xcodeproj/project.pbxproj
@@ -2410,7 +2410,6 @@
 			isa = PBXGroup;
 			children = (
 				844DA7F6282435CD00E6B62E /* README.md */,
-				7B9660B12783500E0014A767 /* ThreadSanitizer.sup */,
 				630C01951EC341D600C52CEF /* Resources */,
 				63AA76AA1EB9D5CD00D153DE /* Configuration */,
 				63AA75931EB8AEDB00D153DE /* SentryTests */,
@@ -3728,6 +3727,7 @@
 		D8B0542F2A7D35F10056BAF6 /* Resources */ = {
 			isa = PBXGroup;
 			children = (
+				7B9660B12783500E0014A767 /* ThreadSanitizer.sup */,
 				D8B0542D2A7D2C720056BAF6 /* PrivacyInfo.xcprivacy */,
 				D8511F722BAC8F750015E6FD /* Sentry.modulemap */,
 			);

diff --git a/Sentry.xcodeproj/xcshareddata/xcschemes/Sentry.xcscheme b/Sentry.xcodeproj/xcshareddata/xcschemes/Sentry.xcscheme
@@ -40,7 +40,7 @@
       <EnvironmentVariables>
          <EnvironmentVariable
             key = "TSAN_OPTIONS"
-            value = "suppressions=$(PROJECT_DIR)/Tests/ThreadSanitizer.sup"
+            value = "suppressions=$(PROJECT_DIR)/Sources/Resources/ThreadSanitizer.sup"
             isEnabled = "YES">
          </EnvironmentVariable>
       </EnvironmentVariables>
@@ -159,6 +159,13 @@
             ReferencedContainer = "container:Sentry.xcodeproj">
          </BuildableReference>
       </MacroExpansion>
+      <EnvironmentVariables>
+         <EnvironmentVariable
+            key = "TSAN_OPTIONS"
+            value = "suppressions=$(PROJECT_DIR)/Sources/Resources/ThreadSanitizer.sup"
+            isEnabled = "YES">
+         </EnvironmentVariable>
+      </EnvironmentVariables>
    </LaunchAction>
    <ProfileAction
       buildConfiguration = "Release"

diff --git a/SentryTestUtils/TestSentryDispatchQueueWrapper.swift b/SentryTestUtils/TestSentryDispatchQueueWrapper.swift
@@ -1,9 +1,12 @@
 import _SentryPrivate
 import Foundation
+@testable import Sentry
 
 /// A wrapper around `SentryDispatchQueueWrapper` that memoized invocations to its methods and allows customization of async logic, specifically: dispatch-after calls can be made to run immediately, or not at all.
 public class TestSentryDispatchQueueWrapper: SentryDispatchQueueWrapper {
 
+    private let dispatchAsyncLock = NSLock()
+
     public var dispatchAsyncCalled = 0
 
     /// Whether or not delayed dispatches should execute.
@@ -13,8 +16,12 @@ public class TestSentryDispatchQueueWrapper: SentryDispatchQueueWrapper {
     public var dispatchAsyncInvocations = Invocations<() -> Void>()
     public var dispatchAsyncExecutesBlock = true
     public override func dispatchAsync(_ block: @escaping () -> Void) {
-        dispatchAsyncCalled += 1
-        dispatchAsyncInvocations.record(block)
+
+        dispatchAsyncLock.synchronized {
+            dispatchAsyncCalled += 1
+            dispatchAsyncInvocations.record(block)
+        }
+
         if dispatchAsyncExecutesBlock {
             block()
         }

diff --git a/Sources/Resources/ThreadSanitizer.sup b/Sources/Resources/ThreadSanitizer.sup
@@ -0,0 +1,11 @@
+# ThreadSanitizer suppressions file
+# For syntax details, see https://github.com/google/sanitizers/wiki/ThreadSanitizerSuppressions
+
+# Races to fix
+race:returnResponse
+race:enableNetworkTracking
+race:enableNetworkBreadcrumbs
+race:disable
+race:URLSessionDataTaskMock
+race:getOriginalImplementation
+race:SentrySpanContext
diff --git a/Sources/Sentry/SentryDependencyContainer.m b/Sources/Sentry/SentryDependencyContainer.m
@@ -5,6 +5,7 @@
 #import "SentryDisplayLinkWrapper.h"
 #import "SentryExtraContextProvider.h"
 #import "SentryFileManager.h"
+#import "SentryInternalCDefines.h"
 #import "SentryLog.h"
 #import "SentryNSProcessInfoWrapper.h"
 #import "SentryNSTimerFactory.h"
@@ -120,7 +121,8 @@ - (SentryAppStateManager *)appStateManager
     }
 }
 
-- (SentryCrashWrapper *)crashWrapper
+- (SentryCrashWrapper *)crashWrapper SENTRY_DISABLE_THREAD_SANITIZER(
+    "double-checked lock produce false alarms")
 {
     if (_crashWrapper == nil) {
         @synchronized(sentryDependencyContainerLock) {
@@ -132,7 +134,8 @@ - (SentryCrashWrapper *)crashWrapper
     return _crashWrapper;
 }
 
-- (SentryCrash *)crashReporter
+- (SentryCrash *)crashReporter SENTRY_DISABLE_THREAD_SANITIZER(
+    "double-checked lock produce false alarms")
 {
     if (_crashReporter == nil) {
         @synchronized(sentryDependencyContainerLock) {
@@ -145,7 +148,8 @@ - (SentryCrash *)crashReporter
     return _crashReporter;
 }
 
-- (SentrySysctl *)sysctlWrapper
+- (SentrySysctl *)sysctlWrapper SENTRY_DISABLE_THREAD_SANITIZER(
+    "double-checked lock produce false alarms")
 {
     if (_sysctlWrapper == nil) {
         @synchronized(sentryDependencyContainerLock) {
@@ -157,7 +161,8 @@ - (SentrySysctl *)sysctlWrapper
     return _sysctlWrapper;
 }
 
-- (SentryThreadInspector *)threadInspector
+- (SentryThreadInspector *)threadInspector SENTRY_DISABLE_THREAD_SANITIZER(
+    "double-checked lock produce false alarms")
 {
     if (_threadInspector == nil) {
         @synchronized(sentryDependencyContainerLock) {
@@ -170,7 +175,8 @@ - (SentryThreadInspector *)threadInspector
     return _threadInspector;
 }
 
-- (SentryExtraContextProvider *)extraContextProvider
+- (SentryExtraContextProvider *)extraContextProvider SENTRY_DISABLE_THREAD_SANITIZER(
+    "double-checked lock produce false alarms")
 {
     if (_extraContextProvider == nil) {
         @synchronized(sentryDependencyContainerLock) {
@@ -193,7 +199,8 @@ - (SentryNSNotificationCenterWrapper *)notificationCenterWrapper
 }
 
 #if TARGET_OS_IOS
-- (SentryUIDeviceWrapper *)uiDeviceWrapper
+- (SentryUIDeviceWrapper *)uiDeviceWrapper SENTRY_DISABLE_THREAD_SANITIZER(
+    "double-checked lock produce false alarms")
 {
 #    if SENTRY_HAS_UIKIT
     if (_uiDeviceWrapper == nil) {
@@ -214,7 +221,8 @@ - (SentryUIDeviceWrapper *)uiDeviceWrapper
 #endif // TARGET_OS_IOS
 
 #if SENTRY_UIKIT_AVAILABLE
-- (SentryScreenshot *)screenshot
+- (SentryScreenshot *)screenshot SENTRY_DISABLE_THREAD_SANITIZER(
+    "double-checked lock produce false alarms")
 {
 #    if SENTRY_HAS_UIKIT
     if (_screenshot == nil) {
@@ -233,7 +241,8 @@ - (SentryScreenshot *)screenshot
 #    endif // SENTRY_HAS_UIKIT
 }
 
-- (SentryViewHierarchy *)viewHierarchy
+- (SentryViewHierarchy *)viewHierarchy SENTRY_DISABLE_THREAD_SANITIZER(
+    "double-checked lock produce false alarms")
 {
 #    if SENTRY_HAS_UIKIT
     if (_viewHierarchy == nil) {
@@ -252,7 +261,8 @@ - (SentryViewHierarchy *)viewHierarchy
 #    endif // SENTRY_HAS_UIKIT
 }
 
-- (SentryUIApplication *)application
+- (SentryUIApplication *)application SENTRY_DISABLE_THREAD_SANITIZER(
+    "double-checked lock produce false alarms")
 {
 #    if SENTRY_HAS_UIKIT
     if (_application == nil) {
@@ -271,7 +281,8 @@ - (SentryUIApplication *)application
 #    endif // SENTRY_HAS_UIKIT
 }
 
-- (SentryFramesTracker *)framesTracker
+- (SentryFramesTracker *)framesTracker SENTRY_DISABLE_THREAD_SANITIZER(
+    "double-checked lock produce false alarms")
 {
 #    if SENTRY_HAS_UIKIT
     if (_framesTracker == nil) {
@@ -294,7 +305,8 @@ - (SentryFramesTracker *)framesTracker
 #    endif // SENTRY_HAS_UIKIT
 }
 
-- (SentrySwizzleWrapper *)swizzleWrapper
+- (SentrySwizzleWrapper *)swizzleWrapper SENTRY_DISABLE_THREAD_SANITIZER(
+    "double-checked lock produce false alarms")
 {
 #    if SENTRY_HAS_UIKIT
     if (_swizzleWrapper == nil) {
@@ -315,6 +327,7 @@ - (SentrySwizzleWrapper *)swizzleWrapper
 #endif // SENTRY_UIKIT_AVAILABLE
 
 - (SentryANRTracker *)getANRTracker:(NSTimeInterval)timeout
+    SENTRY_DISABLE_THREAD_SANITIZER("double-checked lock produce false alarms")
 {
     if (_anrTracker == nil) {
         @synchronized(sentryDependencyContainerLock) {
@@ -331,7 +344,8 @@ - (SentryANRTracker *)getANRTracker:(NSTimeInterval)timeout
     return _anrTracker;
 }
 
-- (SentryNSProcessInfoWrapper *)processInfoWrapper
+- (SentryNSProcessInfoWrapper *)processInfoWrapper SENTRY_DISABLE_THREAD_SANITIZER(
+    "double-checked lock produce false alarms")
 {
     if (_processInfoWrapper == nil) {
         @synchronized(sentryDependencyContainerLock) {
@@ -343,7 +357,8 @@ - (SentryNSProcessInfoWrapper *)processInfoWrapper
     return _processInfoWrapper;
 }
 
-- (SentrySystemWrapper *)systemWrapper
+- (SentrySystemWrapper *)systemWrapper SENTRY_DISABLE_THREAD_SANITIZER(
+    "double-checked lock produce false alarms")
 {
     if (_systemWrapper == nil) {
         @synchronized(sentryDependencyContainerLock) {
@@ -355,7 +370,8 @@ - (SentrySystemWrapper *)systemWrapper
     return _systemWrapper;
 }
 
-- (SentryDispatchFactory *)dispatchFactory
+- (SentryDispatchFactory *)dispatchFactory SENTRY_DISABLE_THREAD_SANITIZER(
+    "double-checked lock produce false alarms")
 {
     if (_dispatchFactory == nil) {
         @synchronized(sentryDependencyContainerLock) {
@@ -367,7 +383,8 @@ - (SentryDispatchFactory *)dispatchFactory
     return _dispatchFactory;
 }
 
-- (SentryNSTimerFactory *)timerFactory
+- (SentryNSTimerFactory *)timerFactory SENTRY_DISABLE_THREAD_SANITIZER(
+    "double-checked lock produce false alarms")
 {
     if (_timerFactory == nil) {
         @synchronized(sentryDependencyContainerLock) {
@@ -380,7 +397,8 @@ - (SentryNSTimerFactory *)timerFactory
 }
 
 #if SENTRY_HAS_METRIC_KIT
-- (SentryMXManager *)metricKitManager
+- (SentryMXManager *)metricKitManager SENTRY_DISABLE_THREAD_SANITIZER(
+    "double-checked lock produce false alarms")
 {
     if (_metricKitManager == nil) {
         @synchronized(sentryDependencyContainerLock) {
@@ -399,7 +417,8 @@ - (SentryMXManager *)metricKitManager
 #endif // SENTRY_HAS_METRIC_KIT
 
 #if SENTRY_HAS_REACHABILITY
-- (SentryReachability *)reachability
+- (SentryReachability *)reachability SENTRY_DISABLE_THREAD_SANITIZER(
+    "double-checked lock produce false alarms")
 {
     if (_reachability == nil) {
         @synchronized(sentryDependencyContainerLock) {

diff --git a/Sources/Sentry/SentryFramesTracker.m b/Sources/Sentry/SentryFramesTracker.m
@@ -7,7 +7,7 @@
 #    import "SentryDelayedFramesTracker.h"
 #    import "SentryDispatchQueueWrapper.h"
 #    import "SentryDisplayLinkWrapper.h"
-#    import "SentryInternalDefines.h"
+#    import "SentryInternalCDefines.h"
 #    import "SentryLog.h"
 #    import "SentryProfiler+Private.h"
 #    import "SentryProfilingConditionals.h"
@@ -87,22 +87,16 @@ - (void)setDisplayLinkWrapper:(SentryDisplayLinkWrapper *)displayLinkWrapper
 {
     _displayLinkWrapper = displayLinkWrapper;
 }
-
-/**
- * As you can only disable the thread sanitizer for methods, we must manually create the setter
- * here.
- */
 - (void)setPreviousFrameSystemTimestamp:(uint64_t)previousFrameSystemTimestamp
-    SENTRY_DISABLE_THREAD_SANITIZER
+    SENTRY_DISABLE_THREAD_SANITIZER("As you can only disable the thread sanitizer for methods, we "
+                                    "must manually create the setter here.")
 {
     _previousFrameSystemTimestamp = previousFrameSystemTimestamp;
 }
 
-/**
- * As you can only disable the thread sanitizer for methods, we must manually create the getter
- * here.
- */
-- (uint64_t)getPreviousFrameSystemTimestamp SENTRY_DISABLE_THREAD_SANITIZER
+- (uint64_t)getPreviousFrameSystemTimestamp SENTRY_DISABLE_THREAD_SANITIZER(
+    "As you can only disable the thread sanitizer for methods, we must manually create the getter "
+    "here.")
 {
     return _previousFrameSystemTimestamp;
 }
@@ -251,7 +245,7 @@ - (void)recordTimestamp:(uint64_t)timestamp value:(NSNumber *)value array:(NSMut
 }
 #    endif // SENTRY_TARGET_PROFILING_SUPPORTED
 
-- (SentryScreenFrames *)currentFrames SENTRY_DISABLE_THREAD_SANITIZER
+- (SentryScreenFrames *)currentFrames SENTRY_DISABLE_THREAD_SANITIZER()
 {
 #    if SENTRY_TARGET_PROFILING_SUPPORTED
     return [[SentryScreenFrames alloc] initWithTotal:_totalFrames
@@ -268,7 +262,7 @@ - (SentryScreenFrames *)currentFrames SENTRY_DISABLE_THREAD_SANITIZER
 }
 
 - (CFTimeInterval)getFramesDelay:(uint64_t)startSystemTimestamp
-              endSystemTimestamp:(uint64_t)endSystemTimestamp SENTRY_DISABLE_THREAD_SANITIZER
+              endSystemTimestamp:(uint64_t)endSystemTimestamp SENTRY_DISABLE_THREAD_SANITIZER()
 {
     return [self.delayedFramesTracker getFramesDelay:startSystemTimestamp
                                   endSystemTimestamp:endSystemTimestamp

diff --git a/Sources/Sentry/SentryTracer.m b/Sources/Sentry/SentryTracer.m
@@ -5,6 +5,7 @@
 #import "SentryEvent+Private.h"
 #import "SentryFileManager.h"
 #import "SentryHub+Private.h"
+#import "SentryInternalCDefines.h"
 #import "SentryInternalDefines.h"
 #import "SentryLog.h"
 #import "SentryNSDictionarySanitize.h"
@@ -745,7 +746,8 @@ - (SentryTransaction *)toTransaction
 
 #if SENTRY_HAS_UIKIT
 
-- (nullable SentryAppStartMeasurement *)getAppStartMeasurement
+- (nullable SentryAppStartMeasurement *)getAppStartMeasurement SENTRY_DISABLE_THREAD_SANITIZER(
+    "double-checked lock produce false alarms")
 {
     // Only send app start measurement for transactions generated by auto performance
     // instrumentation.

diff --git a/Sources/Sentry/include/SentryInternalCDefines.h b/Sources/Sentry/include/SentryInternalCDefines.h
@@ -1 +1,14 @@
 typedef unsigned long long bytes;
+
+/**
+ * For disabling the thread sanitizer for a method
+ */
+#if defined(__has_feature)
+#    if __has_feature(thread_sanitizer)
+#        define SENTRY_DISABLE_THREAD_SANITIZER(message) __attribute__((no_sanitize("thread")))
+#    else
+#        define SENTRY_DISABLE_THREAD_SANITIZER(message)
+#    endif
+#else
+#    define SENTRY_DISABLE_THREAD_SANITIZER(message)
+#endif
diff --git a/Sources/Sentry/include/SentryInternalDefines.h b/Sources/Sentry/include/SentryInternalDefines.h
@@ -66,16 +66,3 @@ static NSString *const SentryPlatformName = @"cocoa";
 #define SPAN_DATA_BLOCKED_MAIN_THREAD @"blocked_main_thread"
 #define SPAN_DATA_THREAD_ID @"thread.id"
 #define SPAN_DATA_THREAD_NAME @"thread.name"
-
-/**
- * For disabling the thread sanitizer for a method
- */
-#if defined(__has_feature)
-#    if __has_feature(thread_sanitizer)
-#        define SENTRY_DISABLE_THREAD_SANITIZER __attribute__((no_sanitize("thread")))
-#    else
-#        define SENTRY_DISABLE_THREAD_SANITIZER
-#    endif
-#else
-#    define SENTRY_DISABLE_THREAD_SANITIZER
-#endif
diff --git a/Sources/SentryCrash/Recording/Monitors/SentryCrashMonitor_MachException.c b/Sources/SentryCrash/Recording/Monitors/SentryCrashMonitor_MachException.c
@@ -32,6 +32,7 @@
 #include "SentryCrashStackCursor_MachineContext.h"
 #include "SentryCrashSystemCapabilities.h"
 #include "SentryCrashThread.h"
+#include "SentryInternalCDefines.h"
 
 // #define SentryCrashLogger_LocalLevel TRACE
 #include "SentryCrashLogger.h"
@@ -409,7 +410,7 @@ handleExceptions(void *const userData)
 // ============================================================================
 
 static void
-uninstallExceptionHandler(void)
+uninstallExceptionHandler(void) SENTRY_DISABLE_THREAD_SANITIZER("Known data race to fix")
 {
     SentryCrashLOG_DEBUG("Uninstalling mach exception handler.");