Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

This change fixes currently broken ReactContext listeners mechanism. #22318

Conversation

dryganets
Copy link
Contributor

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.

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 (required)
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
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 16, 2018
@pull-bot
Copy link

Warnings
⚠️

📋 Release Notes - This PR appears to be missing Release Notes.

Generated by 🚫 dangerJS

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dryganets
Copy link
Contributor Author

@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
Copy link
Contributor Author

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.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@hramos hramos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, pending results of our internal performance tests.

@matthargett
Copy link
Contributor

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

@hramos
Copy link
Contributor

hramos commented Feb 12, 2019

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

@react-native-bot
Copy link
Collaborator

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

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

@react-native-bot react-native-bot added the Merged This PR has been merged. label Feb 25, 2019
@cpojer
Copy link
Contributor

cpojer commented Mar 15, 2019

@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
Copy link
Contributor Author

dryganets commented Mar 15, 2019

@cpojer, could you share a stack trace?

@fkgozali
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

dryganets commented Mar 18, 2019

@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
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor Author

dryganets commented Mar 18, 2019

@fkgozali,
here is it: #24015

@elicwhite elicwhite added p: Microsoft Partner: Microsoft Partner and removed Partner p: Microsoft Partner: Microsoft labels Jul 3, 2019
facebook-github-bot pushed a commit that referenced this pull request Oct 12, 2021
Summary:
This sync includes the following changes:
- **[579c008a7](facebook/react@579c008a7 )**: [Fizz/Flight] pipeToNodeWritable(..., writable).startWriting() -> renderToPipeableStream(...).pipe(writable) ([#22450](facebook/react#22450)) //<Sebastian Markbåge>//
- **[f2c381131](facebook/react@f2c381131 )**: fix: useSyncExternalStoreExtra ([#22500](facebook/react#22500)) //<Daishi Kato>//
- **[0ecbbe142](facebook/react@0ecbbe142 )**: Sync hydrate discrete events in capture phase and dont replay discrete events ([#22448](facebook/react#22448)) //<salazarm>//
- **[a724a3b57](facebook/react@a724a3b57 )**: [RFC] Codemod invariant -> throw new Error ([#22435](facebook/react#22435)) //<Andrew Clark>//
- **[201af81b0](facebook/react@201af81b0 )**: Release pooled cache reference in complete/unwind ([#22464](facebook/react#22464)) //<Joseph Savona>//
- **[033efe731](facebook/react@033efe731 )**: Call get snapshot in useSyncExternalStore server shim ([#22453](facebook/react#22453)) //<salazarm>//
- **[7843b142a](facebook/react@7843b142a )**: [Fizz/Flight] Pass in Destination lazily to startFlowing instead of in createRequest ([#22449](facebook/react#22449)) //<Sebastian Markbåge>//
- **[d9fb383d6](facebook/react@d9fb383d6 )**: Extract queueing logic into shared functions ([#22452](facebook/react#22452)) //<Andrew Clark>//
- **[9175f4d15](facebook/react@9175f4d15 )**: Scheduling Profiler: Show Suspense resource .displayName ([#22451](facebook/react#22451)) //<Brian Vaughn>//
- **[eba248c39](facebook/react@eba248c39 )**: [Fizz/Flight] Remove reentrancy hack ([#22446](facebook/react#22446)) //<Sebastian Markbåge>//
- **[66388150e](facebook/react@66388150e )**: Remove usereducer eager bailout ([#22445](facebook/react#22445)) //<Joseph Savona>//
- **[d3e086932](facebook/react@d3e086932 )**: Make root.unmount() synchronous  ([#22444](facebook/react#22444)) //<Andrew Clark>//
- **[2cc6d79c9](facebook/react@2cc6d79c9 )**: Rename onReadyToStream to onCompleteShell ([#22443](facebook/react#22443)) //<Sebastian Markbåge>//
- **[c88fb49d3](facebook/react@c88fb49d3 )**: Improve DEV errors if string coercion throws (Temporal.*, Symbol, etc.) ([#22064](facebook/react#22064)) //<Justin Grant>//
- **[05726d72c](facebook/react@05726d72c )**: [Fix] Errors should not "unsuspend" a transition ([#22423](facebook/react#22423)) //<Andrew Clark>//
- **[3746eaf98](facebook/react@3746eaf98 )**: Packages/React/src/ReactLazy ---> changing -1 to unintialized ([#22421](facebook/react#22421)) //<BIKI DAS>//
- **[04ccc01d9](facebook/react@04ccc01d9 )**: Hydration errors should force a client render ([#22416](facebook/react#22416)) //<Andrew Clark>//
- **[029fdcebb](facebook/react@029fdcebb )**: root.hydrate -> root.isDehydrated ([#22420](facebook/react#22420)) //<Andrew Clark>//
- **[af87f5a83](facebook/react@af87f5a83 )**: Scheduling Profiler marks should include thrown Errors ([#22417](facebook/react#22417)) //<Brian Vaughn>//
- **[d47339ea3](facebook/react@d47339ea3 )**: [Fizz] Remove assignID mechanism ([#22410](facebook/react#22410)) //<Sebastian Markbåge>//
- **[3a50d9557](facebook/react@3a50d9557 )**: Never attach ping listeners in legacy Suspense ([#22407](facebook/react#22407)) //<Andrew Clark>//
- **[82c8fa90b](facebook/react@82c8fa90b )**: Add back useMutableSource temporarily ([#22396](facebook/react#22396)) //<Andrew Clark>//
- **[5b57bc6e3](facebook/react@5b57bc6e3 )**: [Draft] don't patch console during first render ([#22308](facebook/react#22308)) //<Luna Ruan>//
- **[cf07c3df1](facebook/react@cf07c3df1 )**: Delete all but one `build2` reference ([#22391](facebook/react#22391)) //<Andrew Clark>//
- **[bb0d06935](facebook/react@bb0d06935 )**: [build2 -> build] Local scripts //<Andrew Clark>//
- **[0c81d347b](facebook/react@0c81d347b )**: Write artifacts to `build` instead of `build2` //<Andrew Clark>//
- **[4da03c9fb](facebook/react@4da03c9fb )**: useSyncExternalStore React Native version ([#22367](facebook/react#22367)) //<salazarm>//
- **[48d475c9e](facebook/react@48d475c9e )**: correct typos ([#22294](facebook/react#22294)) //<Bowen>//
- **[cb6c619c0](facebook/react@cb6c619c0 )**: Remove Fiber fields that were used for hydrating useMutableSource ([#22368](facebook/react#22368)) //<Sebastian Silbermann>//
- **[64e70f82e](facebook/react@64e70f82e )**: [Fizz] add avoidThisFallback support ([#22318](facebook/react#22318)) //<salazarm>//
- **[3ee7a004e](facebook/react@3ee7a004e )**: devtools: Display actual ReactDOM API name in root type ([#22363](facebook/react#22363)) //<Sebastian Silbermann>//
- **[79b8fc667](facebook/react@79b8fc667 )**: Implement getServerSnapshot in userspace shim ([#22359](facebook/react#22359)) //<Andrew Clark>//
- **[86b3e2461](facebook/react@86b3e2461 )**: Implement useSyncExternalStore on server ([#22347](facebook/react#22347)) //<Andrew Clark>//
- **[8209de269](facebook/react@8209de269 )**: Delete useMutableSource implementation ([#22292](facebook/react#22292)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions e8feb11...afcb9cd

jest_e2e[run_all_tests]

Reviewed By: yungsters

Differential Revision: D31541359

fbshipit-source-id: c35941bc303fdf55cb061e9996200dc868a6f2af
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Microsoft Partner: Microsoft Partner Platform: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants