Add locking around CatalystInstance.getJavaScriptContext()

Reviewed By: kathryngray

Differential Revision: D5861693

fbshipit-source-id: 226ff15622d5e1a8ae3ad4c63f1434bd95c1fa21

ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstance.java

@@ -9,13 +9,11 @@
 
 package com.facebook.react.bridge;
 
-import javax.annotation.Nullable;
-
-import java.util.Collection;
-
 import com.facebook.proguard.annotations.DoNotStrip;
 import com.facebook.react.bridge.queue.ReactQueueConfiguration;
 import com.facebook.react.common.annotations.VisibleForTesting;
+import java.util.Collection;
+import javax.annotation.Nullable;
 
 /**
  * A higher level API on top of the asynchronous JSC bridge. This provides an
@@ -93,6 +91,10 @@ public interface CatalystInstance
 
   /**
    * Get the C pointer (as a long) to the JavaScriptCore context associated with this instance.
+   *
+   * <p>Use the following pattern to ensure that the JS context is not cleared while you are using
+   * it: JavaScriptContextHolder jsContext = reactContext.getJavaScriptContextHolder()
+   * synchronized(jsContext) { nativeThingNeedingJsContext(jsContext.get()); }
    */
-  long getJavaScriptContext();
+  JavaScriptContextHolder getJavaScriptContextHolder();
 }

ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java

@@ -91,6 +91,8 @@ public class CatalystInstanceImpl implements CatalystInstance {
   private boolean mJSBundleHasLoaded;
   private @Nullable String mSourceURL;
 
+  private JavaScriptContextHolder mJavaScriptContextHolder;
+
   // C++ parts
   private final HybridData mHybridData;
   private native static HybridData initHybrid();
@@ -126,6 +128,8 @@ public class CatalystInstanceImpl implements CatalystInstance {
       mNativeModuleRegistry.getJavaModules(this),
       mNativeModuleRegistry.getCxxModules());
     Log.d(ReactConstants.TAG, "Initializing React Xplat Bridge after initializeBridge");
+
+    mJavaScriptContextHolder = new JavaScriptContextHolder(getJavaScriptContext());
   }
 
   private static class BridgeCallback implements ReactCallback {
@@ -333,6 +337,11 @@ public class CatalystInstanceImpl implements CatalystInstance {
           public void run() {
             // Kill non-UI threads from neutral third party
             // potentially expensive, so don't run on UI thread
+
+            // contextHolder is used as a lock to guard against other users of the JS VM having
+            // the VM destroyed underneath them, so notify them before we resetNative
+            mJavaScriptContextHolder.clear();
+
             mHybridData.resetNative();
             getReactQueueConfiguration().destroy();
             Log.d(ReactConstants.TAG, "CatalystInstanceImpl.destroy() end");
@@ -436,7 +445,11 @@ public class CatalystInstanceImpl implements CatalystInstance {
   public native void setGlobalVariable(String propName, String jsonValue);
 
   @Override
-  public native long getJavaScriptContext();
+  public JavaScriptContextHolder getJavaScriptContextHolder() {
+    return mJavaScriptContextHolder;
+  }
+
+  private native long getJavaScriptContext();
 
   private void incrementPendingJSCalls() {
     int oldPendingCalls = mPendingJSCalls.getAndIncrement();

ReactAndroid/src/main/java/com/facebook/react/bridge/JavaScriptContextHolder.java

@@ -0,0 +1,29 @@
+// Copyright 2004-present Facebook. All Rights Reserved.
+
+package com.facebook.react.bridge;
+
+import javax.annotation.concurrent.GuardedBy;
+
+/**
+ * Wrapper for JavaScriptContext native pointer. CatalystInstanceImpl creates this on demand, and
+ * will call clear() before destroying the VM. People who need the raw JavaScriptContext pointer
+ * can synchronize on this wrapper object to guarantee that it will not be destroyed.
+ */
+public class JavaScriptContextHolder {
+  @GuardedBy("this")
+  private long mContext;
+
+  public JavaScriptContextHolder(long context) {
+    mContext = context;
+  }
+
+  @GuardedBy("this")
+  public long get() {
+    return mContext;
+  }
+
+  public synchronized void clear() {
+    mContext = 0;
+  }
+
+}

ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java

@@ -9,22 +9,19 @@
 
 package com.facebook.react.bridge;
 
-import javax.annotation.Nullable;
-
-import java.lang.ref.WeakReference;
-import java.util.concurrent.CopyOnWriteArraySet;
-
 import android.app.Activity;
 import android.content.Context;
 import android.content.ContextWrapper;
 import android.content.Intent;
 import android.os.Bundle;
 import android.view.LayoutInflater;
-
 import com.facebook.infer.annotation.Assertions;
 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;
 
 /**
  * Abstract ContextWrapper for Android application or activity {@link Context} and
@@ -372,9 +369,12 @@ public class ReactContext extends ContextWrapper {
   }
 
   /**
-   * Get the C pointer (as a long) to the JavaScriptCore context associated with this instance.
+   * Get the C pointer (as a long) to the JavaScriptCore context associated with this instance. Use
+   * the following pattern to ensure that the JS context is not cleared while you are using it:
+   * JavaScriptContextHolder jsContext = reactContext.getJavaScriptContextHolder()
+   * synchronized(jsContext) { nativeThingNeedingJsContext(jsContext.get()); }
    */
-  public long getJavaScriptContext() {
-    return mCatalystInstance.getJavaScriptContext();
+  public JavaScriptContextHolder getJavaScriptContextHolder() {
+    return mCatalystInstance.getJavaScriptContextHolder();
   }
 }

ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManagerImpl.java

@@ -30,6 +30,7 @@ import com.facebook.react.R;
 import com.facebook.react.bridge.CatalystInstance;
 import com.facebook.react.bridge.DefaultNativeModuleCallExceptionHandler;
 import com.facebook.react.bridge.JavaJSExecutor;
+import com.facebook.react.bridge.JavaScriptContextHolder;
 import com.facebook.react.bridge.ReactContext;
 import com.facebook.react.bridge.ReactMarker;
 import com.facebook.react.bridge.ReactMarkerConstants;
@@ -711,10 +712,12 @@ public class DevSupportManagerImpl implements
           return;
         }
         try {
-          long jsContext = mCurrentContext.getJavaScriptContext();
+          JavaScriptContextHolder jsContext = mCurrentContext.getJavaScriptContextHolder();
+          synchronized (jsContext) {
             Class clazz = Class.forName("com.facebook.react.packagerconnection.SamplingProfilerPackagerMethod");
-          RequestHandler handler = (RequestHandler)clazz.getConstructor(long.class).newInstance(jsContext);
+            RequestHandler handler = (RequestHandler)clazz.getConstructor(long.class).newInstance(jsContext.get());
             handler.onRequest(null, responder);
+          }
         } catch (Exception e) {
           // Module not present
         }