Bridge Invalidation breaks RNCallKeep RTCEventEmitter on recent RN versions

[jonastelzio]

Bug report

• [ ] I've checked the example to reproduce the issue.

• Reproduced on:

• [ ] Android

• [x] iOS

Description

Issue specific til RN version 0.64 (and probably some versions prior).

This issue is created mainly because I'd like to discuss how to resolve this issue before writing code that does it. I'm not an expert on writing modules for react native on IOS - I'm more experienced with android in that regard. But I do have a solution in mind.

This issue stems from changes to react native made in this commit.

Because of the way RNCallKeep.m is put together, we're running into an issue where an invalidate call across the RN bridge, causes stopObserving to be called on RCTEventEmitter, but it never calls startObserving again because of the way RCTEventEmitter.m is now put together internally.

Allow me to explain: When RNCallKeep.m is initialized, it's done so as a Singleton:

+ (id)allocWithZone:(NSZone *)zone {
    static RNCallKeep *sharedInstance = nil;
    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^{
        sharedInstance = [super allocWithZone:zone];
    });
    return sharedInstance;
}

This has a particularly unfortunate side effect - namely that dealloc is never called. This is on it's own an issue, but I'm not sure how big of an issue. Before the commit I've linked further up, react native would use dealloc to decide if it wanted to call stopObserving. This method would never be called prior to that change, because a singleton cannot be deallocated by react native during a bridge invalidation.

However, because facebook now uses invalidate to do this check, they DO call stopObserving, causing us to set

- (void)stopObserving
{
    _hasListeners = FALSE;
}

BUT, because RNCallKeep is never deallocated, the _listenerCount variable is never reinitialized to zero (see this)

Because React Native internally performs this check, and that variable will be left at whatever value it had before the reload, they NEVER call startObserving again if the class that implements RTCEventEmitter is initialized as a singleton.

Proposed solution

I propose that we remove the RTCEventEmitter from RNCallKeep.m, and instead create a new class (perhaps RNCallKeepRTCEventEmitter.m?) that implements this interface, which is NOT initialized as a singleton. Instead RNCallKeep.m will get a new static function, registerRTCEventEmitter, which we call from RNCallKeepRTCEventEmitter.m with a reference to self on startObserving, and with nil on stopObserving

This way we can keep the singleton and have RN initialize and deinitialize the RTCEventEmitter as it needs.

Thoughts, concerns, ideas?

Steps to Reproduce

Reload app in Development Mode, and all handlers stops firing.

Versions

- Callkeep: All
- React Native: 0.64
- iOS: All

[jonastelzio]

@manuquentin @danjenkins I'd like to talk about how to solve this issue before I start coding away at it.

[MrAlexWeber]

This issue is quite annoying when debugging, so I'm writing to express my support for @jonastelzio in capturing this issue. Please fix it when you're able!

[Andriiklymiuk]

Have you found the fix for this issue? it also happens with codepush update in release mode also rn 0.65.1 react native codepush 7.0.1

[TunvirRahman]

Having the same issue in RN 0.66.3 and XCode 13. Have anyone fixed the issue?

[Romick2005]

Having the same issue in RN 0.66.3 and XCode 13.

Have anyone fixed the issue?

I've solve this hot reload issue for dev mode singleton initialization like this:

[cristianoccazinsp]

Observing the same issue (pun intended).

My work around was to screw this "has listeners" optimizations and fire events regardless (as opposed to what the RN recommends). Most of the time the events will have a listener, at least for the module I'm working on.

[Gongreg]

Sharing my workaround: We were using codepush/expo updates and in our production app callkeep didn't work after restarting the app due to update. To solve it before restarting the app I unsubscribe all the events on JS side with

  //Remove all possible react-native-callkeep events that we might subscribe to
  const RNCallKeep = require("react-native-callkeep").default

  ;[
    "didReceiveStartCallAction",
    "answerCall",
    "endCall",
    "didActivateAudioSession",
    "didDeactivateAudioSession",
    "didDisplayIncomingCall",
    "didPerformSetMutedCallAction",
    "didToggleHoldCallAction",
    "didPerformDTMFAction",
    "didResetProvider",
    "checkReachability",
    "didLoadWithEvents",
    "showIncomingCallUi",
  ].forEach((eventType) => RNCallKeep.removeEventListener(eventType))

