Merge 0f5b1f5 into 459ae5e

getsentry · Dec 5, 2022 · b112ed8 · b112ed8
2 parents 459ae5e + 0f5b1f5
commit b112ed8
Show file tree

Hide file tree

Showing 8 changed files with 53 additions and 13 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -17,6 +17,7 @@ This version adds a dependency on Swift.
 - The SDK no longer reports an OOM when a crash happens after closing the SDK (#2468)
 - Don't capture zero size screenshots ([#2459](https://github.com/getsentry/sentry-cocoa/pull/2459))
 - Use the preexisting app release version format for profiles (#2470)
+- Remove `SentrySystemEventBreadcrumbs` observers with the most specific detail possible (#2489)
 
 ### Breaking Changes
 

diff --git a/Sources/Sentry/SentryAutoBreadcrumbTrackingIntegration.m b/Sources/Sentry/SentryAutoBreadcrumbTrackingIntegration.m
@@ -29,11 +29,12 @@ - (BOOL)installWithOptions:(nonnull SentryOptions *)options
              breadcrumbTracker:[[SentryBreadcrumbTracker alloc]
                                    initWithSwizzleWrapper:[SentryDependencyContainer sharedInstance]
                                                               .swizzleWrapper]
-        systemEventBreadcrumbs:[[SentrySystemEventBreadcrumbs alloc]
-                                      initWithFileManager:[SentryDependencyContainer sharedInstance]
-                                                              .fileManager
-                                   andCurrentDateProvider:[SentryDefaultCurrentDateProvider
-                                                              sharedInstance]]];
+        systemEventBreadcrumbs:
+            [[SentrySystemEventBreadcrumbs alloc]
+                         initWithFileManager:[SentryDependencyContainer sharedInstance].fileManager
+                      andCurrentDateProvider:[SentryDefaultCurrentDateProvider sharedInstance]
+                andNotificationCenterWrapper:[SentryDependencyContainer sharedInstance]
+                                                 .notificationCenterWrapper]];
 
     return YES;
 }

diff --git a/Sources/Sentry/SentrySystemEventBreadcrumbs.m b/Sources/Sentry/SentrySystemEventBreadcrumbs.m
@@ -3,6 +3,7 @@
 #import "SentryCurrentDateProvider.h"
 #import "SentryDependencyContainer.h"
 #import "SentryLog.h"
+#import "SentryNSNotificationCenterWrapper.h"
 #import "SentrySDK.h"
 
 // all those notifications are not available for tvOS
@@ -14,16 +15,19 @@
 SentrySystemEventBreadcrumbs ()
 @property (nonatomic, strong) SentryFileManager *fileManager;
 @property (nonatomic, strong) id<SentryCurrentDateProvider> currentDateProvider;
+@property (nonatomic, strong) SentryNSNotificationCenterWrapper *notificationCenterWrapper;
 @end
 
 @implementation SentrySystemEventBreadcrumbs
 
 - (instancetype)initWithFileManager:(SentryFileManager *)fileManager
              andCurrentDateProvider:(id<SentryCurrentDateProvider>)currentDateProvider
+       andNotificationCenterWrapper:(SentryNSNotificationCenterWrapper *)notificationCenterWrapper
 {
     if (self = [super init]) {
         _fileManager = fileManager;
         _currentDateProvider = currentDateProvider;
+        _notificationCenterWrapper = notificationCenterWrapper;
     }
     return self;
 }
@@ -41,11 +45,30 @@ - (void)start
 - (void)stop
 {
 #if TARGET_OS_IOS
-    NSNotificationCenter *defaultCenter = [NSNotificationCenter defaultCenter];
-    [defaultCenter removeObserver:self];
+    // Remove the observers with the most specific detail possible, see
+    // https://developer.apple.com/documentation/foundation/nsnotificationcenter/1413994-removeobserver
+    [self.notificationCenterWrapper removeObserver:self name:UIKeyboardDidShowNotification];
+    [self.notificationCenterWrapper removeObserver:self name:UIKeyboardDidHideNotification];
+    [self.notificationCenterWrapper removeObserver:self
+                                              name:UIApplicationUserDidTakeScreenshotNotification];
+    [self.notificationCenterWrapper removeObserver:self
+                                              name:UIDeviceBatteryLevelDidChangeNotification];
+    [self.notificationCenterWrapper removeObserver:self
+                                              name:UIDeviceBatteryStateDidChangeNotification];
+    [self.notificationCenterWrapper removeObserver:self
+                                              name:UIDeviceOrientationDidChangeNotification];
+    [self.notificationCenterWrapper removeObserver:self
+                                              name:UIDeviceOrientationDidChangeNotification];
 #endif
 }
 
