Make inspector executor safe to destroy on any thread

Summary:
Changelog: [Internal]

Fixes a crash that may happen on Android when the `inspectorEnableModernCDPRegistry` feature flag is true, and clarifies the documentation of `HostTarget::create()` to avoid similar issues in future integrations.

## Context

The executor provided to `HostTarget::create()` ("the inspector executor") is used throughout the CDP backend to schedule work on the inspector thread. (See also D53356953.) To facilitate this, the executor is a copyable `std::function`.

On Android, the executor is backed by a Java object, to which we hold a reference from C++ using JNI. This reference is expressed as a `facebook::jni::global_ref` which gets copied as part of the executor. (`global_ref` is a RAII wrapper around the `NewGlobalRef` / `DeleteGlobalRef` JNI functions.)

## The bug

All the *calls* to the inspector executor from C++ happen on threads that are already properly [attached to the JVM](https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/invocation.html#attaching_to_the_vm) ( = the UI thread / the JS thread) and are therefore allowed to make JNI calls. However, there are cases where a copy of the inspector executor may have its *destructor* run on an unexpected thread, namely the (Hermes) JS GC thread. (This happens when the executor itself is captured in a lambda that's stored in a `jsi::HostObject` or `jsi::HostFunction`, which is in turn [destroyed on the JS GC thread](https://github.com/facebook/react-native/blob/5a0ae6e2d9d7f2357f9ea6c5dc1d573233075326/packages/react-native/ReactCommon/jsi/jsi/jsi.h#L118-L126).) In such cases, the `DeleteGlobalRef` call mentioned above will crash, because the JS GC thread is not attached to the JVM.

## The fix

First, we document that copies of the inspector executor provided to `HostTarget::create` may indeed be destroyed on arbitrary threads. This is an unavoidable consequence of the existing design. Second, we adapt the Android integration to this requirement.

`fbjni` provides the [`jni::ThreadScope`](https://github.com/facebookincubator/fbjni/blob/968e3815f92aeb0670f5d88ae975fbbd47a4b482/cxx/fbjni/detail/Environment.h#L93-L123) RAII helper to manage attaching C++ threads to the JVM. If we had any explicit control over the setup and teardown of the JS GC thread, we could create a single `jni::ThreadScope` to globally ensure the safety of JS-finalizer-to-JNI calls in React Native. However, neither JSI nor the Hermes API provides such control.

Instead, we essentially resort to creating a temporary `ThreadScope` around each `DeleteGlobalRef` call where we don't control the calling thread. We do this using a new kind of JNI reference wrapper class called `SafeReleaseJniRef`. Retaining a `SafeReleaseJniRef` instead of a plain `global_ref` is all that's needed to make a particular reference safe to destroy on any thread.

Reviewed By: huntie

Differential Revision: D56620131

fbshipit-source-id: 0b6f32a7bd6477d0384af19c42e21d9242ce623d

packages/react-native/ReactAndroid/src/main/jni/react/jni/ReactInstanceManagerInspectorTarget.cpp

@@ -6,6 +6,7 @@
  */
 
 #include "ReactInstanceManagerInspectorTarget.h"
+#include "SafeReleaseJniRef.h"
 
 #include <fbjni/NativeRunnable.h>
 #include <jsinspector-modern/InspectorFlags.h>
@@ -39,7 +40,11 @@ ReactInstanceManagerInspectorTarget::ReactInstanceManagerInspectorTarget(
 
   if (inspectorFlags.getEnableModernCDPRegistry()) {
     inspectorTarget_ = HostTarget::create(
-        *this, [javaExecutor = make_global(executor)](auto callback) mutable {
+        *this,
+        [javaExecutor =
+             // Use a SafeReleaseJniRef because this lambda may be copied to
+             // arbitrary threads.
+         SafeReleaseJniRef(make_global(executor))](auto callback) mutable {
           auto jrunnable =
               JNativeRunnable::newObjectCxxArgs(std::move(callback));
           javaExecutor->execute(jrunnable);

packages/react-native/ReactAndroid/src/main/jni/react/jni/SafeReleaseJniRef.h

@@ -0,0 +1,73 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+#include <fbjni/fbjni.h>
+
+#include <type_traits>
+#include <utility>
+
+namespace facebook::react {
+
+/**
+ * A wrapper around a JNI reference (e.g. jni::global_ref<>) that makes it safe
+ * to destroy (decrement refcount) from any thread. This wrapping is necessary
+ * when we don't have control over the thread where a given JNI reference's
+ * destructor will run.
+ *
+ * In particular, this is needed when a JNI reference is owned by a JSI
+ * HostObject or HostFunction, since those may be destroyed on an arbitrary
+ * thread (e.g. ther Hermes GC thread) which may not be attached to the JVM.
+ * (This is explicitly documented for HostObject, and is the observed behavior
+ * in Hermes for HostFunctions.)
+ */
+template <typename RefT>
+class SafeReleaseJniRef {
+  using T = std::remove_reference<decltype(*std::declval<RefT>())>::type;
+
+ public:
+  /* explicit */ SafeReleaseJniRef(RefT ref) : ref_(std::move(ref)) {}
+  SafeReleaseJniRef(const SafeReleaseJniRef& other) = default;
+  SafeReleaseJniRef(SafeReleaseJniRef&& other) = default;
+  SafeReleaseJniRef& operator=(const SafeReleaseJniRef& other) = default;
+  SafeReleaseJniRef& operator=(SafeReleaseJniRef&& other) = default;
+
+  ~SafeReleaseJniRef() {
+    if (ref_) {
+      jni::ThreadScope ts;
+      ref_.reset();
+    }
+  }
+
+  operator bool() const noexcept {
+    return (bool)ref_;
+  }
+
+  T& operator*() noexcept {
+    return *ref_;
+  }
+
+  T* operator->() noexcept {
+    return &*ref_;
+  }
+
+  const T& operator*() const noexcept {
+    return *ref_;
+  }
+
+  const T* operator->() const noexcept {
+    return &*ref_;
+  }
+
+  operator RefT() const {
+    return ref_;
+  }
+
+ private:
+  RefT ref_;
+};
+
+} // namespace facebook::react

packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.cpp

@@ -8,6 +8,7 @@
 #include "JReactHostInspectorTarget.h"
 #include <fbjni/NativeRunnable.h>
 #include <jsinspector-modern/InspectorFlags.h>
+#include <react/jni/SafeReleaseJniRef.h>
 
 using namespace facebook::jni;
 using namespace facebook::react::jsinspector_modern;
@@ -23,7 +24,10 @@ JReactHostInspectorTarget::JReactHostInspectorTarget(
     inspectorTarget_ = HostTarget::create(
         *this,
         [javaExecutor =
-             javaExecutor_](std::function<void()>&& callback) mutable {
+             // Use a SafeReleaseJniRef because this lambda may be copied to
+             // arbitrary threads.
+         SafeReleaseJniRef(javaExecutor_)](
+            std::function<void()>&& callback) mutable {
           auto jrunnable =
               JNativeRunnable::newObjectCxxArgs(std::move(callback));
           javaExecutor->execute(jrunnable);

packages/react-native/ReactCommon/jsinspector-modern/HostTarget.h

@@ -162,6 +162,10 @@ class JSINSPECTOR_EXPORT HostTarget
    * \param executor An executor that may be used to call methods on this
    * HostTarget while it exists. \c create additionally guarantees that the
    * executor will not be called after the HostTarget is destroyed.
+   * \note Copies of the provided executor may be destroyed on arbitrary
+   * threads, including after the HostTarget is destroyed. Callers must ensure
+   * that such destructor calls are safe - e.g. if using a lambda as the
+   * executor, all captured values must be safe to destroy from any thread.
    */
   static std::shared_ptr<HostTarget> create(
       HostTargetDelegate& delegate,