Fix: Remove sync call to main thread from SentryUIDeviceWrapper (#3295)

SentryUIDeviceWrapper previously had a synchronized call to the main thread when the class was initialized in a background thread, potentially leading to deadlocks with SentryDependencyContainer. To address this, the synchronized call has been replaced with an asynchronous one. Additionally, a new start function has been introduced and is invoked from SentrySDK.start to ensure initialization as early as possible.
getsentry · Sep 25, 2023 · 898177b · 898177b
1 parent 72c8d84
commit 898177b
Show file tree

Hide file tree

Showing 10 changed files with 75 additions and 24 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+### Fixes
+
+- Remove sync call to main thread from SentryUIDeviceWrapper (#3295)
+
 ### Features
 
 - Record changes to network connectivity in breadcrumbs (#3232)

diff --git a/SentryTestUtils/TestSentryDispatchQueueWrapper.swift b/SentryTestUtils/TestSentryDispatchQueueWrapper.swift
@@ -26,6 +26,13 @@ public class TestSentryDispatchQueueWrapper: SentryDispatchQueueWrapper {
     public var blockOnMainInvocations = Invocations<() -> Void>()
     public var blockBeforeMainBlock: () -> Bool = { true }
 
+    public override func dispatch(onMainQueue block: @escaping () -> Void) {
+        blockOnMainInvocations.record(block)
+        if blockBeforeMainBlock() {
+            block()
+        }
+    }
+
     public override func dispatchAsync(onMainQueue block: @escaping () -> Void) {
         blockOnMainInvocations.record(block)
         if blockBeforeMainBlock() {

diff --git a/Sources/Sentry/SentryDispatchQueueWrapper.m b/Sources/Sentry/SentryDispatchQueueWrapper.m
@@ -1,4 +1,5 @@
 #import "SentryDispatchQueueWrapper.h"
+#import "SentryThreadWrapper.h"
 #import <Foundation/Foundation.h>
 
 NS_ASSUME_NONNULL_BEGIN
@@ -41,6 +42,11 @@ - (void)dispatchAsyncOnMainQueue:(void (^)(void))block
     });
 }
 
+- (void)dispatchOnMainQueue:(void (^)(void))block
+{
+    [SentryThreadWrapper onMainThread:block];
+}
+
 - (void)dispatchSyncOnMainQueue:(void (^)(void))block
 {
     if ([NSThread isMainThread]) {

diff --git a/Sources/Sentry/SentrySDK.m b/Sources/Sentry/SentrySDK.m
@@ -14,6 +14,7 @@
 #import "SentryMeta.h"
 #import "SentryOptions+Private.h"
 #import "SentryScope.h"
+#import "SentryUIDeviceWrapper.h"
 
 @interface
 SentrySDK ()
@@ -152,6 +153,9 @@ + (void)startWithOptions:(SentryOptions *)options
 
     [SentryCrashWrapper.sharedInstance startBinaryImageCache];
     [SentryDependencyContainer.sharedInstance.binaryImageCache start];
+#if TARGET_OS_IOS
+    [SentryDependencyContainer.sharedInstance.uiDeviceWrapper start];
+#endif
 }
 
 + (void)startWithConfigureOptions:(void (^)(SentryOptions *options))configureOptions
@@ -401,11 +405,14 @@ + (void)close
 
     [SentrySDK setCurrentHub:nil];
 
-    [SentryDependencyContainer reset];
-
     [SentryCrashWrapper.sharedInstance stopBinaryImageCache];
     [SentryDependencyContainer.sharedInstance.binaryImageCache stop];
 
+#if TARGET_OS_IOS
+    [SentryDependencyContainer.sharedInstance.uiDeviceWrapper stop];
+#endif
+
+    [SentryDependencyContainer reset];
     SENTRY_LOG_DEBUG(@"SDK closed!");
 }
 

diff --git a/Sources/Sentry/SentryUIDeviceWrapper.m b/Sources/Sentry/SentryUIDeviceWrapper.m
@@ -14,33 +14,32 @@ @implementation SentryUIDeviceWrapper
 
 #if TARGET_OS_IOS
 
-- (instancetype)init
+- (void)start
 {
-    if (self = [super init]) {
-        [SentryDependencyContainer.sharedInstance.dispatchQueueWrapper dispatchSyncOnMainQueue:^{
-            // Needed to read the device orientation on demand
-            if (!UIDevice.currentDevice.isGeneratingDeviceOrientationNotifications) {
-                self.cleanupDeviceOrientationNotifications = YES;
-                [UIDevice.currentDevice beginGeneratingDeviceOrientationNotifications];
-            }
-
-            // Needed so we can read the battery level
-            if (!UIDevice.currentDevice.isBatteryMonitoringEnabled) {
-                self.cleanupBatteryMonitoring = YES;
-                UIDevice.currentDevice.batteryMonitoringEnabled = YES;
-            }
-        }];
-    }
-    return self;
+    [SentryDependencyContainer.sharedInstance.dispatchQueueWrapper dispatchOnMainQueue:^{
+        if (!UIDevice.currentDevice.isGeneratingDeviceOrientationNotifications) {
+            self.cleanupDeviceOrientationNotifications = YES;
+            [UIDevice.currentDevice beginGeneratingDeviceOrientationNotifications];
+        }
+
+        // Needed so we can read the battery level
+        if (!UIDevice.currentDevice.isBatteryMonitoringEnabled) {
+            self.cleanupBatteryMonitoring = YES;
+            UIDevice.currentDevice.batteryMonitoringEnabled = YES;
+        }
+    }];
 }
 
 - (void)stop
 {
-    [SentryDependencyContainer.sharedInstance.dispatchQueueWrapper dispatchSyncOnMainQueue:^{
-        if (self.cleanupDeviceOrientationNotifications) {
+    BOOL needsCleanUp = self.cleanupDeviceOrientationNotifications;
+    BOOL needsDisablingBattery = self.cleanupBatteryMonitoring;
+
+    [SentryDependencyContainer.sharedInstance.dispatchQueueWrapper dispatchOnMainQueue:^{
+        if (needsCleanUp) {
             [UIDevice.currentDevice endGeneratingDeviceOrientationNotifications];
         }
-        if (self.cleanupBatteryMonitoring) {
+        if (needsDisablingBattery) {
             UIDevice.currentDevice.batteryMonitoringEnabled = NO;
         }
     }];

diff --git a/Sources/Sentry/include/SentryDispatchQueueWrapper.h b/Sources/Sentry/include/SentryDispatchQueueWrapper.h
@@ -15,6 +15,8 @@ NS_ASSUME_NONNULL_BEGIN
 
 - (void)dispatchAsyncOnMainQueue:(void (^)(void))block;
 
+- (void)dispatchOnMainQueue:(void (^)(void))block;
+
 - (void)dispatchSyncOnMainQueue:(void (^)(void))block;
 
 - (void)dispatchAfter:(NSTimeInterval)interval block:(dispatch_block_t)block;

diff --git a/Sources/Sentry/include/SentryUIDeviceWrapper.h b/Sources/Sentry/include/SentryUIDeviceWrapper.h
@@ -11,7 +11,7 @@ NS_ASSUME_NONNULL_BEGIN
 @interface SentryUIDeviceWrapper : NSObject
 
 #if TARGET_OS_IOS
-- (instancetype)init;
+- (void)start;
 - (void)stop;
 - (UIDeviceOrientation)orientation;
 - (BOOL)isBatteryMonitoringEnabled;

diff --git a/Tests/SentryTests/SentryCrash/SentryUIDeviceWrapperTests.swift b/Tests/SentryTests/SentryCrash/SentryUIDeviceWrapperTests.swift
@@ -7,6 +7,7 @@ class SentryUIDeviceWrapperTests: XCTestCase {
         let dispatchQueue = TestSentryDispatchQueueWrapper()
         SentryDependencyContainer.sharedInstance().dispatchQueueWrapper = dispatchQueue
         let sut = SentryUIDeviceWrapper()
+        sut.start()
         XCTAssertEqual(dispatchQueue.blockOnMainInvocations.count, 1)
 
         sut.stop()

diff --git a/Tests/SentryTests/SentryCrash/TestSentryUIDeviceWrapper.swift b/Tests/SentryTests/SentryCrash/TestSentryUIDeviceWrapper.swift
@@ -6,6 +6,14 @@ class TestSentryUIDeviceWrapper: SentryUIDeviceWrapper {
     var internalIsBatteryMonitoringEnabled = true
     var internalBatteryLevel: Float = 0.6
     var internalBatteryState = UIDevice.BatteryState.charging
+    var started = false
+
+    override func start() {
+        started = true
+    }
+    override func stop() {
+        started = false
+    }
 
     override func orientation() -> UIDeviceOrientation {
         return internalOrientation

diff --git a/Tests/SentryTests/SentrySDKTests.swift b/Tests/SentryTests/SentrySDKTests.swift
@@ -585,7 +585,24 @@ class SentrySDKTests: XCTestCase {
         XCTAssertTrue(testTTDTracker.registerFullDisplayCalled)
     }
 #endif
-
+
+#if os(iOS)
+    func testSentryUIDeviceWrapperStarted() {
+        let deviceWrapper = TestSentryUIDeviceWrapper()
+        SentryDependencyContainer.sharedInstance().uiDeviceWrapper = deviceWrapper
+        SentrySDK.start(options: fixture.options)
+        XCTAssertTrue(deviceWrapper.started)
+    }
+
+    func testSentryUIDeviceWrapperStopped() {
+        let deviceWrapper = TestSentryUIDeviceWrapper()
+        SentryDependencyContainer.sharedInstance().uiDeviceWrapper = deviceWrapper
+        SentrySDK.start(options: fixture.options)
+        SentrySDK.close()
+        XCTAssertFalse(deviceWrapper.started)
+    }
+#endif
+
     func testClose_SetsClientToNil() {
         SentrySDK.start { options in
             options.dsn = SentrySDKTests.dsnAsString