This change fixes currently broken ReactContext listeners mechanism. (#22318)

dryganets · facebook-github-bot · commit 972f39985a3a · 2019-02-25T08:06:22.000-08:00
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: #22318 Reviewed By: mdvacca Differential Revision: D13106915 Pulled By: hramos fbshipit-source-id: d506e5035a7f7bea1b57a6308fb5d9b5fcb277a7
diff --git 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<LifecycleEventListener> mLifecycleEventListeners =
-      new CopyOnWriteArraySet<>();
-  private final CopyOnWriteArraySet<ActivityEventListener> mActivityEventListeners =
-      new CopyOnWriteArraySet<>();
+  private final SynchronizedWeakHashSet<LifecycleEventListener> mLifecycleEventListeners =
+      new SynchronizedWeakHashSet<>();
+  private final SynchronizedWeakHashSet<ActivityEventListener> mActivityEventListeners =
+      new SynchronizedWeakHashSet<>();
+
+
+  private final GuardedIteration<LifecycleEventListener> mResumeIteration =
+    new GuardedIteration<LifecycleEventListener>() {
+      @Override
+      public void onIterate(LifecycleEventListener listener) {
+        listener.onHostResume();
+      }
+    };
+
+  private final GuardedIteration<LifecycleEventListener> mPauseIteration =
+    new GuardedIteration<LifecycleEventListener>() {
+      @Override
+      public void onIterate(LifecycleEventListener listener) {
+        listener.onHostPause();
+      }
+    };
+
+  private final GuardedIteration<LifecycleEventListener> mDestroyIteration =
+    new GuardedIteration<LifecycleEventListener>() {
+      @Override
+      public void onIterate(LifecycleEventListener listener) {
+        listener.onHostDestroy();
+      }
+    };
 
   private LifecycleState mLifecycleState = LifecycleState.BEFORE_CREATE;
 
@@ -188,26 +212,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<ActivityEventListener>() {
+      @Override
+      public void onIterate(ActivityEventListener listener) {
         listener.onNewIntent(intent);
-      } catch (RuntimeException e) {
-        handleException(e);
       }
-    }
+    });
   }
 
   /**
@@ -216,13 +233,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);
   }
 
@@ -232,13 +243,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;
   }
 
@@ -256,14 +261,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<ActivityEventListener>() {
+      @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 JavaScriptContextHolder getJavaScriptContextHolder() {
     return mCatalystInstance.getJavaScriptContextHolder();
   }
 
+  private abstract class GuardedIteration<T> implements SynchronizedWeakHashSet.Iteration<T> {
+    @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
@@ -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<T> {
+  private WeakHashMap<T, Void> mMap = new WeakHashMap<>();
+  private Queue<Pair<T, Command>> 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<T> 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<T, Command> 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<T> {
+    void iterate(T item);
+  }
+
+  private enum Command {
+    ADD,
+    REMOVE
+  }
+}
