fix: Reporting crashes when restarting the SDK

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 ### Fixes
 
+- Reporting crashes when restarting the SDK (#2440)
 - Core data span status with error (#2439)
 
 ## 7.31.2

Samples/iOS-Swift/iOS-Swift/AppDelegate.swift

@@ -7,9 +7,8 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
     var window: UIWindow?
 
     static let defaultDSN = "https://[email protected]/5428557"
-
-    func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]?) -> Bool {
-
+
+    static func startSentry() {
         // For testing purposes, we want to be able to change the DSN and store it to disk. In a real app, you shouldn't need this behavior.
         let dsn = DSNStorage.shared.getDSN() ?? AppDelegate.defaultDSN
         DSNStorage.shared.saveDSN(dsn: dsn)
@@ -45,6 +44,11 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
             let httpStatusCodeRange = HttpStatusCodeRange(min: 400, max: 599)
             options.failedRequestStatusCodes = [ httpStatusCodeRange ]
         }
+    }
+
+    func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]?) -> Bool {
+
+        AppDelegate.startSentry()
 
         if #available(iOS 14.0, *) {
             metricKit.receiveReports()

Samples/iOS-Swift/iOS-Swift/ViewController.swift

@@ -291,4 +291,11 @@ class ViewController: UIViewController {
         SentrySDK.flush(timeout: 5)
     }
 
+    @IBAction func close(_ sender: Any) {
+        SentrySDK.close()
+    }
+
+    @IBAction func startSDK(_ sender: Any) {
+        AppDelegate.startSentry()
+    }
 }

Samples/iOS-Swift/iOS13-Swift/AppDelegate.swift

@@ -5,26 +5,30 @@ import UIKit
 class AppDelegate: UIResponder, UIApplicationDelegate {
 
     static let defaultDSN = "https://[email protected]/5428557"
+
+    static func startSentry() {
+        // For testing purposes, we want to be able to change the DSN and store it to disk. In a real app, you shouldn't need this behavior.
+        let dsn = DSNStorage.shared.getDSN() ?? AppDelegate.defaultDSN
+        DSNStorage.shared.saveDSN(dsn: dsn)
+
+        SentrySDK.start { options in
+            options.dsn = dsn
+            options.beforeSend = { event in
+                return event
+            }
+            options.debug = true
+            // Sampling 100% - In Production you probably want to adjust this
+            options.tracesSampleRate = 1.0
+            options.sessionTrackingIntervalMillis = 5_000
+            if ProcessInfo.processInfo.arguments.contains("--io.sentry.profiling.enable") {
+                options.profilesSampleRate = 1
+            }
+        }
+    }
 
     func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]?) -> Bool {
 
-            // For testing purposes, we want to be able to change the DSN and store it to disk. In a real app, you shouldn't need this behavior.
-            let dsn = DSNStorage.shared.getDSN() ?? AppDelegate.defaultDSN
-            DSNStorage.shared.saveDSN(dsn: dsn)
-
-            SentrySDK.start { options in
-                options.dsn = dsn
-                options.beforeSend = { event in
-                    return event
-                }
-                options.debug = true
-                // Sampling 100% - In Production you probably want to adjust this
-                options.tracesSampleRate = 1.0
-                options.sessionTrackingIntervalMillis = 5_000
-                if ProcessInfo.processInfo.arguments.contains("--io.sentry.profiling.enable") {
-                    options.profilesSampleRate = 1
-                }
-            }
+        AppDelegate.startSentry()
 
         return true
     }

Sentry.xcodeproj/project.pbxproj

@@ -382,6 +382,7 @@
 		7B6D98EB24C6E84F005502FA /* SentryCrashInstallationReporterTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7B6D98EA24C6E84F005502FA /* SentryCrashInstallationReporterTests.swift */; };
 		7B6D98ED24C703F8005502FA /* Async.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7B6D98EC24C703F8005502FA /* Async.swift */; };
 		7B72D23A28D074BC0014798A /* TestExtensions.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7B72D23928D074BC0014798A /* TestExtensions.swift */; };
