From 24ea3f185ba0a5445d472b26784fad495da7a417 Mon Sep 17 00:00:00 2001 From: Sergei Dryganets Date: Fri, 16 Nov 2018 11:27:42 -0800 Subject: [PATCH 1/3] This change fixes currently broken ReactContext listeners mechanism. 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/react/bridge/ReactContext.java | 95 ++++++++++++------- 1 file changed, 59 insertions(+), 36 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java index d83fe1be13db05..b77f89d986247a 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java @@ -18,7 +18,8 @@ import com.facebook.react.bridge.queue.ReactQueueConfiguration; import com.facebook.react.common.LifecycleState; import java.lang.ref.WeakReference; -import java.util.concurrent.CopyOnWriteArraySet; +import java.util.Map; +import java.util.WeakHashMap; import javax.annotation.Nullable; /** @@ -32,10 +33,10 @@ public class ReactContext extends ContextWrapper { "ReactContext#getJSModule should only happen once initialize() has been called on your " + "native module."; - private final CopyOnWriteArraySet mLifecycleEventListeners = - new CopyOnWriteArraySet<>(); - private final CopyOnWriteArraySet mActivityEventListeners = - new CopyOnWriteArraySet<>(); + private final Map mLifecycleEventListeners = + new WeakHashMap<>(); + private final Map mActivityEventListeners = + new WeakHashMap<>(); private LifecycleState mLifecycleState = LifecycleState.BEFORE_CREATE; @@ -132,7 +133,11 @@ public LifecycleState getLifecycleState() { } public void addLifecycleEventListener(final LifecycleEventListener listener) { - mLifecycleEventListeners.add(listener); + + synchronized (mLifecycleEventListeners) { + mLifecycleEventListeners.put(listener, null); + } + if (hasActiveCatalystInstance()) { switch (mLifecycleState) { case BEFORE_CREATE: @@ -143,8 +148,10 @@ public void addLifecycleEventListener(final LifecycleEventListener listener) { new Runnable() { @Override public void run() { - if (!mLifecycleEventListeners.contains(listener)) { - return; + synchronized (mLifecycleEventListeners) { + if (!mLifecycleEventListeners.containsKey(listener)) { + return; + } } try { listener.onHostResume(); @@ -161,15 +168,21 @@ public void run() { } public void removeLifecycleEventListener(LifecycleEventListener listener) { - mLifecycleEventListeners.remove(listener); + synchronized (mLifecycleEventListeners) { + mLifecycleEventListeners.remove(listener); + } } public void addActivityEventListener(ActivityEventListener listener) { - mActivityEventListeners.add(listener); + synchronized (mActivityEventListeners) { + mActivityEventListeners.put(listener, null); + } } public void removeActivityEventListener(ActivityEventListener listener) { - mActivityEventListeners.remove(listener); + synchronized (mActivityEventListeners) { + mActivityEventListeners.remove(listener); + } } /** @@ -179,11 +192,13 @@ public void onHostResume(@Nullable Activity activity) { mLifecycleState = LifecycleState.RESUMED; mCurrentActivity = new WeakReference(activity); ReactMarker.logMarker(ReactMarkerConstants.ON_HOST_RESUME_START); - for (LifecycleEventListener listener : mLifecycleEventListeners) { - try { - listener.onHostResume(); - } catch (RuntimeException e) { - handleException(e); + synchronized (mLifecycleEventListeners) { + for (LifecycleEventListener listener : mLifecycleEventListeners.keySet()) { + try { + listener.onHostResume(); + } catch (RuntimeException e) { + handleException(e); + } } } ReactMarker.logMarker(ReactMarkerConstants.ON_HOST_RESUME_END); @@ -192,11 +207,13 @@ public void onHostResume(@Nullable Activity activity) { public void onNewIntent(@Nullable Activity activity, Intent intent) { UiThreadUtil.assertOnUiThread(); mCurrentActivity = new WeakReference(activity); - for (ActivityEventListener listener : mActivityEventListeners) { - try { - listener.onNewIntent(intent); - } catch (RuntimeException e) { - handleException(e); + synchronized (mActivityEventListeners) { + for (ActivityEventListener listener : mActivityEventListeners.keySet()) { + try { + listener.onNewIntent(intent); + } catch (RuntimeException e) { + handleException(e); + } } } } @@ -207,11 +224,13 @@ public void onNewIntent(@Nullable Activity activity, Intent intent) { public void onHostPause() { mLifecycleState = LifecycleState.BEFORE_RESUME; ReactMarker.logMarker(ReactMarkerConstants.ON_HOST_PAUSE_START); - for (LifecycleEventListener listener : mLifecycleEventListeners) { - try { - listener.onHostPause(); - } catch (RuntimeException e) { - handleException(e); + synchronized (mLifecycleEventListeners) { + for (LifecycleEventListener listener : mLifecycleEventListeners.keySet()) { + try { + listener.onHostPause(); + } catch (RuntimeException e) { + handleException(e); + } } } ReactMarker.logMarker(ReactMarkerConstants.ON_HOST_PAUSE_END); @@ -223,11 +242,13 @@ public void onHostPause() { public void onHostDestroy() { UiThreadUtil.assertOnUiThread(); mLifecycleState = LifecycleState.BEFORE_CREATE; - for (LifecycleEventListener listener : mLifecycleEventListeners) { - try { - listener.onHostDestroy(); - } catch (RuntimeException e) { - handleException(e); + synchronized (mLifecycleEventListeners) { + for (LifecycleEventListener listener : mLifecycleEventListeners.keySet()) { + try { + listener.onHostDestroy(); + } catch (RuntimeException e) { + handleException(e); + } } } mCurrentActivity = null; @@ -248,11 +269,13 @@ public void destroy() { * Should be called by the hosting Fragment in {@link Fragment#onActivityResult} */ public void onActivityResult(Activity activity, int requestCode, int resultCode, Intent data) { - for (ActivityEventListener listener : mActivityEventListeners) { - try { - listener.onActivityResult(activity, requestCode, resultCode, data); - } catch (RuntimeException e) { - handleException(e); + synchronized (mActivityEventListeners) { + for (ActivityEventListener listener : mActivityEventListeners.keySet()) { + try { + listener.onActivityResult(activity, requestCode, resultCode, data); + } catch (RuntimeException e) { + handleException(e); + } } } } From 79202a608db1d2cce2ad7142c0e8c9dfade52bec Mon Sep 17 00:00:00 2001 From: Sergei Dryganets Date: Mon, 19 Nov 2018 17:05:14 -0800 Subject: [PATCH 2/3] Revert "This change fixes currently broken ReactContext listeners mechanism." This reverts commit 24ea3f185ba0a5445d472b26784fad495da7a417. --- .../facebook/react/bridge/ReactContext.java | 95 +++++++------------ 1 file changed, 36 insertions(+), 59 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java index b77f89d986247a..d83fe1be13db05 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java @@ -18,8 +18,7 @@ import com.facebook.react.bridge.queue.ReactQueueConfiguration; import com.facebook.react.common.LifecycleState; import java.lang.ref.WeakReference; -import java.util.Map; -import java.util.WeakHashMap; +import java.util.concurrent.CopyOnWriteArraySet; import javax.annotation.Nullable; /** @@ -33,10 +32,10 @@ public class ReactContext extends ContextWrapper { "ReactContext#getJSModule should only happen once initialize() has been called on your " + "native module."; - private final Map mLifecycleEventListeners = - new WeakHashMap<>(); - private final Map mActivityEventListeners = - new WeakHashMap<>(); + private final CopyOnWriteArraySet mLifecycleEventListeners = + new CopyOnWriteArraySet<>(); + private final CopyOnWriteArraySet mActivityEventListeners = + new CopyOnWriteArraySet<>(); private LifecycleState mLifecycleState = LifecycleState.BEFORE_CREATE; @@ -133,11 +132,7 @@ public LifecycleState getLifecycleState() { } public void addLifecycleEventListener(final LifecycleEventListener listener) { - - synchronized (mLifecycleEventListeners) { - mLifecycleEventListeners.put(listener, null); - } - + mLifecycleEventListeners.add(listener); if (hasActiveCatalystInstance()) { switch (mLifecycleState) { case BEFORE_CREATE: @@ -148,10 +143,8 @@ public void addLifecycleEventListener(final LifecycleEventListener listener) { new Runnable() { @Override public void run() { - synchronized (mLifecycleEventListeners) { - if (!mLifecycleEventListeners.containsKey(listener)) { - return; - } + if (!mLifecycleEventListeners.contains(listener)) { + return; } try { listener.onHostResume(); @@ -168,21 +161,15 @@ public void run() { } public void removeLifecycleEventListener(LifecycleEventListener listener) { - synchronized (mLifecycleEventListeners) { - mLifecycleEventListeners.remove(listener); - } + mLifecycleEventListeners.remove(listener); } public void addActivityEventListener(ActivityEventListener listener) { - synchronized (mActivityEventListeners) { - mActivityEventListeners.put(listener, null); - } + mActivityEventListeners.add(listener); } public void removeActivityEventListener(ActivityEventListener listener) { - synchronized (mActivityEventListeners) { - mActivityEventListeners.remove(listener); - } + mActivityEventListeners.remove(listener); } /** @@ -192,13 +179,11 @@ public void onHostResume(@Nullable Activity activity) { mLifecycleState = LifecycleState.RESUMED; mCurrentActivity = new WeakReference(activity); ReactMarker.logMarker(ReactMarkerConstants.ON_HOST_RESUME_START); - synchronized (mLifecycleEventListeners) { - for (LifecycleEventListener listener : mLifecycleEventListeners.keySet()) { - try { - listener.onHostResume(); - } catch (RuntimeException e) { - handleException(e); - } + for (LifecycleEventListener listener : mLifecycleEventListeners) { + try { + listener.onHostResume(); + } catch (RuntimeException e) { + handleException(e); } } ReactMarker.logMarker(ReactMarkerConstants.ON_HOST_RESUME_END); @@ -207,13 +192,11 @@ public void onHostResume(@Nullable Activity activity) { public void onNewIntent(@Nullable Activity activity, Intent intent) { UiThreadUtil.assertOnUiThread(); mCurrentActivity = new WeakReference(activity); - synchronized (mActivityEventListeners) { - for (ActivityEventListener listener : mActivityEventListeners.keySet()) { - try { - listener.onNewIntent(intent); - } catch (RuntimeException e) { - handleException(e); - } + for (ActivityEventListener listener : mActivityEventListeners) { + try { + listener.onNewIntent(intent); + } catch (RuntimeException e) { + handleException(e); } } } @@ -224,13 +207,11 @@ public void onNewIntent(@Nullable Activity activity, Intent intent) { public void onHostPause() { mLifecycleState = LifecycleState.BEFORE_RESUME; ReactMarker.logMarker(ReactMarkerConstants.ON_HOST_PAUSE_START); - synchronized (mLifecycleEventListeners) { - for (LifecycleEventListener listener : mLifecycleEventListeners.keySet()) { - try { - listener.onHostPause(); - } catch (RuntimeException e) { - handleException(e); - } + for (LifecycleEventListener listener : mLifecycleEventListeners) { + try { + listener.onHostPause(); + } catch (RuntimeException e) { + handleException(e); } } ReactMarker.logMarker(ReactMarkerConstants.ON_HOST_PAUSE_END); @@ -242,13 +223,11 @@ public void onHostPause() { public void onHostDestroy() { UiThreadUtil.assertOnUiThread(); mLifecycleState = LifecycleState.BEFORE_CREATE; - synchronized (mLifecycleEventListeners) { - for (LifecycleEventListener listener : mLifecycleEventListeners.keySet()) { - try { - listener.onHostDestroy(); - } catch (RuntimeException e) { - handleException(e); - } + for (LifecycleEventListener listener : mLifecycleEventListeners) { + try { + listener.onHostDestroy(); + } catch (RuntimeException e) { + handleException(e); } } mCurrentActivity = null; @@ -269,13 +248,11 @@ public void destroy() { * Should be called by the hosting Fragment in {@link Fragment#onActivityResult} */ public void onActivityResult(Activity activity, int requestCode, int resultCode, Intent data) { - synchronized (mActivityEventListeners) { - for (ActivityEventListener listener : mActivityEventListeners.keySet()) { - try { - listener.onActivityResult(activity, requestCode, resultCode, data); - } catch (RuntimeException e) { - handleException(e); - } + for (ActivityEventListener listener : mActivityEventListeners) { + try { + listener.onActivityResult(activity, requestCode, resultCode, data); + } catch (RuntimeException e) { + handleException(e); } } } From 42d9843b1df6f859daa0b607389829fda7339380 Mon Sep 17 00:00:00 2001 From: Sergei Dryganets Date: Mon, 19 Nov 2018 15:57:21 -0800 Subject: [PATCH 3/3] ReactContext listeners garbage collected automatically. --- .../facebook/react/bridge/ReactContext.java | 92 +++++++++++-------- .../react/bridge/SynchronizedWeakHashSet.java | 86 +++++++++++++++++ 2 files changed, 140 insertions(+), 38 deletions(-) create mode 100644 ReactAndroid/src/main/java/com/facebook/react/bridge/SynchronizedWeakHashSet.java diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java index d83fe1be13db05..f10045ec569ef8 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java @@ -18,7 +18,6 @@ import com.facebook.react.bridge.queue.ReactQueueConfiguration; import com.facebook.react.common.LifecycleState; import java.lang.ref.WeakReference; -import java.util.concurrent.CopyOnWriteArraySet; import javax.annotation.Nullable; /** @@ -32,10 +31,35 @@ public class ReactContext extends ContextWrapper { "ReactContext#getJSModule should only happen once initialize() has been called on your " + "native module."; - private final CopyOnWriteArraySet mLifecycleEventListeners = - new CopyOnWriteArraySet<>(); - private final CopyOnWriteArraySet mActivityEventListeners = - new CopyOnWriteArraySet<>(); + private final SynchronizedWeakHashSet mLifecycleEventListeners = + new SynchronizedWeakHashSet<>(); + private final SynchronizedWeakHashSet mActivityEventListeners = + new SynchronizedWeakHashSet<>(); + + + private final GuardedIteration mResumeIteration = + new GuardedIteration() { + @Override + public void onIterate(LifecycleEventListener listener) { + listener.onHostResume(); + } + }; + + private final GuardedIteration mPauseIteration = + new GuardedIteration() { + @Override + public void onIterate(LifecycleEventListener listener) { + listener.onHostPause(); + } + }; + + private final GuardedIteration mDestroyIteration = + new GuardedIteration() { + @Override + public void onIterate(LifecycleEventListener listener) { + listener.onHostDestroy(); + } + }; private LifecycleState mLifecycleState = LifecycleState.BEFORE_CREATE; @@ -179,26 +203,19 @@ public void onHostResume(@Nullable Activity activity) { mLifecycleState = LifecycleState.RESUMED; mCurrentActivity = new WeakReference(activity); ReactMarker.logMarker(ReactMarkerConstants.ON_HOST_RESUME_START); - for (LifecycleEventListener listener : mLifecycleEventListeners) { - try { - listener.onHostResume(); - } catch (RuntimeException e) { - handleException(e); - } - } + mLifecycleEventListeners.iterate(mResumeIteration); ReactMarker.logMarker(ReactMarkerConstants.ON_HOST_RESUME_END); } - public void onNewIntent(@Nullable Activity activity, Intent intent) { + public void onNewIntent(@Nullable Activity activity, final Intent intent) { UiThreadUtil.assertOnUiThread(); mCurrentActivity = new WeakReference(activity); - for (ActivityEventListener listener : mActivityEventListeners) { - try { + mActivityEventListeners.iterate(new GuardedIteration() { + @Override + public void onIterate(ActivityEventListener listener) { listener.onNewIntent(intent); - } catch (RuntimeException e) { - handleException(e); } - } + }); } /** @@ -207,13 +224,7 @@ public void onNewIntent(@Nullable Activity activity, Intent intent) { public void onHostPause() { mLifecycleState = LifecycleState.BEFORE_RESUME; ReactMarker.logMarker(ReactMarkerConstants.ON_HOST_PAUSE_START); - for (LifecycleEventListener listener : mLifecycleEventListeners) { - try { - listener.onHostPause(); - } catch (RuntimeException e) { - handleException(e); - } - } + mLifecycleEventListeners.iterate(mPauseIteration); ReactMarker.logMarker(ReactMarkerConstants.ON_HOST_PAUSE_END); } @@ -223,13 +234,7 @@ public void onHostPause() { public void onHostDestroy() { UiThreadUtil.assertOnUiThread(); mLifecycleState = LifecycleState.BEFORE_CREATE; - for (LifecycleEventListener listener : mLifecycleEventListeners) { - try { - listener.onHostDestroy(); - } catch (RuntimeException e) { - handleException(e); - } - } + mLifecycleEventListeners.iterate(mDestroyIteration); mCurrentActivity = null; } @@ -247,14 +252,13 @@ public void destroy() { /** * Should be called by the hosting Fragment in {@link Fragment#onActivityResult} */ - public void onActivityResult(Activity activity, int requestCode, int resultCode, Intent data) { - for (ActivityEventListener listener : mActivityEventListeners) { - try { + public void onActivityResult(final Activity activity, final int requestCode, final int resultCode, final Intent data) { + mActivityEventListeners.iterate(new GuardedIteration() { + @Override + public void onIterate(ActivityEventListener listener) { listener.onActivityResult(activity, requestCode, resultCode, data); - } catch (RuntimeException e) { - handleException(e); } - } + }); } public void assertOnUiQueueThread() { @@ -350,4 +354,16 @@ public JavaScriptContextHolder getJavaScriptContextHolder() { return mCatalystInstance.getJavaScriptContextHolder(); } + private abstract class GuardedIteration implements SynchronizedWeakHashSet.Iteration { + @Override + public void iterate(T listener) { + try { + onIterate(listener); + } catch (RuntimeException e) { + handleException(e); + } + } + + public abstract void onIterate(T listener); + } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/SynchronizedWeakHashSet.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/SynchronizedWeakHashSet.java new file mode 100644 index 00000000000000..b7ce40b0cad46a --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/SynchronizedWeakHashSet.java @@ -0,0 +1,86 @@ +// Copyright (c) Facebook, Inc. and its affiliates. + +// This source code is licensed under the MIT license found in the +// LICENSE file in the root directory of this source tree. +package com.facebook.react.bridge; + +import android.util.Pair; + +import java.util.ArrayDeque; +import java.util.Queue; +import java.util.WeakHashMap; + +/** + * Thread-safe Set based on the WeakHashMap. + * + * Doesn't implement the `iterator` method because it's tricky to support modifications + * to the collection while somebody is using an `Iterator` to iterate over it. + * + * Instead, it provides an `iterate` method for traversing the collection. Any add/remove operations + * that occur during iteration are postponed until the iteration has completed. + */ +public class SynchronizedWeakHashSet { + private WeakHashMap mMap = new WeakHashMap<>(); + private Queue> mPendingOperations = new ArrayDeque<>(); + private boolean mIterating; + + public boolean contains(T item) { + synchronized (mMap) { + return mMap.containsKey(item); + } + } + + public void add(T item) { + synchronized (mMap) { + if (mIterating) { + mPendingOperations.add(new Pair<>(item, Command.ADD)); + } else { + mMap.put(item, null); + } + } + } + + public void remove(T item) { + synchronized (mMap) { + if (mIterating) { + mPendingOperations.add(new Pair<>(item, Command.REMOVE)); + } else { + mMap.remove(item); + } + } + } + + public void iterate(Iteration iterated) { + synchronized (mMap) { + // Protection from modification during iteration on the same thread + mIterating = true; + for (T listener: mMap.keySet()) { + iterated.iterate(listener); + } + mIterating = false; + + while (!mPendingOperations.isEmpty()) { + Pair pair = mPendingOperations.poll(); + switch (pair.second) { + case ADD: + mMap.put(pair.first, null); + break; + case REMOVE: + mMap.remove(pair.first); + break; + default: + throw new AssertionException("Unsupported command" + pair.second); + } + } + } + } + + public interface Iteration { + void iterate(T item); + } + + private enum Command { + ADD, + REMOVE + } +}