Delete jsi::Functions before jsi::Runtime gets deleted

Summary:
## The Problem
1. `CatalystInstanceImpl` indirectly holds on to the `jsi::Runtime`. When you destroy `CatalystInstanceImpl`, you destroy the `jsi::Runtime`. As a part of reloading React Native, we destroy and re-create `CatalystInstanceImpl`, which destroys and re-creates the `jsi::Runtime`.
2. When JS passes in a callback to a TurboModule method, we take that callback (a `jsi::Function`) and wrap it in a Java `Callback` (implemented by `JCxxCallbackImpl`). This Java `Callback`, when executed, schedules the `jsi::Function` to be invoked on a Java thread at a later point in time. **Note:** The Java NativeModule can hold on to the Java `Callback` (and, by transitivity, the `jsi::Function`) for potentially forever.
3. It is a requirement of `jsi::Runtime` that all objects associated with the Runtime (ex: `jsi::Function`) must be destroyed before the Runtime itself is destroyed. See: https://fburl.com/m3mqk6wt

### jsi.h
```
/// .................................................... In addition, to
/// make shutdown safe, destruction of objects associated with the Runtime
/// must be destroyed before the Runtime is destroyed, or from the
/// destructor of a managed HostObject or HostFunction.  Informally, this
/// means that the main source of unsafe behavior is to hold a jsi object
/// in a non-Runtime-managed object, and not clean it up before the Runtime
/// is shut down.  If your lifecycle is such that avoiding this is hard,
/// you will probably need to do use your own locks.
class Runtime {
 public:
  virtual ~Runtime();
```

Therefore, when you delete `CatalystInstanceImpl`, you could end up with a situation where the `jsi::Runtime` is destroyed before all `jsi::Function`s are destroyed. In dev, this leads the program to crash when you reload the app after having used a TurboModule method that uses callbacks.

## The Solution
If the only reference to a `HostObject` or a `HostFunction` is in the JS Heap, then the `HostObject` and `HostFunction` destructors can destroy JSI objects. The TurboModule cache is the only thing, aside from the JS Heap, that holds a reference to all C++ TurboModules. But that cache (and the entire native side of `TurboModuleManager`) is destroyed when we call `mHybridData.resetNative()` in `TurboModuleManager.onCatalystInstanceDestroy()` in D16552730. (I verified this by commenting out `mHybridData.resetNative()` and placing a breakpoint in the destructor of `JavaTurboModule`). So, when we're cleaning up `TurboModuleManager`, the only reference to a Java TurboModule is the JS Heap. Therefore, it's safe and correct for us to destroy all `jsi::Function`s created by the Java TurboModule in `~JavaTurboModule`. So, in this diff, I keep a set of all `CallbackWrappers`, and explicitly call `destroy()` on them in the `JavaTurboModule` destructor. Note that since `~JavaTurboModule` accesses `callbackWrappers_`, it must be executed on the JS Thread, since `createJavaCallbackFromJSIFunction` also accesses `callbackWrappers_` on the JS Thread.

For additional safety, I also eagerly destroyed the `jsi::Function` after it's been invoked once. I'm not yet sure if we only want JS callbacks to only ever be invoked once. So, I've created a Task to document this work: T48128233.

Reviewed By: mdvacca

Differential Revision: D16623340

fbshipit-source-id: 3a4c3efc70b9b3c8d329f19fdf4b4423c489695b

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

@@ -23,6 +23,7 @@
 import com.facebook.react.bridge.queue.ReactQueueConfigurationSpec;
 import com.facebook.react.common.ReactConstants;
 import com.facebook.react.common.annotations.VisibleForTesting;
+import com.facebook.react.config.ReactFeatureFlags;
 import com.facebook.react.module.annotations.ReactModule;
 import com.facebook.react.turbomodule.core.JSCallInvokerHolderImpl;
 import com.facebook.react.turbomodule.core.interfaces.TurboModule;
@@ -359,24 +360,56 @@ public void run() {
                 listener.onBridgeDestroyed();
               }
             }