This way the listener count is reset to 0 in native code and the when the app is restarted the callkeep startObserving is executed.

[glesperance]

@Gongreg We are having the same issue. Could you give more details about your implementation? It"s not exactly clear when one must call the above, especially in dev mode. App center / codepush doesnt seem to provide a convenient way to achieve the above either.

Right now, the only solution that is working for us is to do what @cristianoccazinsp suggested that is:

 - (void)stopObserving
 {
-     _hasListeners = FALSE;
+    //_hasListeners = FALSE;
 }

However, the ramifications of doing this aren"t clear at this moment

[Gongreg]

Hey @glesperance , my implementation does not take care of the issue inside dev mode. It is only relevant in case of app updates (reloading JS code in production with codepush/expo updates).

Regarding //_hasListeners = FALSE; change sadly i don't know the implications of it.

[Romick2005]

Sharing my workaround: We were using codepush/expo updates and in our production app callkeep didn't work after restarting the app due to update. To solve it before restarting the app I unsubscribe all the events on JS side with

  //Remove all possible react-native-callkeep events that we might subscribe to
  const RNCallKeep = require("react-native-callkeep").default

  ;[
    "didReceiveStartCallAction",
    "answerCall",
    "endCall",
    "didActivateAudioSession",
    "didDeactivateAudioSession",
    "didDisplayIncomingCall",
    "didPerformSetMutedCallAction",
    "didToggleHoldCallAction",
    "didPerformDTMFAction",
    "didResetProvider",
    "checkReachability",
    "didLoadWithEvents",
    "showIncomingCallUi",
  ].forEach((eventType) => RNCallKeep.removeEventListener(eventType))

This way the listener count is reset to 0 in native code and the when the app is restarted the callkeep startObserving is executed.

What do you mean by "restart"? Doesn't it a common practice to unsubscribe from all subscribed listeners on app start/close and component mount/unmount? In normal restart (app close and then open) all listeners counters will be reset. So for this case I think you do not need to call a function to remove all listeners. But I assume your case is not to close app completely, but just do code update and refresh screen?

[Gongreg]

@Romick2005 . This issue is relevant in two cases: DEV mode & over the air (OTA) updates in production.

In our case we don't really care about DEV mode issue, since it does not impact users.

