From 6d186f706cc57368bbed7d276d32dd861f966d19 Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Mon, 3 Jun 2024 10:20:55 -0700 Subject: [PATCH 1/2] Add Java assertions for order of Host/Instance inspector target teardown Summary: Changelog: [Internal] There is currently a lifecycle bug in both the Bridge (`com.facebook.react.bridge`) and Bridgeless (`com.facebook.react.runtime`) integrations of Fusebox in React Native Android, whereby `HostTarget::unregisterInstance` gets called after the `HostTarget` has been destroyed. This manifests as a handful of related C++ crashes depending on the exact circumstances and build flags. This diff makes the bug trigger a Java assertion instead of a C++ crash for ease of debugging. The next diff in the stack will actually fix the lifecycle issue. Differential Revision: D58031217 --- .../react/bridge/CatalystInstanceImpl.java | 11 ++++++++++- .../ReactInstanceManagerInspectorTarget.java | 6 ++++++ .../facebook/react/runtime/ReactHostImpl.java | 19 +++++++++++++------ .../react/runtime/ReactHostInspectorTarget.kt | 4 ++++ 4 files changed, 33 insertions(+), 7 deletions(-) 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 cbcd2f1c3d08..31fd36010ab2 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 @@ -7,6 +7,7 @@ package com.facebook.react.bridge; +import static com.facebook.infer.annotation.Assertions.assertCondition; import static com.facebook.infer.annotation.ThreadConfined.UI; import static com.facebook.systrace.Systrace.TRACE_TAG_REACT_JAVA_BRIDGE; @@ -112,6 +113,8 @@ public String toString() { public native NativeMethodCallInvokerHolderImpl getNativeMethodCallInvokerHolder(); + private @Nullable ReactInstanceManagerInspectorTarget mInspectorTarget; + private CatalystInstanceImpl( final ReactQueueConfigurationSpec reactQueueConfigurationSpec, final JavaScriptExecutor jsExecutor, @@ -134,6 +137,7 @@ private CatalystInstanceImpl( mJSExceptionHandler = jSExceptionHandler; mNativeModulesQueueThread = mReactQueueConfiguration.getNativeModulesQueueThread(); mTraceListener = new JSProfilerTraceListener(this); + mInspectorTarget = inspectorTarget; Systrace.endSection(TRACE_TAG_REACT_JAVA_BRIDGE); FLog.d(ReactConstants.TAG, "Initializing React Xplat Bridge before initializeBridge"); @@ -146,7 +150,7 @@ private CatalystInstanceImpl( mNativeModulesQueueThread, mNativeModuleRegistry.getJavaModules(this), mNativeModuleRegistry.getCxxModules(), - inspectorTarget); + mInspectorTarget); FLog.d(ReactConstants.TAG, "Initializing React Xplat Bridge after initializeBridge"); Systrace.endSection(TRACE_TAG_REACT_JAVA_BRIDGE); @@ -346,6 +350,11 @@ public void destroy() { return; } + if (mInspectorTarget != null) { + assertCondition( + mInspectorTarget.isValid(), + "ReactInstanceManager inspector target destroyed before instance was unregistered"); + } unregisterFromInspector(); // TODO: tell all APIs to shut down diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactInstanceManagerInspectorTarget.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactInstanceManagerInspectorTarget.java index f99291dabad9..ee60099bc2e4 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactInstanceManagerInspectorTarget.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactInstanceManagerInspectorTarget.java @@ -7,12 +7,14 @@ package com.facebook.react.bridge; +import com.facebook.infer.annotation.Nullsafe; import com.facebook.jni.HybridData; import com.facebook.proguard.annotations.DoNotStripAny; import java.util.concurrent.Executor; import javax.annotation.Nullable; @DoNotStripAny +@Nullsafe(Nullsafe.Mode.LOCAL) public class ReactInstanceManagerInspectorTarget implements AutoCloseable { public interface TargetDelegate { public void onReload(); @@ -46,6 +48,10 @@ public void close() { mHybridData.resetNative(); } + /*internal*/ boolean isValid() { + return mHybridData.isValid(); + } + static { ReactBridge.staticInit(); } diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostImpl.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostImpl.java index 67aa1aac94ee..51410faba7c1 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostImpl.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostImpl.java @@ -1333,9 +1333,7 @@ private Task getOrCreateReloadTask(String reason) { final ReactInstance reactInstance = reactInstanceTaskUnwrapper.unwrap(task, "1: Starting reload"); - if (reactInstance != null) { - reactInstance.unregisterFromInspector(); - } + unregisterInstanceFromInspector(reactInstance); final ReactContext reactContext = mBridgelessReactContextRef.getNullable(); if (reactContext == null) { @@ -1512,9 +1510,7 @@ private Task getOrCreateDestroyTask(final String reason, @Nullable Excepti final ReactInstance reactInstance = reactInstanceTaskUnwrapper.unwrap(task, "1: Starting destroy"); - if (reactInstance != null) { - reactInstance.unregisterFromInspector(); - } + unregisterInstanceFromInspector(reactInstance); // Step 1: Destroy DevSupportManager if (mUseDevSupport) { @@ -1678,4 +1674,15 @@ private void destroyReactHostInspectorTarget() { mReactHostInspectorTarget = null; } } + + private void unregisterInstanceFromInspector(final @Nullable ReactInstance reactInstance) { + if (reactInstance != null) { + if (InspectorFlags.getFuseboxEnabled()) { + Assertions.assertCondition( + mReactHostInspectorTarget != null && mReactHostInspectorTarget.isValid(), + "Host inspector target destroyed before instance was unregistered"); + } + reactInstance.unregisterFromInspector(); + } + } } diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostInspectorTarget.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostInspectorTarget.kt index c6f916155abd..be72c5f4d18e 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostInspectorTarget.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostInspectorTarget.kt @@ -30,6 +30,10 @@ internal class ReactHostInspectorTarget(private val reactHostImpl: ReactHostImpl mHybridData.resetNative() } + fun isValid(): Boolean { + return mHybridData.isValid() + } + private companion object { init { SoLoader.loadLibrary("rninstance") From dbe477ec44a1287ae12766959f10fb0527a2db3b Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Mon, 3 Jun 2024 10:20:55 -0700 Subject: [PATCH 2/2] Defer HostTarget destruction until after the instance has been unregistered Summary: Changelog: [Internal] Fixes a lifecycle bug in both the Bridge (`com.facebook.react.bridge`) and Bridgeless (`com.facebook.react.runtime`) integrations of Fusebox in React Native Android, whereby `HostTarget::unregisterInstance` gets called after the `HostTarget` has been destroyed. The solution consists of two parts: 1. If a ReactHost / InstanceManager is asked to destroy itself while it contains no active ReactInstance / ReactContext, we destroy the `HostTarget` immediately. 2. Otherwise, if there *is* a live ReactInstance / ReactContext that has yet to be destroyed, we wait for that to happen before destroying the `HostTarget`. In practice, we do this by checking for the BEFORE_CREATE ( = Host destroyed) lifecycle state every time we destroy a ReactInstance / ReactContext. Differential Revision: D58031215 --- .../com/facebook/react/ReactInstanceManager.java | 15 ++++++++++++--- .../facebook/react/runtime/ReactHostImpl.java | 16 ++++++++++++++-- 2 files changed, 26 insertions(+), 5 deletions(-) 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 ea77bef57315..41c3e9f086e1 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 @@ -701,7 +701,6 @@ public void onViewDetachedFromWindow(View v) { @Deprecated public void onHostDestroy() { UiThreadUtil.assertOnUiThread(); - destroyInspectorTarget(); if (mUseDeveloperSupport) { mDevSupportManager.setDevSupportEnabled(false); @@ -752,7 +751,6 @@ public void destroy() { } mHasStartedDestroying = true; - destroyInspectorTarget(); if (mUseDeveloperSupport) { mDevSupportManager.setDevSupportEnabled(false); @@ -782,6 +780,12 @@ public void destroy() { } } + // If the host is being destroyed, now that the current context/instance + // has been destroyed, we can safely destroy the host's inspector target. + if (mLifecycleState == LifecycleState.BEFORE_CREATE) { + destroyInspectorHostTarget(); + } + mHasStartedCreatingInitialContext = false; if (!mKeepActivity) { mCurrentActivity = null; @@ -835,6 +839,10 @@ private synchronized void moveToBeforeCreateLifecycleState() { if (mLifecycleState == LifecycleState.BEFORE_RESUME) { currentContext.onHostDestroy(mKeepActivity); } + } else { + // There's no current context that requires the host inspector target to + // be kept alive, so we can destroy it immediately. + destroyInspectorHostTarget(); } mLifecycleState = LifecycleState.BEFORE_CREATE; } @@ -1542,7 +1550,8 @@ public void onResume() { return mInspectorTarget; } - private void destroyInspectorTarget() { + @ThreadConfined(UI) + private void destroyInspectorHostTarget() { if (mInspectorTarget != null) { mInspectorTarget.close(); mInspectorTarget = null; diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostImpl.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostImpl.java index 51410faba7c1..6dccefca3dff 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostImpl.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostImpl.java @@ -892,7 +892,11 @@ private Task getOrCreateStartTask() { @ThreadConfined(UI) private void moveToHostDestroy(@Nullable ReactContext currentContext) { mReactLifecycleStateManager.moveToOnHostDestroy(currentContext); - destroyReactHostInspectorTarget(); + if (currentContext == null) { + // There's no current context/instance that requires the host inspector + // target to be kept alive, so we can destroy it immediately. + destroyInspectorHostTarget(); + } setCurrentActivity(null); } @@ -1668,13 +1672,15 @@ public void setJsEngineResolutionAlgorithm( return mReactHostInspectorTarget; } - private void destroyReactHostInspectorTarget() { + @ThreadConfined(UI) + private void destroyInspectorHostTarget() { if (mReactHostInspectorTarget != null) { mReactHostInspectorTarget.close(); mReactHostInspectorTarget = null; } } + @ThreadConfined(UI) private void unregisterInstanceFromInspector(final @Nullable ReactInstance reactInstance) { if (reactInstance != null) { if (InspectorFlags.getFuseboxEnabled()) { @@ -1684,5 +1690,11 @@ private void unregisterInstanceFromInspector(final @Nullable ReactInstance react } reactInstance.unregisterFromInspector(); } + if (mReactLifecycleStateManager.getLifecycleState() == LifecycleState.BEFORE_CREATE) { + // If the host is being destroyed, now that the current context/instance + // has been unregistered, we can safely destroy the host's inspector + // target. + destroyInspectorHostTarget(); + } } }