From bfb0319ce0c7440c1a41a24eba0988222d74f9ee Mon Sep 17 00:00:00 2001 From: Alex Hunt Date: Tue, 26 Mar 2024 17:52:52 -0700 Subject: [PATCH] Register CatalystInstanceImpl with modern CDP backend (2/2) (#43251) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/43251 Integrates the modern CDP backend with `CatalystInstanceImpl` (the React Native instance implementation) on Android. This complete the modern CDP integration for Bridge. Changelog: [Internal] Reviewed By: motiz88 Differential Revision: D51458010 fbshipit-source-id: 6f73868da9d0d4cc5d086a4569c78444cb1b83ec --- .../ReactAndroid/api/ReactAndroid.api | 1 + .../facebook/react/ReactInstanceManager.java | 5 +- .../react/bridge/CatalystInstanceImpl.java | 23 +++++- .../jni/react/jni/CatalystInstanceImpl.cpp | 17 +++- .../main/jni/react/jni/CatalystInstanceImpl.h | 12 ++- .../ReactCommon/cxxreact/Instance.cpp | 81 +++++++++++++------ 6 files changed, 106 insertions(+), 33 deletions(-) diff --git a/packages/react-native/ReactAndroid/api/ReactAndroid.api b/packages/react-native/ReactAndroid/api/ReactAndroid.api index 21703cb8afeb2e..82ff782176deac 100644 --- a/packages/react-native/ReactAndroid/api/ReactAndroid.api +++ b/packages/react-native/ReactAndroid/api/ReactAndroid.api @@ -619,6 +619,7 @@ public class com/facebook/react/bridge/CatalystInstanceImpl : com/facebook/react public class com/facebook/react/bridge/CatalystInstanceImpl$Builder { public fun ()V public fun build ()Lcom/facebook/react/bridge/CatalystInstanceImpl; + public fun setInspectorTarget (Lcom/facebook/react/bridge/ReactInstanceManagerInspectorTarget;)Lcom/facebook/react/bridge/CatalystInstanceImpl$Builder; public fun setJSBundleLoader (Lcom/facebook/react/bridge/JSBundleLoader;)Lcom/facebook/react/bridge/CatalystInstanceImpl$Builder; public fun setJSExceptionHandler (Lcom/facebook/react/bridge/JSExceptionHandler;)Lcom/facebook/react/bridge/CatalystInstanceImpl$Builder; public fun setJSExecutor (Lcom/facebook/react/bridge/JavaScriptExecutor;)Lcom/facebook/react/bridge/CatalystInstanceImpl$Builder; diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java index a1b218dda4d837..7eed8007bfb565 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java @@ -1360,15 +1360,14 @@ private ReactApplicationContext createReactContext( NativeModuleRegistry nativeModuleRegistry = processPackages(reactContext, mPackages); - getOrCreateInspectorTarget(); - CatalystInstanceImpl.Builder catalystInstanceBuilder = new CatalystInstanceImpl.Builder() .setReactQueueConfigurationSpec(ReactQueueConfigurationSpec.createDefault()) .setJSExecutor(jsExecutor) .setRegistry(nativeModuleRegistry) .setJSBundleLoader(jsBundleLoader) - .setJSExceptionHandler(exceptionHandler); + .setJSExceptionHandler(exceptionHandler) + .setInspectorTarget(getOrCreateInspectorTarget()); ReactMarker.logMarker(CREATE_CATALYST_INSTANCE_START); // CREATE_CATALYST_INSTANCE_END is in JSCExecutor.cpp diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java index f86dffbf341dd2..7333289de793d0 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java @@ -119,7 +119,8 @@ private CatalystInstanceImpl( final JavaScriptExecutor jsExecutor, final NativeModuleRegistry nativeModuleRegistry, final JSBundleLoader jsBundleLoader, - JSExceptionHandler jSExceptionHandler) { + JSExceptionHandler jSExceptionHandler, + @Nullable ReactInstanceManagerInspectorTarget inspectorTarget) { FLog.d(ReactConstants.TAG, "Initializing React Xplat Bridge."); Systrace.beginSection(TRACE_TAG_REACT_JAVA_BRIDGE, "createCatalystInstanceImpl"); @@ -146,7 +147,8 @@ private CatalystInstanceImpl( mReactQueueConfiguration.getJSQueueThread(), mNativeModulesQueueThread, mNativeModuleRegistry.getJavaModules(this), - mNativeModuleRegistry.getCxxModules()); + mNativeModuleRegistry.getCxxModules(), + inspectorTarget); FLog.d(ReactConstants.TAG, "Initializing React Xplat Bridge after initializeBridge"); Systrace.endSection(TRACE_TAG_REACT_JAVA_BRIDGE); @@ -214,7 +216,8 @@ private native void initializeBridge( MessageQueueThread jsQueue, MessageQueueThread moduleQueue, Collection javaModules, - Collection cxxModules); + Collection cxxModules, + @Nullable ReactInstanceManagerInspectorTarget inspectorTarget); @Override public void setSourceURLs(String deviceURL, String remoteURL) { @@ -329,6 +332,8 @@ public void invokeCallback(final int callbackID, final NativeArrayInterface argu jniCallJSCallback(callbackID, (NativeArray) arguments); } + private native void unregisterFromInspector(); + /** * Destroys this catalyst instance, waiting for any other threads in ReactQueueConfiguration * (besides the UI thread) to finish running. Must be called from the UI thread so that we can @@ -343,6 +348,8 @@ public void destroy() { return; } + unregisterFromInspector(); + // TODO: tell all APIs to shut down ReactMarker.logMarker(ReactMarkerConstants.DESTROY_CATALYST_INSTANCE_START); mDestroyed = true; @@ -655,6 +662,7 @@ public static class Builder { private @Nullable NativeModuleRegistry mRegistry; private @Nullable JavaScriptExecutor mJSExecutor; private @Nullable JSExceptionHandler mJSExceptionHandler; + private @Nullable ReactInstanceManagerInspectorTarget mInspectorTarget; public Builder setReactQueueConfigurationSpec( ReactQueueConfigurationSpec ReactQueueConfigurationSpec) { @@ -682,13 +690,20 @@ public Builder setJSExceptionHandler(JSExceptionHandler handler) { return this; } + public Builder setInspectorTarget( + @Nullable ReactInstanceManagerInspectorTarget inspectorTarget) { + mInspectorTarget = inspectorTarget; + return this; + } + public CatalystInstanceImpl build() { return new CatalystInstanceImpl( Assertions.assertNotNull(mReactQueueConfigurationSpec), Assertions.assertNotNull(mJSExecutor), Assertions.assertNotNull(mRegistry), Assertions.assertNotNull(mJSBundleLoader), - Assertions.assertNotNull(mJSExceptionHandler)); + Assertions.assertNotNull(mJSExceptionHandler), + mInspectorTarget); } } } diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp index 4bc161bdb6b76e..8dccfab349aa05 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp @@ -6,6 +6,7 @@ */ #include "CatalystInstanceImpl.h" +#include "ReactInstanceManagerInspectorTarget.h" #include #include @@ -125,6 +126,9 @@ void CatalystInstanceImpl::registerNatives() { "getRuntimeExecutor", CatalystInstanceImpl::getRuntimeExecutor), makeNativeMethod( "getRuntimeScheduler", CatalystInstanceImpl::getRuntimeScheduler), + makeNativeMethod( + "unregisterFromInspector", + CatalystInstanceImpl::unregisterFromInspector), }); } @@ -157,7 +161,9 @@ void CatalystInstanceImpl::initializeBridge( jni::alias_ref::javaobject> javaModules, jni::alias_ref::javaobject> - cxxModules) { + cxxModules, + jni::alias_ref + inspectorTarget) { set_react_native_logfunc(&log); // TODO mhorowitz: how to assert here? @@ -193,7 +199,10 @@ void CatalystInstanceImpl::initializeBridge( std::make_unique(callback), jseh->getExecutorFactory(), std::make_unique(jsQueue), - moduleRegistry_); + moduleRegistry_, + inspectorTarget != nullptr + ? inspectorTarget->cthis()->getInspectorTarget() + : nullptr); } void CatalystInstanceImpl::extendNativeModules( @@ -406,4 +415,8 @@ CatalystInstanceImpl::getRuntimeScheduler() { return runtimeScheduler_; } +void CatalystInstanceImpl::unregisterFromInspector() { + instance_->unregisterFromInspector(); +} + } // namespace facebook::react diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h b/packages/react-native/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h index 6e16f11bdc5a6a..a81b3f6c2d0f40 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h +++ b/packages/react-native/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h @@ -19,6 +19,7 @@ #include "JSLoader.h" #include "JavaModuleWrapper.h" #include "ModuleRegistryBuilder.h" +#include "ReactInstanceManagerInspectorTarget.h" namespace facebook::react { @@ -59,7 +60,9 @@ class CatalystInstanceImpl : public jni::HybridClass { jni::JCollection::javaobject> javaModules, jni::alias_ref::javaobject> - cxxModules); + cxxModules, + jni::alias_ref + inspectorTarget); void extendNativeModules( jni::alias_ref { void createAndInstallRuntimeSchedulerIfNecessary(); + /** + * Unregisters the instance from the inspector. This method must be called + * on the main thread, after initializeBridge has finished executing and + * before the destructor for Instance has started. + */ + void unregisterFromInspector(); + // This should be the only long-lived strong reference, but every C++ class // will have a weak reference. std::shared_ptr instance_; diff --git a/packages/react-native/ReactCommon/cxxreact/Instance.cpp b/packages/react-native/ReactCommon/cxxreact/Instance.cpp index ed0a2443efcf5d..06de3adb8e6fdb 100644 --- a/packages/react-native/ReactCommon/cxxreact/Instance.cpp +++ b/packages/react-native/ReactCommon/cxxreact/Instance.cpp @@ -58,32 +58,67 @@ void Instance::initializeBridge( callback_ = std::move(callback); moduleRegistry_ = std::move(moduleRegistry); parentInspectorTarget_ = parentInspectorTarget; - jsQueue->runOnQueueSync( - [this, &jsef, jsQueue, parentInspectorTarget]() mutable { - nativeToJsBridge_ = std::make_shared( - jsef.get(), moduleRegistry_, jsQueue, callback_); - - nativeToJsBridge_->initializeRuntime(); - - if (parentInspectorTarget) { - inspectorTarget_ = &parentInspectorTarget->registerInstance(*this); - RuntimeExecutor runtimeExecutorIfJsi = getRuntimeExecutor(); - runtimeInspectorTarget_ = &inspectorTarget_->registerRuntime( - nativeToJsBridge_->getInspectorTargetDelegate(), - runtimeExecutorIfJsi ? runtimeExecutorIfJsi : [](auto) {}); + + jsQueue->runOnQueueSync([this, &jsef, jsQueue]() mutable { + nativeToJsBridge_ = std::make_shared( + jsef.get(), moduleRegistry_, jsQueue, callback_); + + // If a parent inspector HostTarget is provided, perform inspector + // initialization synchronously. + if (parentInspectorTarget_ != nullptr) { + auto inspectorExecutor = parentInspectorTarget_->executorFromThis(); + std::mutex inspectorInitializedMutex; + std::condition_variable inspectorInitializedCv; + bool inspectorInitialized = false; + + // Schedule work on the inspector thread. inspectorExecutor is guaranteed + // to execute this callback, so we can safely run this logic + // synchronously and reference parentInspectorTarget_. + inspectorExecutor([this, + &inspectorInitialized, + &inspectorInitializedMutex, + &inspectorInitializedCv]( + jsinspector_modern::HostTarget& hostTarget) { + // NOTE: By passing *this, we strongly assume the Instance will still + // be alive by the time this executes. + // - On iOS, instance creation is done syncrhonously + // (RCTCxxBridge#_initializeBridgeLocked). + // - On Android, we explicitly wait for instance creation before + // destruction (ReactInstanceManager#mReactContextLock). + inspectorTarget_ = &hostTarget.registerInstance(*this); + RuntimeExecutor runtimeExecutorIfJsi = getRuntimeExecutor(); + runtimeInspectorTarget_ = &inspectorTarget_->registerRuntime( + nativeToJsBridge_->getInspectorTargetDelegate(), + runtimeExecutorIfJsi ? runtimeExecutorIfJsi : [](auto) {}); + + // Signal that initialization is complete + { + std::lock_guard lock(inspectorInitializedMutex); + inspectorInitialized = true; } + inspectorInitializedCv.notify_one(); + }); + + // Wait for the initialization work to complete + { + std::unique_lock lock(inspectorInitializedMutex); + inspectorInitializedCv.wait( + lock, [&inspectorInitialized] { return inspectorInitialized; }); + } + } - /** - * After NativeToJsBridge is created, the jsi::Runtime should exist. - * Also, the JS message queue thread exists. So, it's safe to - * schedule all queued up js Calls. - */ - jsCallInvoker_->setNativeToJsBridgeAndFlushCalls(nativeToJsBridge_); + // Initialize the JavaScript runtime after we've initialized the inspector + nativeToJsBridge_->initializeRuntime(); - std::scoped_lock lock(m_syncMutex); - m_syncReady = true; - m_syncCV.notify_all(); - }); + // After NativeToJsBridge is created, the jsi::Runtime should exist. Also, + // the JS message queue thread exists. So, it's safe to schedule all queued + // up JS calls. + jsCallInvoker_->setNativeToJsBridgeAndFlushCalls(nativeToJsBridge_); + + std::scoped_lock lock(m_syncMutex); + m_syncReady = true; + m_syncCV.notify_all(); + }); CHECK(nativeToJsBridge_); }