From c365360ebfbcc86dde878b7eaa05da0afc9539ce Mon Sep 17 00:00:00 2001 From: Alan Lee Date: Thu, 13 Jun 2024 00:48:28 -0700 Subject: [PATCH] add default early JS error handler (#44884) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/44884 Removing JavaScript error handler supplied to ReactHostImpl.java which is just a stub and creating a default handler in ReactInstance.java which uses NativeExceptionHandler TurboModule to handle error. Changelog: [Android][BREAKING] Removing `ReactJsExceptionHandler` param from ReactHostImpl() constructor and providing a default private implementation Differential Revision: D58385767 --- .../ReactAndroid/api/ReactAndroid.api | 4 +- .../react/defaults/DefaultReactHost.kt | 4 -- .../facebook/react/runtime/ReactHostImpl.java | 7 --- .../facebook/react/runtime/ReactInstance.java | 52 ++++++++++++++++++- .../facebook/react/runtime/ReactHostTest.kt | 7 +-- 5 files changed, 53 insertions(+), 21 deletions(-) diff --git a/packages/react-native/ReactAndroid/api/ReactAndroid.api b/packages/react-native/ReactAndroid/api/ReactAndroid.api index 3112b7cf36af02..94fb364725106d 100644 --- a/packages/react-native/ReactAndroid/api/ReactAndroid.api +++ b/packages/react-native/ReactAndroid/api/ReactAndroid.api @@ -3778,8 +3778,8 @@ public abstract class com/facebook/react/runtime/JSRuntimeFactory { } public class com/facebook/react/runtime/ReactHostImpl : com/facebook/react/ReactHost { - public fun (Landroid/content/Context;Lcom/facebook/react/runtime/ReactHostDelegate;Lcom/facebook/react/fabric/ComponentFactory;Ljava/util/concurrent/Executor;Ljava/util/concurrent/Executor;Lcom/facebook/react/interfaces/exceptionmanager/ReactJsExceptionHandler;ZZ)V - public fun (Landroid/content/Context;Lcom/facebook/react/runtime/ReactHostDelegate;Lcom/facebook/react/fabric/ComponentFactory;ZLcom/facebook/react/interfaces/exceptionmanager/ReactJsExceptionHandler;Z)V + public fun (Landroid/content/Context;Lcom/facebook/react/runtime/ReactHostDelegate;Lcom/facebook/react/fabric/ComponentFactory;Ljava/util/concurrent/Executor;Ljava/util/concurrent/Executor;ZZ)V + public fun (Landroid/content/Context;Lcom/facebook/react/runtime/ReactHostDelegate;Lcom/facebook/react/fabric/ComponentFactory;ZZ)V public fun addBeforeDestroyListener (Lkotlin/jvm/functions/Function0;)V public fun addReactInstanceEventListener (Lcom/facebook/react/ReactInstanceEventListener;)V public fun createSurface (Landroid/content/Context;Ljava/lang/String;Landroid/os/Bundle;)Lcom/facebook/react/interfaces/fabric/ReactSurface; diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/defaults/DefaultReactHost.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/defaults/DefaultReactHost.kt index b32e79c61cc6ba..06452e525015b4 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/defaults/DefaultReactHost.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/defaults/DefaultReactHost.kt @@ -17,7 +17,6 @@ import com.facebook.react.bridge.ReactContext import com.facebook.react.common.annotations.UnstableReactNativeAPI import com.facebook.react.common.build.ReactBuildConfig import com.facebook.react.fabric.ComponentFactory -import com.facebook.react.interfaces.exceptionmanager.ReactJsExceptionHandler import com.facebook.react.runtime.JSCInstance import com.facebook.react.runtime.ReactHostImpl import com.facebook.react.runtime.cxxreactpackage.CxxReactPackage @@ -74,8 +73,6 @@ public object DefaultReactHost { reactPackages = packageList, jsRuntimeFactory = jsRuntimeFactory, turboModuleManagerDelegateBuilder = defaultTmmDelegateBuilder) - // TODO: T180971255 Improve default exception handler - val reactJsExceptionHandler = ReactJsExceptionHandler { _ -> } val componentFactory = ComponentFactory() DefaultComponentsRegistry.register(componentFactory) // TODO: T164788699 find alternative of accessing ReactHostImpl for initialising reactHost @@ -85,7 +82,6 @@ public object DefaultReactHost { defaultReactHostDelegate, componentFactory, true /* allowPackagerServerAccess */, - reactJsExceptionHandler, useDevSupport, ) .apply { 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 baa00162754930..c34c652c93edc5 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 @@ -57,7 +57,6 @@ import com.facebook.react.fabric.ComponentFactory; import com.facebook.react.fabric.FabricUIManager; import com.facebook.react.interfaces.TaskInterface; -import com.facebook.react.interfaces.exceptionmanager.ReactJsExceptionHandler; import com.facebook.react.interfaces.fabric.ReactSurface; import com.facebook.react.internal.featureflags.ReactNativeFeatureFlags; import com.facebook.react.modules.appearance.AppearanceModule; @@ -102,7 +101,6 @@ public class ReactHostImpl implements ReactHost { private final Context mContext; private final ReactHostDelegate mReactHostDelegate; private final ComponentFactory mComponentFactory; - private final ReactJsExceptionHandler mReactJsExceptionHandler; private final DevSupportManager mDevSupportManager; private final Executor mBGExecutor; private final Executor mUIExecutor; @@ -143,7 +141,6 @@ public ReactHostImpl( ReactHostDelegate delegate, ComponentFactory componentFactory, boolean allowPackagerServerAccess, - ReactJsExceptionHandler reactJsExceptionHandler, boolean useDevSupport) { this( context, @@ -151,7 +148,6 @@ public ReactHostImpl( componentFactory, Executors.newSingleThreadExecutor(), Task.UI_THREAD_EXECUTOR, - reactJsExceptionHandler, allowPackagerServerAccess, useDevSupport); } @@ -162,7 +158,6 @@ public ReactHostImpl( ComponentFactory componentFactory, Executor bgExecutor, Executor uiExecutor, - ReactJsExceptionHandler reactJsExceptionHandler, boolean allowPackagerServerAccess, boolean useDevSupport) { mContext = context; @@ -170,7 +165,6 @@ public ReactHostImpl( mComponentFactory = componentFactory; mBGExecutor = bgExecutor; mUIExecutor = uiExecutor; - mReactJsExceptionHandler = reactJsExceptionHandler; mQueueThreadExceptionHandler = ReactHostImpl.this::handleHostException; mMemoryPressureRouter = new MemoryPressureRouter(context); mAllowPackagerServerAccess = allowPackagerServerAccess; @@ -1067,7 +1061,6 @@ private Task getOrCreateReactInstanceTask() { mComponentFactory, devSupportManager, mQueueThreadExceptionHandler, - mReactJsExceptionHandler, mUseDevSupport, getOrCreateReactHostInspectorTarget()); diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactInstance.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactInstance.java index 107ada4b40ca4f..f4d1eede8342ae 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactInstance.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactInstance.java @@ -7,9 +7,16 @@ package com.facebook.react.runtime; +import static com.facebook.react.util.JSStackTrace.COLUMN_KEY; +import static com.facebook.react.util.JSStackTrace.FILE_KEY; +import static com.facebook.react.util.JSStackTrace.LINE_NUMBER_KEY; +import static com.facebook.react.util.JSStackTrace.METHOD_NAME_KEY; + import android.content.res.AssetManager; import android.view.View; import com.facebook.common.logging.FLog; +import com.facebook.fbreact.specs.NativeExceptionsManagerSpec; +import com.facebook.infer.annotation.Assertions; import com.facebook.infer.annotation.Nullsafe; import com.facebook.infer.annotation.ThreadConfined; import com.facebook.infer.annotation.ThreadSafe; @@ -21,12 +28,15 @@ import com.facebook.react.bridge.Arguments; import com.facebook.react.bridge.JSBundleLoader; import com.facebook.react.bridge.JSBundleLoaderDelegate; +import com.facebook.react.bridge.JavaOnlyArray; +import com.facebook.react.bridge.JavaOnlyMap; import com.facebook.react.bridge.JavaScriptContextHolder; import com.facebook.react.bridge.NativeArray; import com.facebook.react.bridge.NativeMap; import com.facebook.react.bridge.NativeModule; import com.facebook.react.bridge.ReactNoCrashSoftException; import com.facebook.react.bridge.ReactSoftExceptionLogger; +import com.facebook.react.bridge.ReadableMap; import com.facebook.react.bridge.RuntimeExecutor; import com.facebook.react.bridge.RuntimeScheduler; import com.facebook.react.bridge.queue.MessageQueueThread; @@ -110,7 +120,6 @@ final class ReactInstance { ComponentFactory componentFactory, DevSupportManager devSupportManager, QueueThreadExceptionHandler exceptionHandler, - ReactJsExceptionHandler reactExceptionManager, boolean useDevSupport, @Nullable ReactHostInspectorTarget reactHostInspectorTarget) { mBridgelessReactContext = bridgelessReactContext; @@ -154,6 +163,7 @@ final class ReactInstance { // Notify JS if profiling is enabled boolean isProfiling = Systrace.isTracing(Systrace.TRACE_TAG_REACT_APPS | Systrace.TRACE_TAG_REACT_JS_VM_CALLS); + mHybridData = initHybrid( jsRuntimeFactory, @@ -161,7 +171,7 @@ final class ReactInstance { nativeModulesMessageQueueThread, mJavaTimerManager, jsTimerExecutor, - reactExceptionManager, + new ReactJsExceptionHandlerImpl(nativeModulesMessageQueueThread), bindingsInstaller, isProfiling, reactHostInspectorTarget); @@ -311,6 +321,44 @@ public ReactQueueConfiguration getReactQueueConfiguration() { return mQueueConfiguration; } + private class ReactJsExceptionHandlerImpl implements ReactJsExceptionHandler { + private final MessageQueueThread mNativemodulesmessagequeuethread; + + ReactJsExceptionHandlerImpl(MessageQueueThread nativeModulesMessageQueueThread) { + this.mNativemodulesmessagequeuethread = nativeModulesMessageQueueThread; + } + + @Override + public void reportJsException(ParsedError error) { + List frames = error.getFrames(); + List readableMapList = new ArrayList<>(); + for (ReactJsExceptionHandler.ParsedError.StackFrame frame : frames) { + JavaOnlyMap map = new JavaOnlyMap(); + map.putDouble(COLUMN_KEY, frame.getColumnNumber()); + map.putDouble(LINE_NUMBER_KEY, frame.getLineNumber()); + map.putString(FILE_KEY, (String) frame.getFileName()); + map.putString(METHOD_NAME_KEY, (String) frame.getMethodName()); + readableMapList.add(map); + } + + JavaOnlyMap data = new JavaOnlyMap(); + data.putString("message", error.getMessage()); + data.putArray("stack", JavaOnlyArray.from(readableMapList)); + data.putInt("id", error.getExceptionId()); + data.putBoolean("isFatal", error.isFatal()); + + // Simulate async native module method call + mNativemodulesmessagequeuethread.runOnQueue( + () -> { + NativeExceptionsManagerSpec exceptionsManager = + (NativeExceptionsManagerSpec) + Assertions.assertNotNull( + mTurboModuleManager.getModule(NativeExceptionsManagerSpec.NAME)); + exceptionsManager.reportException(data); + }); + } + } + public void loadJSBundle(JSBundleLoader bundleLoader) { // Load the JS bundle Systrace.beginSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, "ReactInstance.loadJSBundle"); diff --git a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/runtime/ReactHostTest.kt b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/runtime/ReactHostTest.kt index 07d770eef585f3..ef83a8690b1c29 100644 --- a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/runtime/ReactHostTest.kt +++ b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/runtime/ReactHostTest.kt @@ -84,12 +84,7 @@ class ReactHostTest { Mockito.doReturn(jSBundleLoader).`when`(reactHostDelegate).jsBundleLoader reactHost = ReactHostImpl( - activityController.get().application, - reactHostDelegate, - componentFactory, - false, - {}, - false) + activityController.get().application, reactHostDelegate, componentFactory, false, false) val taskCompletionSource = TaskCompletionSource().apply { setResult(true) } mockedTaskCompletionSourceCtor = Mockito.mockConstruction(