+		7B7725D8292F5DC20015BBF9 /* SentryCrashInstallationTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 7B7725D7292F5DC20015BBF9 /* SentryCrashInstallationTests.m */; };
 		7B77BE3527EC8445003C9020 /* SentryDiscardReasonMapper.h in Headers */ = {isa = PBXBuildFile; fileRef = 7B77BE3427EC8445003C9020 /* SentryDiscardReasonMapper.h */; };
 		7B77BE3727EC8460003C9020 /* SentryDiscardReasonMapper.m in Sources */ = {isa = PBXBuildFile; fileRef = 7B77BE3627EC8460003C9020 /* SentryDiscardReasonMapper.m */; };
 		7B7A30C624B48321005A4C6E /* SentryCrashWrapper.h in Headers */ = {isa = PBXBuildFile; fileRef = 7B7A30C524B48321005A4C6E /* SentryCrashWrapper.h */; };
@@ -1137,6 +1138,7 @@
 		7B6D98EA24C6E84F005502FA /* SentryCrashInstallationReporterTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryCrashInstallationReporterTests.swift; sourceTree = "<group>"; };
 		7B6D98EC24C703F8005502FA /* Async.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Async.swift; sourceTree = "<group>"; };
 		7B72D23928D074BC0014798A /* TestExtensions.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TestExtensions.swift; sourceTree = "<group>"; };
+		7B7725D7292F5DC20015BBF9 /* SentryCrashInstallationTests.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryCrashInstallationTests.m; sourceTree = "<group>"; };
 		7B77BE3427EC8445003C9020 /* SentryDiscardReasonMapper.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SentryDiscardReasonMapper.h; path = include/SentryDiscardReasonMapper.h; sourceTree = "<group>"; };
 		7B77BE3627EC8460003C9020 /* SentryDiscardReasonMapper.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryDiscardReasonMapper.m; sourceTree = "<group>"; };
 		7B7A30C524B48321005A4C6E /* SentryCrashWrapper.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SentryCrashWrapper.h; path = include/SentryCrashWrapper.h; sourceTree = "<group>"; };
@@ -2208,6 +2210,7 @@
 				63FE71F120DA66EA00CDBAE8 /* Container+DeepSearch_Tests.m */,
 				63FE71F620DA66EB00CDBAE8 /* FileBasedTestCase.h */,
 				63FE71D920DA66E700CDBAE8 /* FileBasedTestCase.m */,
+				7B7725D7292F5DC20015BBF9 /* SentryCrashInstallationTests.m */,
 				D855AD61286ED6A4002573E1 /* SentryCrashTests.m */,
 				63FE71E220DA66E800CDBAE8 /* NSError+SimpleConstructor_Tests.m */,
 				63FE71D520DA66E600CDBAE8 /* RFC3339UTFString_Tests.m */,
@@ -3678,6 +3681,7 @@
 				7BED3576266F7BFF00EAA70D /* TestSentryCrashWrapper.m in Sources */,
 				7BC6EC18255C44540059822A /* SentryDebugMetaTests.swift in Sources */,
 				A811D867248E2770008A41EA /* SentrySystemEventBreadcrumbsTest.swift in Sources */,
+				7B7725D8292F5DC20015BBF9 /* SentryCrashInstallationTests.m in Sources */,
 				7B82D54924E2A2D400EE670F /* SentryIdTests.swift in Sources */,
 				7BD47B4E268F0B470076A663 /* ClearTestState.swift in Sources */,
 				7B6D98ED24C703F8005502FA /* Async.swift in Sources */,

Sources/Sentry/SentryCrashIntegration.m

@@ -157,10 +157,12 @@ + (void)sendAllSentryCrashReports
 - (void)uninstall
 {
     if (nil != installation) {
-        [self.crashAdapter close];
+        [installation uninstall];
         installationToken = 0;
     }
 
+    [self.crashAdapter uninstallAsyncHooks];
+
     [NSNotificationCenter.defaultCenter removeObserver:self
                                                   name:NSCurrentLocaleDidChangeNotification
                                                 object:nil];

Sources/Sentry/SentryCrashWrapper.m

@@ -56,16 +56,9 @@ - (void)installAsyncHooks
     sentrycrash_install_async_hooks();
 }
 
-- (void)close
+- (void)uninstallAsyncHooks
 {
-    SentryCrash *handler = [SentryCrash sharedInstance];
-    @synchronized(handler) {
-        [handler setMonitoring:SentryCrashMonitorTypeNone];
-        handler.onCrash = NULL;
-    }
-
     sentrycrash_deactivate_async_hooks();
-    sentrycrashccd_close();
 }
 
 - (NSDictionary *)systemInfo

Sources/Sentry/include/SentryCrashWrapper.h