-            AsyncTask.execute(
-                new Runnable() {
-                  @Override
-                  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");
-                    ReactMarker.logMarker(ReactMarkerConstants.DESTROY_CATALYST_INSTANCE_END);
-                  }
-                });
+
+            final JSIModule turboModuleManager =
+                ReactFeatureFlags.useTurboModules
+                    ? mJSIModuleRegistry.getModule(JSIModuleType.TurboModuleManager)
+                    : null;
+
+            getReactQueueConfiguration()
+                .getJSQueueThread()
+                .runOnQueue(
+                    new Runnable() {
+                      @Override
+                      public void run() {
+                        // We need to destroy the TurboModuleManager on the JS Thread
+                        if (turboModuleManager != null) {
+                          turboModuleManager.onCatalystInstanceDestroy();
+                        }
+
+                        getReactQueueConfiguration()
+                            .getUIQueueThread()
+                            .runOnQueue(
+                                new Runnable() {
+                                  @Override
+                                  public void run() {
+                                    // AsyncTask.execute must be executed from the UI Thread
+                                    AsyncTask.execute(
+                                        new Runnable() {
+                                          @Override
+                                          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 reset
+                                            // Native
+                                            mJavaScriptContextHolder.clear();
+
+                                            mHybridData.resetNative();
+                                            getReactQueueConfiguration().destroy();
+                                            Log.d(
+                                                ReactConstants.TAG,
+                                                "CatalystInstanceImpl.destroy() end");
+                                            ReactMarker.logMarker(
+                                                ReactMarkerConstants.DESTROY_CATALYST_INSTANCE_END);
+                                          }
+                                        });
+                                  }
+                                });
+                      }
+                    });
           }
         });
 

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