+- (void)dealloc
+{
+    // In dealloc it's safe to unsubscribe for all, see
+    // https://developer.apple.com/documentation/foundation/nsnotificationcenter/1413994-removeobserver
+    [self.notificationCenterWrapper removeObserver:self];
+}
+
 #if TARGET_OS_IOS
 /**
  * Only used for testing, call start() instead.

diff --git a/Sources/Sentry/include/SentrySystemEventBreadcrumbs.h b/Sources/Sentry/include/SentrySystemEventBreadcrumbs.h
@@ -6,11 +6,14 @@
 #    import <UIKit/UIKit.h>
 #endif
 
+@class SentryNSNotificationCenterWrapper;
+
 @interface SentrySystemEventBreadcrumbs : NSObject
 SENTRY_NO_INIT
 
 - (instancetype)initWithFileManager:(SentryFileManager *)fileManager
-             andCurrentDateProvider:(id<SentryCurrentDateProvider>)currentDateProvider;
+             andCurrentDateProvider:(id<SentryCurrentDateProvider>)currentDateProvider
+       andNotificationCenterWrapper:(SentryNSNotificationCenterWrapper *)notificationCenterWrapper;
 
 - (void)start;
 

diff --git a/...s/SentryTests/Integrations/Breadcrumbs/SentryAutoBreadcrumbTrackingIntegrationTests.swift b/...s/SentryTests/Integrations/Breadcrumbs/SentryAutoBreadcrumbTrackingIntegrationTests.swift
@@ -26,7 +26,7 @@ class SentryAutoBreadcrumbTrackingIntegrationTests: XCTestCase {
     func testInstallWithSwizzleEnabled_StartSwizzleCalled() {
         let sut = fixture.sut
 
-        sut.install(with: Options(), breadcrumbTracker: fixture.tracker, systemEventBreadcrumbs: SentrySystemEventBreadcrumbs(fileManager: SentryDependencyContainer.sharedInstance().fileManager, andCurrentDateProvider: DefaultCurrentDateProvider.sharedInstance()))
+        sut.install(with: Options(), breadcrumbTracker: fixture.tracker, systemEventBreadcrumbs: SentrySystemEventBreadcrumbs(fileManager: SentryDependencyContainer.sharedInstance().fileManager, andCurrentDateProvider: DefaultCurrentDateProvider.sharedInstance(), andNotificationCenterWrapper: SentryNSNotificationCenterWrapper()))
 
         XCTAssertEqual(1, fixture.tracker.startInvocations.count)
         XCTAssertEqual(1, fixture.tracker.startSwizzleInvocations.count)
@@ -37,7 +37,7 @@ class SentryAutoBreadcrumbTrackingIntegrationTests: XCTestCase {
 
         let options = Options()
         options.enableSwizzling = false
-        sut.install(with: options, breadcrumbTracker: fixture.tracker, systemEventBreadcrumbs: SentrySystemEventBreadcrumbs(fileManager: SentryDependencyContainer.sharedInstance().fileManager, andCurrentDateProvider: DefaultCurrentDateProvider.sharedInstance()))
+        sut.install(with: options, breadcrumbTracker: fixture.tracker, systemEventBreadcrumbs: SentrySystemEventBreadcrumbs(fileManager: SentryDependencyContainer.sharedInstance().fileManager, andCurrentDateProvider: DefaultCurrentDateProvider.sharedInstance(), andNotificationCenterWrapper: SentryNSNotificationCenterWrapper()))
 
         XCTAssertEqual(1, fixture.tracker.startInvocations.count)
         XCTAssertEqual(0, fixture.tracker.startSwizzleInvocations.count)

diff --git a/Tests/SentryTests/Integrations/Breadcrumbs/SentrySystemEventBreadcrumbsTest.swift b/Tests/SentryTests/Integrations/Breadcrumbs/SentrySystemEventBreadcrumbsTest.swift
@@ -10,6 +10,7 @@ class SentrySystemEventBreadcrumbsTest: XCTestCase {
         let options: Options
         let fileManager: TestFileManager
         var currentDateProvider = TestCurrentDateProvider()
+        let notificationCenterWrapper = TestNSNotificationCenterWrapper()
 
         init() {
             options = Options()
@@ -26,7 +27,11 @@ class SentrySystemEventBreadcrumbsTest: XCTestCase {
             let hub = SentryHub(client: client, andScope: scope)
             SentrySDK.setCurrentHub(hub)
 
-            let systemEvents = SentrySystemEventBreadcrumbs(fileManager: fileManager, andCurrentDateProvider: currentDateProvider)!
+            let systemEvents = SentrySystemEventBreadcrumbs(
+                fileManager: fileManager,
+                andCurrentDateProvider: currentDateProvider,
+                andNotificationCenterWrapper: notificationCenterWrapper
+            )!
             systemEvents.start(currentDevice)
             return systemEvents
         }
@@ -266,6 +271,13 @@ class SentrySystemEventBreadcrumbsTest: XCTestCase {
         }
     }
 
+    func testStopCallsSpecificRemoveObserverMethods() {
+        let scope = Scope()
+        sut = fixture.getSut(scope: scope, currentDevice: nil)
+        sut.stop()
+        XCTAssertEqual(fixture.notificationCenterWrapper.removeObserverWithNameInvocations.count, 7)
+    }
+
     private func assertBreadcrumbAction(scope: Scope, action: String, checks: (([String: Any]) -> Void)? = nil) {
         let ser = scope.serialize()
         if let breadcrumbs = ser["breadcrumbs"] as? [[String: Any]] {

diff --git a/Tests/SentryTests/Integrations/SentryCrash/SentryCrashScopeObserverTests.swift b/Tests/SentryTests/Integrations/SentryCrash/SentryCrashScopeObserverTests.swift
@@ -297,7 +297,7 @@ class SentryCrashScopeObserverTests: XCTestCase {
         return jsonPointer!.pointee
     }
 
-    private func getScopeJson(getField: (SentryCrashScope)-> UnsafeMutablePointer<CChar>?) -> String? {
+    private func getScopeJson(getField: (SentryCrashScope) -> UnsafeMutablePointer<CChar>?) -> String? {
         guard let scopePointer = sentrycrash_scopesync_getScope() else {
             return nil
         }

diff --git a/Tests/SentryTests/SentryScopeSwiftTests.swift b/Tests/SentryTests/SentryScopeSwiftTests.swift
@@ -221,7 +221,7 @@ class SentryScopeSwiftTests: XCTestCase {
 
     func testUseSpanForClear() {
         fixture.scope.span = fixture.transaction
-        fixture.scope.useSpan { (span) in
+        fixture.scope.useSpan { (_) in
             self.fixture.scope.span = nil
         }
         XCTAssertNil(fixture.scope.span)