From 8689f563ed8ed8c25b73f980927e323909e6673a Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Mon, 29 Apr 2024 05:24:53 -0700 Subject: [PATCH] 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 --- .../ReactInstanceManagerInspectorTarget.cpp | 7 +- .../main/jni/react/jni/SafeReleaseJniRef.h | 73 +++++++++++++++++++ .../runtime/jni/JReactHostInspectorTarget.cpp | 6 +- .../jsinspector-modern/HostTarget.h | 4 + 4 files changed, 88 insertions(+), 2 deletions(-) create mode 100644 packages/react-native/ReactAndroid/src/main/jni/react/jni/SafeReleaseJniRef.h diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReactInstanceManagerInspectorTarget.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReactInstanceManagerInspectorTarget.cpp index c97907bc63de2d..1188971031872c 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReactInstanceManagerInspectorTarget.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReactInstanceManagerInspectorTarget.cpp @@ -6,6 +6,7 @@ */ #include "ReactInstanceManagerInspectorTarget.h" +#include "SafeReleaseJniRef.h" #include #include @@ -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); diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/SafeReleaseJniRef.h b/packages/react-native/ReactAndroid/src/main/jni/react/jni/SafeReleaseJniRef.h new file mode 100644 index 00000000000000..65fb1f8502cd41 --- /dev/null +++ b/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 + +#include +#include + +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 +class SafeReleaseJniRef { + using T = std::remove_reference())>::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 diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.cpp index dbcfa3f0a8b509..170baac926f8a7 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.cpp @@ -8,6 +8,7 @@ #include "JReactHostInspectorTarget.h" #include #include +#include using namespace facebook::jni; using namespace facebook::react::jsinspector_modern; @@ -23,7 +24,10 @@ JReactHostInspectorTarget::JReactHostInspectorTarget( inspectorTarget_ = HostTarget::create( *this, [javaExecutor = - javaExecutor_](std::function&& callback) mutable { + // Use a SafeReleaseJniRef because this lambda may be copied to + // arbitrary threads. + SafeReleaseJniRef(javaExecutor_)]( + std::function&& callback) mutable { auto jrunnable = JNativeRunnable::newObjectCxxArgs(std::move(callback)); javaExecutor->execute(jrunnable); diff --git a/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.h b/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.h index 05fe80b97bce57..6e4fafbd30d2e6 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.h +++ b/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 create( HostTargetDelegate& delegate,