@@ -25,11 +25,7 @@ SENTRY_NO_INIT
 
 - (void)installAsyncHooks;
 
-/**
- * It's not really possible to close SentryCrash. Best we can do is to deactivate all the monitors,
- * clear the `onCrash` callback installed on the global handler, and a few more minor things.
- */
-- (void)close;
+- (void)uninstallAsyncHooks;
 
 - (NSDictionary *)systemInfo;
 

Sources/SentryCrash/Installations/SentryCrashInstallation+Private.h

@@ -51,6 +51,17 @@
     IMPLEMENT_REPORT_VALUE_PROPERTY(NAME, NAMEUPPER, TYPE)                                         \
     IMPLEMENT_REPORT_KEY_PROPERTY(NAME, NAMEUPPER)
 
+typedef struct {
+    const char *key;
+    const char *value;
+} ReportField;
+
+typedef struct {
+    SentryCrashReportWriteCallback userCrashCallback;
+    int reportFieldsCount;
+    ReportField *reportFields[0];
+} CrashHandlerData;
+
 @interface
 SentryCrashInstallation ()
 
@@ -84,4 +95,7 @@ SentryCrashInstallation ()
 /** Make an absolute key paths from the specified paths. */
 - (NSArray *)makeKeyPaths:(NSArray *)keyPaths;
 
+/** Only needed for testing */
+- (CrashHandlerData *)g_crashHandlerData;
+
 @end

Sources/SentryCrash/Installations/SentryCrashInstallation.h

@@ -49,6 +49,11 @@
  */
 - (void)install;
 
+/**
+ * Call this instead of `-[SentryCrash uninstall]`.
+ */
+- (void)uninstall;
+
 /** Convenience method to call -[SentryCrash sendAllReportsWithCompletion:].
  * This method will set the SentryCrash sink and then send all outstanding
  * reports.

Sources/SentryCrash/Installations/SentryCrashInstallation.m

@@ -37,17 +37,6 @@
 /** Max number of properties that can be defined for writing to the report */
 #define kMaxProperties 500
 
-typedef struct {
-    const char *key;
-    const char *value;
-} ReportField;
-
-typedef struct {
-    SentryCrashReportWriteCallback userCrashCallback;
-    int reportFieldsCount;
-    ReportField *reportFields[0];
-} CrashHandlerData;
-
 static CrashHandlerData *g_crashHandlerData;
 
 static void
@@ -199,6 +188,11 @@ - (CrashHandlerData *)crashHandlerData
     return (CrashHandlerData *)self.crashHandlerDataBacking.mutableBytes;
 }
 
+- (CrashHandlerData *)g_crashHandlerData
+{
+    return g_crashHandlerData;
+}
+
 - (SentryCrashInstReportField *)reportFieldForProperty:(NSString *)propertyName
 {
     SentryCrashInstReportField *field = [self.fields objectForKey:propertyName];
@@ -298,6 +292,18 @@ - (void)install
     }
 }
 
