fix: Remove SentrySystemEventBreadcrumbs observers with the most specific detail possible

CHANGELOG.md

@@ -16,6 +16,7 @@ This version adds a dependency on Swift.
 - `SentryAppStateManager` correctly unsubscribes from `NSNotificationCenter` when closing the SDK (#2460)
 - The SDK no longer reports an OOM when a crash happens after closing the SDK (#2468)
 - Use the preexisting app release version format for profiles (#2470)
+- Remove `SentrySystemEventBreadcrumbs` observers with the most specific detail possible (#2489)
 
 ### Breaking Changes
 

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;
 }

Sources/Sentry/SentryNSNotificationCenterWrapper.m

@@ -51,6 +51,17 @@ - (void)addObserver:(id)observer selector:(SEL)aSelector name:(NSNotificationNam
                                              object:nil];
 }
 
+- (void)addObserver:(id)observer
+           selector:(SEL)aSelector
+               name:(NSNotificationName)aName
+             object:(id)anObject
+{
+    [NSNotificationCenter.defaultCenter addObserver:observer
+                                           selector:aSelector
+                                               name:aName
+                                             object:anObject];
+}
+
 - (void)removeObserver:(id)observer name:(NSNotificationName)aName
 {
     [NSNotificationCenter.defaultCenter removeObserver:observer name:aName object:nil];

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.
@@ -72,18 +95,17 @@ - (void)initBatteryObserver:(UIDevice *)currentDevice
         currentDevice.batteryMonitoringEnabled = YES;
     }
 
-    NSNotificationCenter *defaultCenter = [NSNotificationCenter defaultCenter];
-
     // Posted when the battery level changes.
-    [defaultCenter addObserver:self
-                      selector:@selector(batteryStateChanged:)
-                          name:UIDeviceBatteryLevelDidChangeNotification
-                        object:currentDevice];
+    [self.notificationCenterWrapper addObserver:self
+                                       selector:@selector(batteryStateChanged:)
+                                           name:UIDeviceBatteryLevelDidChangeNotification
+                                         object:currentDevice];
+
     // Posted when battery state changes.
-    [defaultCenter addObserver:self
-                      selector:@selector(batteryStateChanged:)
-                          name:UIDeviceBatteryStateDidChangeNotification
-                        object:currentDevice];
+    [self.notificationCenterWrapper addObserver:self
+                                       selector:@selector(batteryStateChanged:)
+                                           name:UIDeviceBatteryStateDidChangeNotification
+                                         object:currentDevice];
 }
 
 - (void)batteryStateChanged:(NSNotification *)notification
@@ -132,10 +154,10 @@ - (void)initOrientationObserver:(UIDevice *)currentDevice
     }
 
     // Posted when the orientation of the device changes.
-    [[NSNotificationCenter defaultCenter] addObserver:self
-                                             selector:@selector(orientationChanged:)
-                                                 name:UIDeviceOrientationDidChangeNotification
-                                               object:currentDevice];
+    [self.notificationCenterWrapper addObserver:self
+                                       selector:@selector(orientationChanged:)
+                                           name:UIDeviceOrientationDidChangeNotification
+                                         object:currentDevice];
 }
 
 - (void)orientationChanged:(NSNotification *)notification
@@ -163,18 +185,15 @@ - (void)orientationChanged:(NSNotification *)notification
 
 - (void)initKeyboardVisibilityObserver
 {
-    NSNotificationCenter *defaultCenter = [NSNotificationCenter defaultCenter];
     // Posted immediately after the display of the keyboard.
-    [defaultCenter addObserver:self
-                      selector:@selector(systemEventTriggered:)
-                          name:UIKeyboardDidShowNotification
-                        object:nil];
+    [self.notificationCenterWrapper addObserver:self
+                                       selector:@selector(systemEventTriggered:)
+                                           name:UIKeyboardDidShowNotification];
 
     // Posted immediately after the dismissal of the keyboard.
-    [defaultCenter addObserver:self
-                      selector:@selector(systemEventTriggered:)
-                          name:UIKeyboardDidHideNotification
-                        object:nil];
+    [self.notificationCenterWrapper addObserver:self
+                                       selector:@selector(systemEventTriggered:)
+                                           name:UIKeyboardDidHideNotification];
 }
 
 - (void)systemEventTriggered:(NSNotification *)notification
@@ -189,10 +208,9 @@ - (void)systemEventTriggered:(NSNotification *)notification
 - (void)initScreenshotObserver
 {
     // it's only about the action, but not the SS itself
-    [[NSNotificationCenter defaultCenter] addObserver:self
-                                             selector:@selector(systemEventTriggered:)
-                                                 name:UIApplicationUserDidTakeScreenshotNotification
-                                               object:nil];
+    [self.notificationCenterWrapper addObserver:self
+                                       selector:@selector(systemEventTriggered:)
+                                           name:UIApplicationUserDidTakeScreenshotNotification];
 }
 
 - (void)initTimezoneObserver
@@ -208,10 +226,9 @@ - (void)initTimezoneObserver
     }
 
     // Posted when the timezone of the device changed
-    [[NSNotificationCenter defaultCenter] addObserver:self
-                                             selector:@selector(timezoneEventTriggered)
-                                                 name:NSSystemTimeZoneDidChangeNotification
-                                               object:nil];
+    [self.notificationCenterWrapper addObserver:self
+                                       selector:@selector(timezoneEventTriggered)
+                                           name:NSSystemTimeZoneDidChangeNotification];
 }
 
 - (void)timezoneEventTriggered

Sources/Sentry/include/SentryNSNotificationCenterWrapper.h

@@ -20,6 +20,11 @@ NS_ASSUME_NONNULL_BEGIN
 
 - (void)addObserver:(id)observer selector:(SEL)aSelector name:(NSNotificationName)aName;
 
+- (void)addObserver:(id)observer
+           selector:(SEL)aSelector
+               name:(NSNotificationName)aName
+             object:(id)anObject;
+
 - (void)removeObserver:(id)observer name:(NSNotificationName)aName;
 
 - (void)removeObserver:(id)observer;

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;
 

Tests/SentryTests/Helper/TestNSNotificationCenterWrapper.swift

@@ -8,15 +8,18 @@ import Foundation
 
     public override func addObserver(_ observer: Any, selector aSelector: Selector, name aName: NSNotification.Name) {
         addObserverInvocations.record((observer, aSelector, aName))
+        NotificationCenter.default.addObserver(observer, selector: aSelector, name: aName, object: nil)
     }
 
     var removeObserverWithNameInvocations = Invocations<(observer: Any, name: NSNotification.Name)>()
     public override func removeObserver(_ observer: Any, name aName: NSNotification.Name) {
         removeObserverWithNameInvocations.record((observer, aName))
+        NotificationCenter.default.removeObserver(observer, name: aName, object: nil)
     }
 
     var removeObserverInvocations = Invocations<Any>()
     public override func removeObserver(_ observer: Any) {
         removeObserverInvocations.record(observer)
+        NotificationCenter.default.removeObserver(observer)
     }
 }

Tests/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)

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]] {

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
         }

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)