react-native-webrtc / react-native-webrtc/react-native-callkeep

Bridge Invalidation breaks RNCallKeep RTCEventEmitter on recent RN versions

Open
#406 26 comments 9 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Java
Stars
1.1k
Forks
511
Avg merge
9h 12m
Merged PRs (30d)
2

Description

Bug report

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

  • Reproduced on:

  • Android

  • 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

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with RNCallKeep.m and the React Native RCTEventEmitter changes linked in the issue, focusing on bridge invalidation, stopObserving, and singleton lifecycle. Reproduce by reloading the app in Development Mode on iOS, then evaluate the proposed separate RNCallKeepRTCEventEmitter class and registration flow. Done means event handlers fire again after a reload.

Written by the indexing model from the issue text.

Assessment

Tech stack
objective-c, react-native
Domain
mobile
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.