Don't execute things that may throw in Bridge/JSCExecutor dtors

Summary:In testing, I've found that there's no good way to return stack traces to the server for exceptions that happen in dtor's. If the dtor is not marked nothrow(false), the exceptions are uncatchable (and bubble up as a std::abort without exception info) and if annotated properly, the program instead crashes trying to resume the stack, again a std::abort without exception info.

Instead, I created a separate destroy method that can be called (and protected via fbjni) to make the dtor's no longer execute code that may throw. Note that we don't really expect the code that was previously in ~JSCExecutor() to throw, but it was in production and we had absolutely no info to help debug it.

Reviewed By: mhorowitz

Differential Revision: D2989999

fb-gh-sync-id: 4cf9de5e0592fe6830a9903375363a78e1339a94
shipit-source-id: 4cf9de5e0592fe6830a9903375363a78e1339a94

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

@@ -235,6 +235,7 @@ public class CatalystInstanceImpl implements CatalystInstance {
         new Runnable() {
           @Override
           public void run() {
+            mBridge.destroy();
             mBridge.dispose();
             bridgeDisposeFuture.set(null);
           }

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

@@ -87,6 +87,7 @@ public class ReactBridge extends Countable {
   public native void stopProfiler(String title, String filename);
   private native void handleMemoryPressureModerate();
   private native void handleMemoryPressureCritical();
+  public native void destroy();
 
   /**
    * This method will return a long representing the underlying JSGlobalContextRef pointer or

ReactAndroid/src/main/jni/react/Bridge.cpp

@@ -22,8 +22,7 @@ Bridge::Bridge(JSExecutorFactory* jsExecutorFactory, Callback callback) :
 
 // This must be called on the same thread on which the constructor was called.
 Bridge::~Bridge() {
-  *m_destroyed = true;
-  m_mainExecutor.reset();
+  CHECK(*m_destroyed) << "Bridge::destroy() must be called before deallocating the Bridge!";
 }
 
 void Bridge::loadApplicationScript(const std::string& script, const std::string& sourceURL) {
@@ -132,4 +131,10 @@ void Bridge::callNativeModules(const std::string& callJSON, bool isEndOfBatch) {
   m_callback(parseMethodCalls(callJSON), isEndOfBatch);
 }
 
+void Bridge::destroy() {
+  *m_destroyed = true;
+  m_mainExecutor->destroy();
+  m_mainExecutor.reset();
+}
+
 } }

ReactAndroid/src/main/jni/react/Bridge.h

@@ -74,6 +74,11 @@ public:
    * TODO: get rid of isEndOfBatch
    */
   void callNativeModules(const std::string& callJSON, bool isEndOfBatch);
+
+  /**
+   * Synchronously tears down the bridge and the main executor.
+   */
+  void destroy();
 private:
   Callback m_callback;
   // This is used to avoid a race condition where a proxyCallback gets queued after ~Bridge(),

ReactAndroid/src/main/jni/react/Executor.h

@@ -72,6 +72,7 @@ public:
   virtual void handleMemoryPressureCritical() {
     handleMemoryPressureModerate();
   };
+  virtual void destroy() {};
   virtual ~JSExecutor() {};
 };
 

ReactAndroid/src/main/jni/react/JSCExecutor.cpp

@@ -126,17 +126,17 @@ JSCExecutor::JSCExecutor(
 }
 
 JSCExecutor::~JSCExecutor() {
-  try {
-    *m_isDestroyed = true;
-    if (m_messageQueueThread->isOnThread()) {
+  CHECK(*m_isDestroyed) << "JSCExecutor::destroy() must be called before its destructor!";
+}
+
+void JSCExecutor::destroy() {
+  *m_isDestroyed = true;
+  if (m_messageQueueThread->isOnThread()) {
+    terminateOnJSVMThread();
+  } else {
+    m_messageQueueThread->runOnQueueSync([this] () {
       terminateOnJSVMThread();
-    } else {
-      m_messageQueueThread->runOnQueueSync([this] () {
-        terminateOnJSVMThread();
-      });
-    }
-  } catch (...) {
-    Exceptions::handleUncaughtException();
+    });
   }
 }
 
@@ -392,6 +392,7 @@ void JSCExecutor::receiveMessageFromOwner(const std::string& msgString) {
 void JSCExecutor::terminateOwnedWebWorker(int workerId) {
   auto worker = m_ownedWorkers.at(workerId).getExecutor();
   std::shared_ptr<MessageQueueThread> workerMQT = worker->m_messageQueueThread;
+  worker->destroy();
   m_ownedWorkers.erase(workerId);
   workerMQT->quitSynchronous();
 }

ReactAndroid/src/main/jni/react/JSCExecutor.h

@@ -75,6 +75,7 @@ public:
   virtual void stopProfiler(const std::string &titleString, const std::string &filename) override;
   virtual void handleMemoryPressureModerate() override;
   virtual void handleMemoryPressureCritical() override;
+  virtual void destroy() override;
 
   void installNativeHook(const char *name, JSObjectCallAsFunctionCallback callback);
 

ReactAndroid/src/main/jni/react/Platform.cpp

@@ -26,8 +26,4 @@ namespace JSLogging {
 JSCNativeHook nativeHook = nullptr;
 };
 
-namespace Exceptions {
-HandleUncaughtException handleUncaughtException;
-};
-
 } }

ReactAndroid/src/main/jni/react/Platform.h

@@ -46,9 +46,4 @@ namespace JSLogging {
   extern JSCNativeHook nativeHook;
 };
 
-namespace Exceptions {
-  using HandleUncaughtException = std::function<void()>;
-  extern HandleUncaughtException handleUncaughtException;
-};
-
 } }

ReactAndroid/src/main/jni/react/jni/OnLoad.cpp

@@ -651,6 +651,15 @@ static void create(JNIEnv* env, jobject obj, jobject executor, jobject callback,
   setCountableForJava(env, obj, std::move(bridge));
 }
 
+static void destroy(JNIEnv* env, jobject jbridge) {
+  auto bridge = extractRefPtr<CountableBridge>(env, jbridge);
+  try {
+    bridge->destroy();
+  } catch (...) {
+    translatePendingCppExceptionToJavaException();
+  }
+}
+
 static void loadApplicationScript(
     const RefPtr<CountableBridge>& bridge,
     const std::string& script,
@@ -865,9 +874,6 @@ extern "C" JNIEXPORT jint JNI_OnLoad(JavaVM* vm, void* reserved) {
         return std::unique_ptr<MessageQueueThread>(
             JMessageQueueThread::currentMessageQueueThread().release());
       };
-    Exceptions::handleUncaughtException = [] () {
-      translatePendingCppExceptionToJavaException();
-    };
     PerfLogging::installNativeHooks = addNativePerfLoggingHooks;
     JSLogging::nativeHook = nativeLoggingHook;
 
@@ -950,6 +956,7 @@ extern "C" JNIEXPORT jint JNI_OnLoad(JavaVM* vm, void* reserved) {
 
     registerNatives("com/facebook/react/bridge/ReactBridge", {
         makeNativeMethod("initialize", "(Lcom/facebook/react/bridge/JavaScriptExecutor;Lcom/facebook/react/bridge/ReactCallback;Lcom/facebook/react/bridge/queue/MessageQueueThread;)V", bridge::create),
+        makeNativeMethod("destroy", bridge::destroy),
         makeNativeMethod(
           "loadScriptFromAssets", "(Landroid/content/res/AssetManager;Ljava/lang/String;)V",
           bridge::loadScriptFromAssets),