Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make inspector executor safe to destroy on any thread #44272

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

motiz88
Copy link
Contributor

@motiz88 motiz88 commented Apr 26, 2024

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 ( = 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.) 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 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 SafeReleaseJniRef, a new kind of JNI reference wrapper class that does just this. 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.

Differential Revision: D56620131

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner labels Apr 26, 2024
@analysis-bot
Copy link

analysis-bot commented Apr 26, 2024

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 19,460,108 +5
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 22,833,639 +17
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: be06fd4
Branch: main

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 `SafeReleaseJniRef`, a new kind of JNI reference wrapper class that does just this. 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.

Differential Revision: D56620131
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants