From 9b094ee77a5f82b1e5ebfe6384afb3f660f03a3d Mon Sep 17 00:00:00 2001
From: Ramanpreet Nara <ramanpreet@fb.com>
Date: Thu, 17 Sep 2020 16:04:40 -0700
Subject: [PATCH] Stop accessing JVM in ~JavaTurboModule

Summary:
Inside JavaTurboModule, the native `CallInvoker` is used to schedule work on the NativeModules thread. So, in ~JavaTurboModule(), I scheduled some work on the NativeModules thread. This work holds a copy of the JNI global reference to the Java NativeModule object, and when it's executed, it resets this global reference to the Java NativeModule object. This should ensure that the we don't access the JVM in ~JavaTurboModule, which could crash the program.

I also removed the redundant `quitSynchronous()` in `~CatalystInstanceImpl()`, to prevent the NativeModules thread from being deleted before we delete the `jsi::Runtime`. This shouldn't cause an issue, because we delete the NativeModules thread when we call [ReactQueueConfigurationImpl.destroy()](https://fburl.com/codesearch/p7aurwn3).

Changelog: [Internal]

Reviewed By: ejanzer

Differential Revision: D23744777

fbshipit-source-id: a5c8d3f2ac4287dfef9a4b4404a04b335aa0963d
---
 .../jni/react/jni/CatalystInstanceImpl.cpp    |  6 ------
 .../main/jni/react/jni/CatalystInstanceImpl.h |  1 -
 .../core/platform/android/JavaTurboModule.cpp | 20 +++++++++++++++++++
 .../core/platform/android/JavaTurboModule.h   |  1 +
 4 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp
index 7d5b1acf483..659c8235a64 100644
--- a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp
+++ b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp
@@ -92,12 +92,6 @@ CatalystInstanceImpl::initHybrid(jni::alias_ref<jclass>) {
 CatalystInstanceImpl::CatalystInstanceImpl()
     : instance_(std::make_unique<Instance>()) {}
 
-CatalystInstanceImpl::~CatalystInstanceImpl() {
-  if (moduleMessageQueue_ != NULL) {
-    moduleMessageQueue_->quitSynchronous();
-  }
-}
-
 void CatalystInstanceImpl::registerNatives() {
   registerHybrid({
       makeNativeMethod("initHybrid", CatalystInstanceImpl::initHybrid),
diff --git a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h
index dd9990ef090..c40a691232c 100644
--- a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h
+++ b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h
@@ -37,7 +37,6 @@ class CatalystInstanceImpl : public jni::HybridClass<CatalystInstanceImpl> {
       "Lcom/facebook/react/bridge/CatalystInstanceImpl;";
 
   static jni::local_ref<jhybriddata> initHybrid(jni::alias_ref<jclass>);
-  ~CatalystInstanceImpl() override;
 
   static void registerNatives();
 
diff --git a/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp b/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp
index 4b5d8d6e953..82fbc928fb6 100644
--- a/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp
+++ b/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp
@@ -31,6 +31,26 @@ JavaTurboModule::JavaTurboModule(const InitParams &params)
       instance_(jni::make_global(params.instance)),
       nativeInvoker_(params.nativeInvoker) {}
 
+JavaTurboModule::~JavaTurboModule() {
+  /**
+   * TODO(T75896241): In E2E tests, instance_ is null. Investigate why. Can we
+   * get rid of this null check?
+   */
+  if (!instance_) {
+    return;
+  }
+
+  nativeInvoker_->invokeAsync([instance = std::move(instance_)]() mutable {
+    /**
+     * Reset the global NativeModule ref on the NativeModules thread. Why:
+     *   - ~JavaTurboModule() can be called on a non-JVM thread. If we reset the
+     *     global ref in ~JavaTurboModule(), we might access the JVM from a
+     *     non-JVM thread, which will crash the app.
+     */
+    instance.reset();
+  });
+}
+
 bool JavaTurboModule::isPromiseAsyncDispatchEnabled_ = false;
 void JavaTurboModule::enablePromiseAsyncDispatch(bool enable) {
   isPromiseAsyncDispatchEnabled_ = enable;
diff --git a/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.h b/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.h
index 642cacb194a..93acda48fcf 100644
--- a/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.h
+++ b/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.h
@@ -41,6 +41,7 @@ class JSI_EXPORT JavaTurboModule : public TurboModule {
   };
 
   JavaTurboModule(const InitParams &params);
+  virtual ~JavaTurboModule();
   jsi::Value invokeJavaMethod(
       jsi::Runtime &runtime,
       TurboModuleMethodValueKind valueKind,