From 972f39985a3a74dc48b2b77a88c8f78a20073a46 Mon Sep 17 00:00:00 2001 From: Sergei Dryganets Date: Mon, 25 Feb 2019 08:02:10 -0800 Subject: [PATCH] This change fixes currently broken ReactContext listeners mechanism. (#22318) Summary: 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. 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 Request resolved: https://github.com/facebook/react-native/pull/22318 Reviewed By: mdvacca Differential Revision: D13106915 Pulled By: hramos fbshipit-source-id: d506e5035a7f7bea1b57a6308fb5d9b5fcb277a7 --- .../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 d07d4ae28..1a4c8949c 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.MessageQueueThread; 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; @@ -188,26 +212,19 @@ public class ReactContext extends ContextWrapper { 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); } - } + }); } /** @@ -216,13 +233,7 @@ public class ReactContext extends ContextWrapper { 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); } @@ -232,13 +243,7 @@ public class ReactContext extends ContextWrapper { 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; } @@ -256,14 +261,13 @@ public class ReactContext extends ContextWrapper { /** * 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() { @@ -359,4 +363,16 @@ public class ReactContext extends ContextWrapper { 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 000000000..b7ce40b0c --- /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 + } +}