@@ -32,7 +32,15 @@ public void registerModules(List<JSIModuleSpec> jsiModules) {
   }
 
   public void notifyJSInstanceDestroy() {
-    for (JSIModuleHolder moduleHolder : mModules.values()) {
+    for (Map.Entry<JSIModuleType, JSIModuleHolder> entry : mModules.entrySet()) {
+      JSIModuleType moduleType = entry.getKey();
+
+      // Don't call TurboModuleManager.onCatalystInstanceDestroy
+      if (moduleType == JSIModuleType.TurboModuleManager) {
+        continue;
+      }
+
+      JSIModuleHolder moduleHolder = entry.getValue();
       moduleHolder.notifyJSInstanceDestroy();
     }
   }

ReactCommon/turbomodule/core/TurboCxxModule.cpp

@@ -22,35 +22,49 @@ static CxxModule::Callback makeTurboCxxModuleCallback(
     jsi::Runtime &runtime,
     std::shared_ptr<CallbackWrapper> callbackWrapper) {
   return [callbackWrapper](std::vector<folly::dynamic> args) {
-    callbackWrapper->jsInvoker->invokeAsync([callbackWrapper, args]() {
+    callbackWrapper->jsInvoker().invokeAsync([callbackWrapper, args]() {
       std::vector<jsi::Value> innerArgs;
       for (auto &a : args) {
-        innerArgs.push_back(jsi::valueFromDynamic(callbackWrapper->runtime, a));
+        innerArgs.push_back(
+            jsi::valueFromDynamic(callbackWrapper->runtime(), a));
       }
-      callbackWrapper->callback.call(callbackWrapper->runtime, (const jsi::Value *)innerArgs.data(), innerArgs.size());
+      callbackWrapper->callback().call(
+          callbackWrapper->runtime(),
+          (const jsi::Value *)innerArgs.data(),
+          innerArgs.size());
     });
   };
 }
 
-TurboCxxModule::TurboCxxModule(std::unique_ptr<CxxModule> cxxModule, std::shared_ptr<JSCallInvoker> jsInvoker)
-  : TurboModule(cxxModule->getName(), jsInvoker),
-    cxxMethods_(cxxModule->getMethods()),
-    cxxModule_(std::move(cxxModule)) {}
+TurboCxxModule::TurboCxxModule(
+    std::unique_ptr<CxxModule> cxxModule,
+    std::shared_ptr<JSCallInvoker> jsInvoker)
+    : TurboModule(cxxModule->getName(), jsInvoker),
+      cxxMethods_(cxxModule->getMethods()),
+      cxxModule_(std::move(cxxModule)) {}
 
-jsi::Value TurboCxxModule::get(jsi::Runtime& runtime, const jsi::PropNameID& propName) {
+jsi::Value TurboCxxModule::get(
+    jsi::Runtime &runtime,
+    const jsi::PropNameID &propName) {
   std::string propNameUtf8 = propName.utf8(runtime);
 
   if (propNameUtf8 == "getConstants") {
-    // This is special cased because `getConstants()` is already a part of CxxModule.
+    // This is special cased because `getConstants()` is already a part of
+    // CxxModule.
     return jsi::Function::createFromHostFunction(
         runtime,
         propName,
         0,
-        [this](jsi::Runtime &rt, const jsi::Value &thisVal, const jsi::Value *args, size_t count) {
+        [this](
+            jsi::Runtime &rt,
+            const jsi::Value &thisVal,
+            const jsi::Value *args,
+            size_t count) {
           jsi::Object result(rt);
           auto constants = cxxModule_->getConstants();
           for (auto &pair : constants) {
-            result.setProperty(rt, pair.first.c_str(), jsi::valueFromDynamic(rt, pair.second));
+            result.setProperty(
+                rt, pair.first.c_str(), jsi::valueFromDynamic(rt, pair.second));
           }
           return result;
         });
@@ -62,7 +76,11 @@ jsi::Value TurboCxxModule::get(jsi::Runtime& runtime, const jsi::PropNameID& pro
           runtime,
           propName,
           0,
-          [this, propNameUtf8](jsi::Runtime &rt, const jsi::Value &thisVal, const jsi::Value *args, size_t count) {
+          [this, propNameUtf8](
+              jsi::Runtime &rt,
+              const jsi::Value &thisVal,
+              const jsi::Value *args,
+              size_t count) {
             return invokeMethod(rt, VoidKind, propNameUtf8, args, count);
           });
     }
@@ -77,7 +95,6 @@ jsi::Value TurboCxxModule::invokeMethod(
     const std::string &methodName,
     const jsi::Value *args,
     size_t count) {
-
   auto it = cxxMethods_.begin();
   for (; it != cxxMethods_.end(); it++) {
     auto method = *it;
@@ -87,7 +104,8 @@ jsi::Value TurboCxxModule::invokeMethod(
   }
 
   if (it == cxxMethods_.end()) {
-    throw std::runtime_error("Function '" + methodName + "' cannot be found on cxxmodule: " + name_);
+    throw std::runtime_error(
+        "Function '" + methodName + "' cannot be found on cxxmodule: " + name_);
   }
 
   auto method = *it;
@@ -97,23 +115,37 @@ jsi::Value TurboCxxModule::invokeMethod(
     for (size_t i = 0; i < count; i++) {
       innerArgs.push_back(jsi::dynamicFromValue(runtime, args[i]));
     }
-    return jsi::valueFromDynamic(runtime, method.syncFunc(std::move(innerArgs)));
+    return jsi::valueFromDynamic(
+        runtime, method.syncFunc(std::move(innerArgs)));
   } else if (method.func && !method.isPromise) {
     // Async method.
     CxxModule::Callback first;
     CxxModule::Callback second;
 
     if (count < method.callbacks) {
-      throw std::invalid_argument(folly::to<std::string>("Expected ", method.callbacks,
-                                                         " callbacks, but only ", count, " parameters provided"));
+      throw std::invalid_argument(folly::to<std::string>(
+          "Expected ",
+          method.callbacks,
+          " callbacks, but only ",
+          count,
+          " parameters provided"));
     }
 
     if (method.callbacks == 1) {
-      auto wrapper = std::make_shared<CallbackWrapper>(args[count - 1].getObject(runtime).getFunction(runtime), runtime, jsInvoker_);
+      auto wrapper = std::make_shared<CallbackWrapper>(
+          args[count - 1].getObject(runtime).getFunction(runtime),
+          runtime,
+          jsInvoker_);
       first = makeTurboCxxModuleCallback(runtime, wrapper);
     } else if (method.callbacks == 2) {
-      auto wrapper1 = std::make_shared<CallbackWrapper>(args[count - 2].getObject(runtime).getFunction(runtime), runtime, jsInvoker_);
-      auto wrapper2 = std::make_shared<CallbackWrapper>(args[count - 1].getObject(runtime).getFunction(runtime), runtime, jsInvoker_);
+      auto wrapper1 = std::make_shared<CallbackWrapper>(
+          args[count - 2].getObject(runtime).getFunction(runtime),
+          runtime,
+          jsInvoker_);
+      auto wrapper2 = std::make_shared<CallbackWrapper>(
+          args[count - 1].getObject(runtime).getFunction(runtime),
+          runtime,
+          jsInvoker_);
       first = makeTurboCxxModuleCallback(runtime, wrapper1);
       second = makeTurboCxxModuleCallback(runtime, wrapper2);
     }
@@ -125,19 +157,26 @@ jsi::Value TurboCxxModule::invokeMethod(
 
     method.func(std::move(innerArgs), first, second);
   } else if (method.isPromise) {
-    return createPromiseAsJSIValue(runtime, [method, args, count, this](jsi::Runtime &rt, std::shared_ptr<Promise> promise) {
-      auto resolveWrapper = std::make_shared<CallbackWrapper>(promise->resolve_.getFunction(rt), rt, jsInvoker_);
-      auto rejectWrapper = std::make_shared<CallbackWrapper>(promise->reject_.getFunction(rt), rt, jsInvoker_);
-      CxxModule::Callback resolve = makeTurboCxxModuleCallback(rt, resolveWrapper);
-      CxxModule::Callback reject = makeTurboCxxModuleCallback(rt, rejectWrapper);
-
-      auto innerArgs = folly::dynamic::array();
-      for (size_t i = 0; i < count; i++) {
-        innerArgs.push_back(jsi::dynamicFromValue(rt, args[i]));
-      }
+    return createPromiseAsJSIValue(
+        runtime,
+        [method, args, count, this](
+            jsi::Runtime &rt, std::shared_ptr<Promise> promise) {
+          auto resolveWrapper = std::make_shared<CallbackWrapper>(
+              promise->resolve_.getFunction(rt), rt, jsInvoker_);
+          auto rejectWrapper = std::make_shared<CallbackWrapper>(
+              promise->reject_.getFunction(rt), rt, jsInvoker_);
+          CxxModule::Callback resolve =
+              makeTurboCxxModuleCallback(rt, resolveWrapper);
+          CxxModule::Callback reject =
+              makeTurboCxxModuleCallback(rt, rejectWrapper);
+
+          auto innerArgs = folly::dynamic::array();
+          for (size_t i = 0; i < count; i++) {
+            innerArgs.push_back(jsi::dynamicFromValue(rt, args[i]));
+          }
 
-      method.func(std::move(innerArgs), resolve, reject);
-    });
+          method.func(std::move(innerArgs), resolve, reject);
+        });
   }
 
   return jsi::Value::undefined();

ReactCommon/turbomodule/core/TurboModuleUtils.h

@@ -7,8 +7,10 @@
 
 #pragma once
 
+#include <cassert>
 #include <string>
 
+#include <folly/Optional.h>
 #include <jsi/jsi.h>
 
 #include <ReactCommon/JSCallInvoker.h>
@@ -32,18 +34,61 @@ struct Promise {
   jsi::Function reject_;
 };
 
-using PromiseSetupFunctionType = std::function<void(jsi::Runtime &rt, std::shared_ptr<Promise>)>;
-jsi::Value createPromiseAsJSIValue(jsi::Runtime &rt, const PromiseSetupFunctionType func);
+using PromiseSetupFunctionType =
+    std::function<void(jsi::Runtime &rt, std::shared_ptr<Promise>)>;
+jsi::Value createPromiseAsJSIValue(
+    jsi::Runtime &rt,
+    const PromiseSetupFunctionType func);
 
 // Helper for passing jsi::Function arg to other methods.
-struct CallbackWrapper {
-  CallbackWrapper(jsi::Function callback, jsi::Runtime &runtime, std::shared_ptr<react::JSCallInvoker> jsInvoker)
-  : callback(std::move(callback)),
-    runtime(runtime),
-    jsInvoker(jsInvoker) {}
-  jsi::Function callback;
-  jsi::Runtime &runtime;
-  std::shared_ptr<react::JSCallInvoker> jsInvoker;
+class CallbackWrapper {
+ private:
+  struct Data {
+    Data(
+        jsi::Function callback,
+        jsi::Runtime &runtime,
+        std::shared_ptr<react::JSCallInvoker> jsInvoker)
+        : callback(std::move(callback)),
+          runtime(runtime),
+          jsInvoker(std::move(jsInvoker)) {}
+
+    jsi::Function callback;
+    jsi::Runtime &runtime;
+    std::shared_ptr<react::JSCallInvoker> jsInvoker;
+  };
+
+  folly::Optional<Data> data_;
+
+ public:
+  CallbackWrapper(
+      jsi::Function callback,
+      jsi::Runtime &runtime,
+      std::shared_ptr<react::JSCallInvoker> jsInvoker)
+      : data_(Data{std::move(callback), runtime, jsInvoker}) {}
+
+  // Delete the enclosed jsi::Function
+  void destroy() {
+    data_ = folly::none;
+  }
+
+  bool isDestroyed() {
+    return !data_.hasValue();
+  }
+
+  jsi::Function &callback() {
+    assert(!isDestroyed());
+    return data_->callback;
+  }
+
+  jsi::Runtime &runtime() {
+    assert(!isDestroyed());
+    return data_->runtime;
+  }
+
+  react::JSCallInvoker &jsInvoker() {
+    assert(!isDestroyed());
+    return *(data_->jsInvoker);
+  }
 };
 
 } // namespace react