+- (void)uninstall
+{
+    SentryCrash *handler = [SentryCrash sharedInstance];
+    @synchronized(handler) {
+        if (g_crashHandlerData == self.crashHandlerData) {
+            g_crashHandlerData = NULL;
+            handler.onCrash = NULL;
+        }
+        [handler uninstall];
+    }
+}
+
 - (void)sendAllReportsWithCompletion:(SentryCrashReportFilterCompletion)onCompletion
 {
     NSError *error = [self validateProperties];

Sources/SentryCrash/Recording/Monitors/SentryCrashMonitor.c

@@ -146,11 +146,17 @@ addContextualInfoToEvent(Monitor *monitor, struct SentryCrash_MonitorContext *ev
 }
 
 void
-sentrycrashcm_setEventCallback(void (*onEvent)(struct SentryCrash_MonitorContext *monitorContext))
+sentrycrashcm_setEventCallback(SentryCrashMonitorEventCallback onEvent)
 {
     g_onExceptionEvent = onEvent;
 }
 
+SentryCrashMonitorEventCallback
+sentrycrashcm_getEventCallback(void)
+{
+    return g_onExceptionEvent;
+}
+
 void
 sentrycrashcm_setActiveMonitors(SentryCrashMonitorType monitorTypes)
 {

Sources/SentryCrash/Recording/Monitors/SentryCrashMonitor.h

@@ -55,12 +55,18 @@ void sentrycrashcm_setActiveMonitors(SentryCrashMonitorType monitorTypes);
  */
 SentryCrashMonitorType sentrycrashcm_getActiveMonitors(void);
 
+typedef void (*SentryCrashMonitorEventCallback)(struct SentryCrash_MonitorContext *);
+
 /** Set the callback to call when an event is captured.
  *
  * @param onEvent Called whenever an event is captured.
  */
-void sentrycrashcm_setEventCallback(
-    void (*onEvent)(struct SentryCrash_MonitorContext *monitorContext));
+void sentrycrashcm_setEventCallback(SentryCrashMonitorEventCallback onEvent);
+
+/** Get the current SentryCrashMonitorEventCallback. Only needed for testing.
+ *
+ */
+SentryCrashMonitorEventCallback sentrycrashcm_getEventCallback(void);
 
 // ============================================================================
 #pragma mark - Internal API -

Sources/SentryCrash/Recording/Monitors/SentryCrashMonitor_MachException.c

@@ -109,6 +109,7 @@ typedef struct {
 // ============================================================================
 
 static volatile bool g_isEnabled = false;
+static volatile bool g_isInstalled = false;
 
 static SentryCrash_MonitorContext g_monitorContext;
 static SentryCrashStackCursor g_stackCursor;
@@ -236,6 +237,42 @@ machExceptionForSignal(int sigNum)
     return 0;
 }
 
+// ============================================================================
+#    pragma mark - Reserved threads -
+// ============================================================================
+/**
+ * We only have reserved threads if SentryCrashCRASH_HAS_MACH.
+ */
+
+bool
+sentrycrashcm_isReservedThread(thread_t thread)
+{
+    return thread == g_primaryMachThread || thread == g_secondaryMachThread;
+}
+
+bool
+sentrycrashcm_hasReservedThreads(void)
+{
+    return g_primaryMachThread != 0 && g_secondaryMachThread != 0;
+}
+
+#else
+bool
+sentrycrashcm_isReservedThread(thread_t thread)
+{
+    return false;
+}
+
+bool
+sentrycrashcm_hasReservedThreads(void)
+{
+    return false;
+}
+
+#endif
+
+#if SentryCrashCRASH_HAS_MACH
+
 // ============================================================================
 #    pragma mark - Handler -
 // ============================================================================
@@ -458,7 +495,6 @@ installExceptionHandler()
         goto failed;
     }
     g_secondaryMachThread = pthread_mach_thread_np(g_secondaryPThread);
-    sentrycrashmc_addReservedThread(g_secondaryMachThread);
 
     SentryCrashLOG_DEBUG("Creating primary exception thread.");
     error = pthread_create(&g_primaryPThread, &attr, &handleExceptions, kThreadPrimary);
@@ -468,9 +504,9 @@ installExceptionHandler()
     }
     pthread_attr_destroy(&attr);
     g_primaryMachThread = pthread_mach_thread_np(g_primaryPThread);
-    sentrycrashmc_addReservedThread(g_primaryMachThread);
 
     SentryCrashLOG_DEBUG("Mach exception handler installed.");
+    g_isInstalled = true;
     return true;
 
 failed:
@@ -485,18 +521,18 @@ installExceptionHandler()
 static void
 setEnabled(bool isEnabled)
 {
-    if (isEnabled != g_isEnabled) {
-        g_isEnabled = isEnabled;
-        if (isEnabled) {
-            sentrycrashid_generate(g_primaryEventID);
-            sentrycrashid_generate(g_secondaryEventID);
-            if (!installExceptionHandler()) {
-                return;
-            }
-        } else {
-            uninstallExceptionHandler();
-        }
+    g_isEnabled = isEnabled;
+    if (g_isEnabled && !g_isInstalled) {
+        sentrycrashid_generate(g_primaryEventID);
+        sentrycrashid_generate(g_secondaryEventID);
+        g_isEnabled = installExceptionHandler();
     }
+
+    // We don't call uninstallExceptionHandler by intention. Instead of canceling the exception
+    // handler threads and restarting them, we let them run. They won't do anything in
+    // handleExceptions when g_isEnabled is false. Trying to clean them up lead to weird crashes in
+    // tests cause sentrycrashmc_suspendEnvironment and sentrycrashmc_resumeEnvironment call
+    // sentrycrashcm_isReservedThread. Instead of fixing that, we let them run.
 }
 
 static bool