Make inspector executor safe to destroy on any thread #44272
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Changelog: [Internal]
Fixes a crash that may happen on Android when the
inspectorEnableModernCDPRegistry
feature flag is true, and clarifies the documentation ofHostTarget::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 copyablestd::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 theNewGlobalRef
/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
orjsi::HostFunction
, which is in turn destroyed on the JS GC thread.) In such cases, theDeleteGlobalRef
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 thejni::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 singlejni::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 eachDeleteGlobalRef
call where we don't control the calling thread. We do this usingSafeReleaseJniRef
, a new kind of JNI reference wrapper class that does just this. Retaining aSafeReleaseJniRef
instead of a plainglobal_ref
is all that's needed to make a particular reference safe to destroy on any thread.Differential Revision: D56620131