Skip to content

Commit 7af6cb4

Browse files
alanleedevfacebook-github-bot
authored andcommitted
add default early JS error handler (facebook#44884)
Summary: Pull Request resolved: facebook#44884 Removing JavaScript error handler supplied to ReactHostImpl.java which is just a stub and creating a default private handler in ReactInstance.java that routes exception to `NativeExceptionsManager` Changelog: [Android][BREAKING] Removing `ReactJsExceptionHandler` param from ReactHostImpl() constructor and providing a default private implementation **Internal:** - Provide default error handler that can handle early JavaScript error **Motivation** When app bundle is being loading and before JavaScript side error handling logic is executed, error that occur in between may be lost or not reported. Native code does have various `try/catch` statements to catch errors but in Android the error handler callback is implemented to be passed in while instantiating `ReactHostImpl` where many cases, including OSS, are just stub implementation that does not do anything. __The goal of this diff is to add a default error handler that can be used by both OSS and intern.__ When the JavaScript side error handling is fully setup any JavaScript error is routed to LogBox. For early errors, we want to utilize the native RedBox to display errors. Longer term goal is to have a single native pipeline but this diff is just to cover the early JS errors until this is realized. **Implementation details** The code changes were taken from previously abandoned diff by RSNara (D55439412). - **Previous** - `ReactJsExceptionHandler` (often just a stub) is created in Java and passed into `ReactHostImpl`. - **Updated** - `DefaultReactJsExceptionHandler` defined in `ReactInstance.java` is instantiated and passed into JNI/C++ via `initHybrid()` call. - `ReactJsExceptionHandler.reportJsException()` method is removed from `FbReactExceptionManager` as we are not using the default implementation above. **Related questions** ThreadQueue in Java uses a separate error handler. Question is if there a good reason for having a separate error handler of if we can combine this into a single error handler. Also since ThreadQueues are implemented in Java, there is a question of supporting the error handling via native C++ would be feasible or desirable. **Testing** You use the following preview diff to `throw Error` before error handling code is run during JavaScript bundle setup. It also includes Android log to check the method was called. `jf get --version 229685487` Testing was done using `fb4a` and `rntester-android`. **Note on inconsistent exception message in `rntester-android`**. Currently the only the first exception is displayed in RedBox and rest are ignored. Depending on the timing, early js exception is reported or `SurfaceRegistryBinding::startSurface()` is shown. This seems to be more related to clean up when `ReactInstance.loadJSScript()` throws and is not covered in this diff. Differential Revision: D58385767
1 parent b19bf2b commit 7af6cb4

5 files changed

Lines changed: 53 additions & 21 deletions

File tree

packages/react-native/ReactAndroid/api/ReactAndroid.api

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3778,8 +3778,8 @@ public abstract class com/facebook/react/runtime/JSRuntimeFactory {
37783778
}
37793779

37803780
public class com/facebook/react/runtime/ReactHostImpl : com/facebook/react/ReactHost {
3781-
public fun <init> (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
3782-
public fun <init> (Landroid/content/Context;Lcom/facebook/react/runtime/ReactHostDelegate;Lcom/facebook/react/fabric/ComponentFactory;ZLcom/facebook/react/interfaces/exceptionmanager/ReactJsExceptionHandler;Z)V
3781+
public fun <init> (Landroid/content/Context;Lcom/facebook/react/runtime/ReactHostDelegate;Lcom/facebook/react/fabric/ComponentFactory;Ljava/util/concurrent/Executor;Ljava/util/concurrent/Executor;ZZ)V
3782+
public fun <init> (Landroid/content/Context;Lcom/facebook/react/runtime/ReactHostDelegate;Lcom/facebook/react/fabric/ComponentFactory;ZZ)V
37833783
public fun addBeforeDestroyListener (Lkotlin/jvm/functions/Function0;)V
37843784
public fun addReactInstanceEventListener (Lcom/facebook/react/ReactInstanceEventListener;)V
37853785
public fun createSurface (Landroid/content/Context;Ljava/lang/String;Landroid/os/Bundle;)Lcom/facebook/react/interfaces/fabric/ReactSurface;

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/defaults/DefaultReactHost.kt

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ import com.facebook.react.bridge.ReactContext
1717
import com.facebook.react.common.annotations.UnstableReactNativeAPI
1818
import com.facebook.react.common.build.ReactBuildConfig
1919
import com.facebook.react.fabric.ComponentFactory
20-
import com.facebook.react.interfaces.exceptionmanager.ReactJsExceptionHandler
2120
import com.facebook.react.runtime.JSCInstance
2221
import com.facebook.react.runtime.ReactHostImpl
2322
import com.facebook.react.runtime.cxxreactpackage.CxxReactPackage
@@ -74,8 +73,6 @@ public object DefaultReactHost {
7473
reactPackages = packageList,
7574
jsRuntimeFactory = jsRuntimeFactory,
7675
turboModuleManagerDelegateBuilder = defaultTmmDelegateBuilder)
77-
// TODO: T180971255 Improve default exception handler
78-
val reactJsExceptionHandler = ReactJsExceptionHandler { _ -> }
7976
val componentFactory = ComponentFactory()
8077
DefaultComponentsRegistry.register(componentFactory)
8178
// TODO: T164788699 find alternative of accessing ReactHostImpl for initialising reactHost
@@ -85,7 +82,6 @@ public object DefaultReactHost {
8582
defaultReactHostDelegate,
8683
componentFactory,
8784
true /* allowPackagerServerAccess */,
88-
reactJsExceptionHandler,
8985
useDevSupport,
9086
)
9187
.apply {

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostImpl.java

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,6 @@
5757
import com.facebook.react.fabric.ComponentFactory;
5858
import com.facebook.react.fabric.FabricUIManager;
5959
import com.facebook.react.interfaces.TaskInterface;
60-
import com.facebook.react.interfaces.exceptionmanager.ReactJsExceptionHandler;
6160
import com.facebook.react.interfaces.fabric.ReactSurface;
6261
import com.facebook.react.internal.featureflags.ReactNativeFeatureFlags;
6362
import com.facebook.react.modules.appearance.AppearanceModule;
@@ -102,7 +101,6 @@ public class ReactHostImpl implements ReactHost {
102101
private final Context mContext;
103102
private final ReactHostDelegate mReactHostDelegate;
104103
private final ComponentFactory mComponentFactory;
105-
private final ReactJsExceptionHandler mReactJsExceptionHandler;
106104
private final DevSupportManager mDevSupportManager;
107105
private final Executor mBGExecutor;
108106
private final Executor mUIExecutor;
@@ -143,15 +141,13 @@ public ReactHostImpl(
143141
ReactHostDelegate delegate,
144142
ComponentFactory componentFactory,
145143
boolean allowPackagerServerAccess,
146-
ReactJsExceptionHandler reactJsExceptionHandler,
147144
boolean useDevSupport) {
148145
this(
149146
context,
150147
delegate,
151148
componentFactory,
152149
Executors.newSingleThreadExecutor(),
153150
Task.UI_THREAD_EXECUTOR,
154-
reactJsExceptionHandler,
155151
allowPackagerServerAccess,
156152
useDevSupport);
157153
}
@@ -162,15 +158,13 @@ public ReactHostImpl(
162158
ComponentFactory componentFactory,
163159
Executor bgExecutor,
164160
Executor uiExecutor,
165-
ReactJsExceptionHandler reactJsExceptionHandler,
166161
boolean allowPackagerServerAccess,
167162
boolean useDevSupport) {
168163
mContext = context;
169164
mReactHostDelegate = delegate;
170165
mComponentFactory = componentFactory;
171166
mBGExecutor = bgExecutor;
172167
mUIExecutor = uiExecutor;
173-
mReactJsExceptionHandler = reactJsExceptionHandler;
174168
mQueueThreadExceptionHandler = ReactHostImpl.this::handleHostException;
175169
mMemoryPressureRouter = new MemoryPressureRouter(context);
176170
mAllowPackagerServerAccess = allowPackagerServerAccess;
@@ -1067,7 +1061,6 @@ private Task<ReactInstance> getOrCreateReactInstanceTask() {
10671061
mComponentFactory,
10681062
devSupportManager,
10691063
mQueueThreadExceptionHandler,
1070-
mReactJsExceptionHandler,
10711064
mUseDevSupport,
10721065
getOrCreateReactHostInspectorTarget());
10731066

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactInstance.java

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,16 @@
77

88
package com.facebook.react.runtime;
99

10+
import static com.facebook.react.util.JSStackTrace.COLUMN_KEY;
11+
import static com.facebook.react.util.JSStackTrace.FILE_KEY;
12+
import static com.facebook.react.util.JSStackTrace.LINE_NUMBER_KEY;
13+
import static com.facebook.react.util.JSStackTrace.METHOD_NAME_KEY;
14+
1015
import android.content.res.AssetManager;
1116
import android.view.View;
1217
import com.facebook.common.logging.FLog;
18+
import com.facebook.fbreact.specs.NativeExceptionsManagerSpec;
19+
import com.facebook.infer.annotation.Assertions;
1320
import com.facebook.infer.annotation.Nullsafe;
1421
import com.facebook.infer.annotation.ThreadConfined;
1522
import com.facebook.infer.annotation.ThreadSafe;
@@ -21,12 +28,15 @@
2128
import com.facebook.react.bridge.Arguments;
2229
import com.facebook.react.bridge.JSBundleLoader;
2330
import com.facebook.react.bridge.JSBundleLoaderDelegate;
31+
import com.facebook.react.bridge.JavaOnlyArray;
32+
import com.facebook.react.bridge.JavaOnlyMap;
2433
import com.facebook.react.bridge.JavaScriptContextHolder;
2534
import com.facebook.react.bridge.NativeArray;
2635
import com.facebook.react.bridge.NativeMap;
2736
import com.facebook.react.bridge.NativeModule;
2837
import com.facebook.react.bridge.ReactNoCrashSoftException;
2938
import com.facebook.react.bridge.ReactSoftExceptionLogger;
39+
import com.facebook.react.bridge.ReadableMap;
3040
import com.facebook.react.bridge.RuntimeExecutor;
3141
import com.facebook.react.bridge.RuntimeScheduler;
3242
import com.facebook.react.bridge.queue.MessageQueueThread;
@@ -110,7 +120,6 @@ final class ReactInstance {
110120
ComponentFactory componentFactory,
111121
DevSupportManager devSupportManager,
112122
QueueThreadExceptionHandler exceptionHandler,
113-
ReactJsExceptionHandler reactExceptionManager,
114123
boolean useDevSupport,
115124
@Nullable ReactHostInspectorTarget reactHostInspectorTarget) {
116125
mBridgelessReactContext = bridgelessReactContext;
@@ -154,14 +163,15 @@ final class ReactInstance {
154163
// Notify JS if profiling is enabled
155164
boolean isProfiling =
156165
Systrace.isTracing(Systrace.TRACE_TAG_REACT_APPS | Systrace.TRACE_TAG_REACT_JS_VM_CALLS);
166+
157167
mHybridData =
158168
initHybrid(
159169
jsRuntimeFactory,
160170
jsMessageQueueThread,
161171
nativeModulesMessageQueueThread,
162172
mJavaTimerManager,
163173
jsTimerExecutor,
164-
reactExceptionManager,
174+
new ReactJsExceptionHandlerImpl(nativeModulesMessageQueueThread),
165175
bindingsInstaller,
166176
isProfiling,
167177
reactHostInspectorTarget);
@@ -311,6 +321,44 @@ public ReactQueueConfiguration getReactQueueConfiguration() {
311321
return mQueueConfiguration;
312322
}
313323

324+
private class ReactJsExceptionHandlerImpl implements ReactJsExceptionHandler {
325+
private final MessageQueueThread mNativemodulesmessagequeuethread;
326+
327+
ReactJsExceptionHandlerImpl(MessageQueueThread nativeModulesMessageQueueThread) {
328+
this.mNativemodulesmessagequeuethread = nativeModulesMessageQueueThread;
329+
}
330+
331+
@Override
332+
public void reportJsException(ParsedError error) {
333+
List<ReactJsExceptionHandler.ParsedError.StackFrame> frames = error.getFrames();
334+
List<ReadableMap> readableMapList = new ArrayList<>();
335+
for (ReactJsExceptionHandler.ParsedError.StackFrame frame : frames) {
336+
JavaOnlyMap map = new JavaOnlyMap();
337+
map.putDouble(COLUMN_KEY, frame.getColumnNumber());
338+
map.putDouble(LINE_NUMBER_KEY, frame.getLineNumber());
339+
map.putString(FILE_KEY, (String) frame.getFileName());
340+
map.putString(METHOD_NAME_KEY, (String) frame.getMethodName());
341+
readableMapList.add(map);
342+
}
343+
344+
JavaOnlyMap data = new JavaOnlyMap();
345+
data.putString("message", error.getMessage());
346+
data.putArray("stack", JavaOnlyArray.from(readableMapList));
347+
data.putInt("id", error.getExceptionId());
348+
data.putBoolean("isFatal", error.isFatal());
349+
350+
// Simulate async native module method call
351+
mNativemodulesmessagequeuethread.runOnQueue(
352+
() -> {
353+
NativeExceptionsManagerSpec exceptionsManager =
354+
(NativeExceptionsManagerSpec)
355+
Assertions.assertNotNull(
356+
mTurboModuleManager.getModule(NativeExceptionsManagerSpec.NAME));
357+
exceptionsManager.reportException(data);
358+
});
359+
}
360+
}
361+
314362
public void loadJSBundle(JSBundleLoader bundleLoader) {
315363
// Load the JS bundle
316364
Systrace.beginSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, "ReactInstance.loadJSBundle");

packages/react-native/ReactAndroid/src/test/java/com/facebook/react/runtime/ReactHostTest.kt

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -84,12 +84,7 @@ class ReactHostTest {
8484
Mockito.doReturn(jSBundleLoader).`when`(reactHostDelegate).jsBundleLoader
8585
reactHost =
8686
ReactHostImpl(
87-
activityController.get().application,
88-
reactHostDelegate,
89-
componentFactory,
90-
false,
91-
{},
92-
false)
87+
activityController.get().application, reactHostDelegate, componentFactory, false, false)
9388
val taskCompletionSource = TaskCompletionSource<Boolean>().apply { setResult(true) }
9489
mockedTaskCompletionSourceCtor =
9590
Mockito.mockConstruction(

0 commit comments

Comments
 (0)