From 23717e48aff3d7fdaea30c9b8dcdd6cfbb7802d5 Mon Sep 17 00:00:00 2001 From: Scott Kyle Date: Mon, 21 Sep 2020 17:28:47 -0700 Subject: [PATCH] Call stopObserving on correct queue Summary: Since `dealloc` can be called from any thread, this would result `stopObserving` being called on a different thread/queue than the specified `methodQueue`. We specifically encountered this issue with a module needing the main queue having its `stopObserving` called on a background queue. Changelog: [iOS][Fixed] - Call [RCTEventEmitter stopObserving] on specified method queue Reviewed By: RSNara Differential Revision: D23821741 fbshipit-source-id: 693c3be6876f863da6dd214a829af2cc13a09c3f --- Libraries/NativeAnimation/RCTNativeAnimatedModule.mm | 1 + Libraries/NativeAnimation/RCTNativeAnimatedTurboModule.mm | 1 + Libraries/Network/RCTNetworking.mm | 4 +++- React/CoreModules/RCTAppState.h | 2 +- React/CoreModules/RCTAppState.mm | 5 ----- React/CoreModules/RCTDevSettings.mm | 1 + React/CoreModules/RCTWebSocketModule.mm | 2 ++ React/Modules/RCTEventEmitter.h | 4 +++- React/Modules/RCTEventEmitter.m | 2 +- 9 files changed, 13 insertions(+), 9 deletions(-) diff --git a/Libraries/NativeAnimation/RCTNativeAnimatedModule.mm b/Libraries/NativeAnimation/RCTNativeAnimatedModule.mm index 35adf39289aa07..33fe87ad4d4a10 100644 --- a/Libraries/NativeAnimation/RCTNativeAnimatedModule.mm +++ b/Libraries/NativeAnimation/RCTNativeAnimatedModule.mm @@ -46,6 +46,7 @@ - (instancetype)init - (void)invalidate { + [super invalidate]; [_nodesManager stopAnimationLoop]; [self.bridge.eventDispatcher removeDispatchObserver:self]; [self.bridge.uiManager.observerCoordinator removeObserver:self]; diff --git a/Libraries/NativeAnimation/RCTNativeAnimatedTurboModule.mm b/Libraries/NativeAnimation/RCTNativeAnimatedTurboModule.mm index 6fc594872f874b..45213380d81d7a 100644 --- a/Libraries/NativeAnimation/RCTNativeAnimatedTurboModule.mm +++ b/Libraries/NativeAnimation/RCTNativeAnimatedTurboModule.mm @@ -47,6 +47,7 @@ - (instancetype)init - (void)invalidate { + [super invalidate]; [_nodesManager stopAnimationLoop]; [self.bridge.eventDispatcher removeDispatchObserver:self]; [self.bridge.uiManager.observerCoordinator removeObserver:self]; diff --git a/Libraries/Network/RCTNetworking.mm b/Libraries/Network/RCTNetworking.mm index b857c43a9086e4..c4dbf8a7bf3364 100644 --- a/Libraries/Network/RCTNetworking.mm +++ b/Libraries/Network/RCTNetworking.mm @@ -168,6 +168,8 @@ - (instancetype)initWithHandlersProvider:(NSArray> * (^ - (void)invalidate { + [super invalidate]; + for (NSNumber *requestID in _tasksByRequestID) { [_tasksByRequestID[requestID] cancel]; } @@ -680,7 +682,7 @@ - (RCTNetworkTask *)networkTaskWithRequest:(NSURLRequest *)request completionBlo @"timeout": @(query.timeout()), @"withCredentials": @(query.withCredentials()), }; - + // TODO: buildRequest returns a cancellation block, but there's currently // no way to invoke it, if, for example the request is cancelled while // loading a large file to build the request body diff --git a/React/CoreModules/RCTAppState.h b/React/CoreModules/RCTAppState.h index 6e0f19c3356f65..0921f70e44723b 100644 --- a/React/CoreModules/RCTAppState.h +++ b/React/CoreModules/RCTAppState.h @@ -7,6 +7,6 @@ #import -@interface RCTAppState : RCTEventEmitter +@interface RCTAppState : RCTEventEmitter @end diff --git a/React/CoreModules/RCTAppState.mm b/React/CoreModules/RCTAppState.mm index 66a1e72693f2f6..b0dc10bce5ad6d 100644 --- a/React/CoreModules/RCTAppState.mm +++ b/React/CoreModules/RCTAppState.mm @@ -99,11 +99,6 @@ - (void)stopObserving [[NSNotificationCenter defaultCenter] removeObserver:self]; } -- (void)invalidate -{ - [self stopObserving]; -} - #pragma mark - App Notification Methods - (void)handleMemoryWarning diff --git a/React/CoreModules/RCTDevSettings.mm b/React/CoreModules/RCTDevSettings.mm index 50d9cd0cde8447..4bf03630201fe0 100644 --- a/React/CoreModules/RCTDevSettings.mm +++ b/React/CoreModules/RCTDevSettings.mm @@ -202,6 +202,7 @@ - (dispatch_queue_t)methodQueue - (void)invalidate { + [super invalidate]; #if ENABLE_PACKAGER_CONNECTION [[RCTPackagerConnection sharedPackagerConnection] removeHandler:_reloadToken]; #endif diff --git a/React/CoreModules/RCTWebSocketModule.mm b/React/CoreModules/RCTWebSocketModule.mm index e26423d245438b..3bc43d797f65d6 100644 --- a/React/CoreModules/RCTWebSocketModule.mm +++ b/React/CoreModules/RCTWebSocketModule.mm @@ -53,6 +53,8 @@ - (NSArray *)supportedEvents - (void)invalidate { + [super invalidate]; + _contentHandlers = nil; for (RCTSRWebSocket *socket in _sockets.allValues) { socket.delegate = nil; diff --git a/React/Modules/RCTEventEmitter.h b/React/Modules/RCTEventEmitter.h index 700368c599d016..f04d50f02418e4 100644 --- a/React/Modules/RCTEventEmitter.h +++ b/React/Modules/RCTEventEmitter.h @@ -12,7 +12,7 @@ * RCTEventEmitter is an abstract base class to be used for modules that emit * events to be observed by JS. */ -@interface RCTEventEmitter : NSObject +@interface RCTEventEmitter : NSObject @property (nonatomic, weak) RCTBridge *bridge; @@ -37,6 +37,8 @@ - (void)startObserving; - (void)stopObserving; +- (void)invalidate NS_REQUIRES_SUPER; + - (void)addListener:(NSString *)eventName; - (void)removeListeners:(double)count; diff --git a/React/Modules/RCTEventEmitter.m b/React/Modules/RCTEventEmitter.m index 7717ddbdcac451..6e5a6ff37e7f5d 100644 --- a/React/Modules/RCTEventEmitter.m +++ b/React/Modules/RCTEventEmitter.m @@ -78,7 +78,7 @@ - (void)stopObserving // Does nothing } -- (void)dealloc +- (void)invalidate { if (_listenerCount > 0) { [self stopObserving];