Skip to content

Commit

Permalink
This change fixes currently broken ReactContext listeners mechanism.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
dryganets committed Nov 16, 2018
1 parent 9ea1295 commit 24ea3f1
Showing 1 changed file with 59 additions and 36 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -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<LifecycleEventListener> mLifecycleEventListeners =
new CopyOnWriteArraySet<>();
private final CopyOnWriteArraySet<ActivityEventListener> mActivityEventListeners =
new CopyOnWriteArraySet<>();
private final Map<LifecycleEventListener, Void> mLifecycleEventListeners =
new WeakHashMap<>();
private final Map<ActivityEventListener, Void> mActivityEventListeners =
new WeakHashMap<>();

private LifecycleState mLifecycleState = LifecycleState.BEFORE_CREATE;

Expand Down Expand Up @@ -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:
Expand All @@ -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();
Expand All @@ -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);
}
}

/**
Expand All @@ -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);
Expand All @@ -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);
}
}
}
}
Expand All @@ -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);
Expand All @@ -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;
Expand All @@ -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);
}
}
}
}
Expand Down

0 comments on commit 24ea3f1

Please sign in to comment.