We do care about OTA scenario. We were using codepush (now switched to expo updates). When you are updating the JS code (https://docs.expo.dev/versions/latest/sdk/updates/#updatesreloadasync) it does not kill the app, it only invalidates the JS brigde and starts a new one. So in that case the workaround I added above helps to solve the issue.

[cristianoccazinsp]

The JS bridge can be invalidated in other scenarios too. For example, if you change app permissions from settings, on android the activity restarts, which is not a full app reload but the JS bridge reloads as well.

[glesperance]

Although it seems this is a iOS issue only I agree with @cristianoccazinsp.

We can't rely on the event handlers always being cleanly unregistered.

In our use case, the app itself is always listening to callkeep so -- in the absence of time to research and understand the internals enough to do the refactor -- I will just go with commenting out the stopObserving function alltogether. The only time this would be a false positive is when the app is between restarts.

My underdtanding is that OP is right; this class should be refactored not to be a singleton.

ADDITION/EDIT: I also think that the callkeep framework should never be in a broken (internal) state -- independently of the developper's behaviour.

[glesperance]

Alright, I found 1 middle ground solutions, that maintains a saner RNCallKeep state:

Use Objective-C Key Value Coding(KVC) to sync RTCEventEmitter _listenerCount.

 - (void)stopObserving
 {
     _hasListeners = FALSE;
+
+    // Fix for https://github.com/react-native-webrtc/react-native-callkeep/issues/406
+    // We use Objective-C Key Value Coding(KVC) to sync _RTCEventEmitter_ `_listenerCount`.
+    @try {
+        [self setValue:@0 forKey:@"_listenerCount"];
+    }
+    @catch ( NSException *e ){
+        NSLog(@"[RNCallKeep][stopObserving] exception: %@",e);
+        NSLog(@"[RNCallKeep][stopObserving] RNCallKeep parent class RTCEventEmitter might have a broken state.");
+        NSLog(@"[RNCallKeep][stopObserving] Please verify that the parent RTCEventEmitter.m has iVar `_listenerCount`.");
+    }
 }

This is much more cost-efficient & less risky than undergoing a refactor.

EDIT: Removed the second solution (https://github.com/react-native-webrtc/react-native-callkeep/pull/575) as after re-evaluation it dawned on me that it would be prone to breaking the apps if RTCEventEmitter changed its internal structure

[kp198]

Hey Does this cause a crash for you guys in RCTEventEmitter?

[glesperance]

@kp198 could you clarify your question? Are you talking about the fix itself or asking if we're still experiencing crashes?

[kp198]

@glesperance I was asking if you meant app crashes when you had mentioned Bridge Invalidation breaks RNCallKeep RTCEventEmitter on recent RN versions i.e what exactly does breaks mean? Is it a crash or some unexpected behaviour?

[glesperance]

@kp198 to quote OP:

the way RNCallKeep.m is put together, we're running into an issue where an invalidate call across the RN bridge, causes stopObserving to be called on RCTEventEmitter, but it never calls startObserving again because of the way RCTEventEmitter.m is now put together internally.

The material effect that this had in our case was that Android could no longer receive notifications when in killed state.

https://github.com/react-native-webrtc/react-native-callkeep/pull/574 fixes that issue using KVC.

Let me know if you need more info :)

[frozencap]

@Gongreg when exactly is "before restarting the app" ? you're doing the CodePush routine manually, not using codePush.sync I suppose?

[Gongreg]

@shawarmaz - we are not using codepush anymore. But the idea remains the same. I think you could listen for the status update callback to do the workaround.

[AbdulBsit]

Any updates on this, facing this issue when using with codepush, user opt for later restart and when call arrives after that, it not received any event due to bridge invalidation, thus results in call failed

[gormfrederiksen]

I am now experiencing this problem as well. After an expo OTA the JS application no longer receives RNCallKeep events (I tried the above solution with unregistering the listeners)

[AbdulBsit]

@gormfrederiksen I fixed it with this patch on v4.3.8

diff --git a/node_modules/react-native-callkeep/ios/RNCallKeep/RNCallKeep.m b/node_modules/react-native-callkeep/ios/RNCallKeep/RNCallKeep.m
index e67c3cd..1da65f8 100644
--- a/node_modules/react-native-callkeep/ios/RNCallKeep/RNCallKeep.m
+++ b/node_modules/react-native-callkeep/ios/RNCallKeep/RNCallKeep.m
@@ -131,6 +131,17 @@ RCT_EXPORT_MODULE()
 - (void)stopObserving
 {
     _hasListeners = FALSE;
+
+    // Fix for https://github.com/react-native-webrtc/react-native-callkeep/issues/406
+    // We use Objective-C Key Value Coding(KVC) to sync _RTCEventEmitter_ `_listenerCount`.
+    @try {
+        [self setValue:@0 forKey:@"_listenerCount"];
+    } 
+    @catch ( NSException *e ){
+        NSLog(@"[RNCallKeep][stopObserving] exception: %@",e);
+        NSLog(@"[RNCallKeep][stopObserving] RNCallKeep parent class RTCEventEmitter might have a broken state.");
+        NSLog(@"[RNCallKeep][stopObserving] Please verify that the parent RTCEventEmitter.m has iVar `_listenerCount`.");
+    }
 }
 
 - (void)onAudioRouteChange:(NSNotification *)notification

[manuquentin]

Thanks @AbdulBsit, can you make a pull request for that please ?

[glesperance]

@manuquentin the PR has been made about a year ago #574

[nbonatsakis]

@manuquentin We are using the latest version of this library and seeing that after a reload of the JS bundle (in particular in the case of an Expo update), listeners stop working. I was reading through the source code and ended up on this issue based on that code comment. Was this fix trying to address this problem? Is there something else we have to do in our JS code to resolve the issue?