This change fixes currently broken ReactContext listeners mechanism.

[dryganets]

This mechanism is heavily abused inside of the react-native and inside of the various native modules.
The main problem is that people don't remove their listeners and as result, we have memory leaks.

Some modules like UIManager, NativeAnimatedModule have resources holding Activity context. Those modules are held through a pretty long chain of dependencies.

In order to allow GC to collect those listeners, I replaced the CopyOnWriteSet by WeakHashMap and synchronized access. It is not such a big deal in terms of performance as those listeners are not called/modified too frequently but this prevents hard to debug memory leaks.

Test Plan:

Make a module which reset the JS engine. Use this module to validate the count of activities you have on reset.
You could use LeakCannery to verify there is no memory leaks.
As an addition, you could you a small memory monitor:

watch adb shell dumpsys meminfo YOUR_PACKAGE_NAME

Changelog:

Help reviewers and the release process by writing your own changelog entry. When the change doesn't impact React Native developers, it may be ommitted from the changelog for brevity. See below for an example.

[Android] [Fixed] - ReactContext - lifecycle listeners don't cause the leaks even if not removed.

[pull-bot]

	Warnings
⚠️	📋 Release Notes - This PR appears to be missing Release Notes.

Generated by 🚫 dangerJS

[facebook-github-bot]

@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[dryganets]

@hramos, there is a side effect of this change that I missed.
If anyone will modify the collection of listeners from the listener itself it will cause the crash with ConcurrentModificationException.

Some modules remove the listeners in the onHostDestroy method.
There is a workaround for that to change the onHostDestroy to return boolean and use the iterator.remove() inside but this is going to be a breaking change and not completely obvious.

Another way of fixing it is to make a modification detection and delay the operations on collection until the iteration is done.

[dryganets]

The latest commit addresses removeListener scenario from the listener itself.

CopyOnWriteArraySet supports concurrent modification during iteration while WeakHashMap doesn't.
In order to fix the problem, I'm postponing the modifications till the iteration is complete.

There are some third party views removing the listener in the onHostDestroy callback.
An example is Airbnb Maps view.

Without this tweak, the change is breaking.
it is possible to work it around with ViewManager createViewInstance/onDropViewInstance but I think it is better to have a non-breaking change instead.

[facebook-github-bot]

@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[hramos]

Approved, pending results of our internal performance tests.

[matthargett]

@hramos looks like the import failed? any feedback for the contributor?

[hramos]

The import went fine and it's already approved. I never heard back re: performance tests, I'll ping again.

[react-native-bot]

This pull request was successfully merged by @dryganets in 972f399.

^{When will my fix make it into a release? | Upcoming Releases}

[cpojer]

@dryganets It seems this PR causes a crash in production for us at FB. Concretely we are getting
android_crash:java.util.ConcurrentModificationException:com.facebook.react.bridge.SynchronizedWeakHashSet.iterate when resuming an activity which blames back to this PR.

Is there any chance you could send a PR with a fix for this within the next few days? Otherwise unfortunately we'll have to revert this PR and wait for a proper fix later.

[dryganets]

@cpojer, could you share a stack trace?

[fkgozali]

This is roughly the top of the frames:

android.app.ActivityThread.performResumeActivity(ActivityThread.java:3139)
android.app.ActivityThread.handleResumeActivity(ActivityThread.java:3170)
android.app.ActivityThread$H.handleMessage(ActivityThread.java:1420)
android.os.Handler.dispatchMessage(Handler.java:102)
android.os.Looper.loop(Looper.java:150)
android.app.ActivityThread.main(ActivityThread.java:5621)
at java.lang.reflect.Method.invoke(Native Method)
com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:794)
com.android.internal.os.ZygoteInit.main(ZygoteInit.java:684)
Caused by: java.util.ConcurrentModificationException
java.util.WeakHashMap$HashIterator.next(WeakHashMap.java:165)
com.facebook.react.bridge.SynchronizedWeakHashSet.iterate(SynchronizedWeakHashSet.java:57)
com.facebook.react.bridge.ReactContext.onHostResume(ReactContext.java:215)
com.facebook.react.ReactInstanceManager.moveToResumedLifecycleState(ReactInstanceManager.java:660)
com.facebook.react.ReactInstanceManager.onHostResume(ReactInstanceManager.java:579)
com.facebook.react.ReactInstanceManager.onHostResume(ReactInstanceManager.java:535)

[fkgozali]

The offending code in SynchronizedWeakHashSet.java:57 was:

    synchronized (mMap) {
      // Protection from modification during iteration on the same thread
      mIterating = true;
      for (T listener: mMap.keySet()) { // <<<<<<<< here
        iterated.iterate(listener);
      }
      mIterating = false;

[fkgozali]

there is a side effect of this change that I missed.
If anyone will modify the collection of listeners from the listener itself it will cause the crash with ConcurrentModificationException.

@dryganets - perhaps the stack trace above was hitting this condition?

[dryganets]

@fkgozali, the code you mentioned protects from the ConcurrentModificationException.

The add and remove operations are queued in case of iteration.

I missed that part for Skype and had ConcurrentModificationExceptions in production because of that (For us it was a minor crash with less than 0.01% impact)

The case was - removal of the listener from the listener itself. Despite everything is synchronized - there was a ConcurrentModificationException as the listener was removed on the same thread. Iterator checks if there is a change during iteration and throws the CME ...

I'm reviewing the code now to find the root cause.
Just double checked that you have the latest version of the code with the problem we had fixed.

[dryganets]

@fkgozali a possible offender is containsKey method.
While I don't see it touching the modCount, it is still modifying method.
ie it calls the getTable() which purges the stale weak references.
And depending on implementation it might cause the ConcurrentModificationException.

Also, the version of react-native we are using in production doesn't have that contains check so it explains why we never hit a similar problem after my fix.

I'll send a PR with a fix for this gap.

[fkgozali]

Sounds good, if we can have that fix by EOD today, we can try it out in our apps and hope for the best. Otherwise, I'd have to revert the original commit just for the de-risking purpose (given the volume of the crash is increasing, but not critically high yet). If that happens, we'll work with you to get it re-landed with the fix.

[dryganets]

@fkgozali,
here is it: #24015