From ff1972dabafbbfc18203464b452e0d5b796cdcf6 Mon Sep 17 00:00:00 2001 From: Harry Yu Date: Tue, 18 Jul 2023 10:37:57 -0700 Subject: [PATCH] Wrap NullPointerExceptions and throw new Error in dispatchViewManagerCommand for easier debugging (#38444) Summary: This PR is a modified version of https://github.com/facebook/react-native/issues/38060 which was reverted because it may sometimes cause sensitive info to be logged. This version no longer includes any parameters in the thrown errors. ### Motivation I had a crash-causing error in our error reporting console (Bugsnag) that was extremely hard to debug due to the lack of specificity in the thrown error. ``` java.lang.NullPointerException: Attempt to invoke virtual method 'double java.lang.Double.doubleValue()' on a null object reference at com.facebook.react.bridge.ReadableNativeArray.getDouble(ReadableNativeArray.java:92) at com.facebook.react.bridge.JavaMethodWrapper$4.extractArgument(JavaMethodWrapper.java:64) at com.facebook.react.bridge.JavaMethodWrapper$4.extractArgument(JavaMethodWrapper.java:60) at com.facebook.react.bridge.JavaMethodWrapper.invoke(JavaMethodWrapper.java:356) at com.facebook.react.bridge.JavaModuleWrapper.invoke(JavaModuleWrapper.java:188) at com.facebook.jni.NativeRunnable.run(NativeRunnable.java:-2) at android.os.Handler.handleCallback(Handler.java:883) at android.os.Handler.dispatchMessage(Handler.java:100) at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:27) at android.os.Looper.loop(Looper.java:214) at com.facebook.react.bridge.queue.MessageQueueThreadImpl$4.run(MessageQueueThreadImpl.java:228) at java.lang.Thread.run(Thread.java:919) ``` I noticed that `invoke` in `JavaMethodWrapper` tacks on the JS method name on other errors, so wanted to change this to handle NullPointerExceptions more gracefully too. This helps make it easier to debug issues like https://github.com/facebook/react-native/issues/23126, https://github.com/facebook/react-native/issues/19413, https://github.com/facebook/react-native/issues/27633, https://github.com/facebook/react-native/issues/23378, https://github.com/facebook/react-native/issues/29250, https://github.com/facebook/react-native/issues/28262, https://github.com/facebook/react-native/issues/34001 and likely many more. ### After adding NullPointerException Even after adding the NullPointerException, I got errors like this: ``` com.facebook.react.bridge.NativeArgumentsParseException: Attempt to invoke virtual method 'double java.lang.Double.doubleValue()' on a null object reference (constructing arguments for UIManager.dispatchViewManagerCommand at argument index 0) with parameters [null, 4.0, []] at com.facebook.react.bridge.JavaMethodWrapper.invoke(JavaMethodWrapper.java:371) at com.facebook.react.bridge.JavaModuleWrapper.invoke(JavaModuleWrapper.java:188) at com.facebook.jni.NativeRunnable.run(NativeRunnable.java:-2) at android.os.Handler.handleCallback(Handler.java:942) at android.os.Handler.dispatchMessage(Handler.java:99) at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:27) at android.os.Looper.loopOnce(Looper.java:226) at android.os.Looper.loop(Looper.java:313) at com.facebook.react.bridge.queue.MessageQueueThreadImpl$4.run(MessageQueueThreadImpl.java:228) at java.lang.Thread.run(Thread.java:1012) ``` This helped, but still didn't help me easily find which library calling `dispatchViewManagerCommand` was causing this. ## Changelog: [ANDROID] [CHANGED] - Wrap NullPointerExceptions when thrown by native code called from JS for readability [GENERAL] [CHANGED] - Throw Error in dispatchViewManagerCommand when non-numeric tag is passed for easier debugging Pull Request resolved: https://github.com/facebook/react-native/pull/38444 Test Plan: Test change on our app via a beta release. With these changes, we got better stacktraces like these: ### Java stacktrace ``` com.facebook.react.bridge.NativeArgumentsParseException: Attempt to invoke virtual method 'double java.lang.Double.doubleValue()' on a null object reference (constructing arguments for UIManager.dispatchViewManagerCommand at argument index 0) with parameters [null, 4.0, []] at com.facebook.react.bridge.JavaMethodWrapper.invoke(JavaMethodWrapper.java:371) at com.facebook.react.bridge.JavaModuleWrapper.invoke(JavaModuleWrapper.java:188) at com.facebook.jni.NativeRunnable.run(NativeRunnable.java:-2) at android.os.Handler.handleCallback(Handler.java:942) at android.os.Handler.dispatchMessage(Handler.java:99) at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:27) at android.os.Looper.loopOnce(Looper.java:226) at android.os.Looper.loop(Looper.java:313) at com.facebook.react.bridge.queue.MessageQueueThreadImpl$4.run(MessageQueueThreadImpl.java:228) at java.lang.Thread.run(Thread.java:1012) ``` ### JS stacktrace (after dispatchViewManagerCommand change) ``` Error dispatchViewManagerCommand: found null reactTag with args [] node_modules/react-native/Libraries/ReactNative/PaperUIManager.js:120:21 dispatchViewManagerCommand node_modules/react-native-maps/lib/MapMarker.js:68:67 _runCommand node_modules/react-native-maps/lib/MapMarker.js:60:24 redraw templates/Components/MapViewWithMarkers/PlaceMarker.tsx:88:32 anonymous (native) apply node_modules/react-native/Libraries/Core/Timers/JSTimers.js:213:22 anonymous node_modules/react-native/Libraries/Core/Timers/JSTimers.js:111:14 _callTimer node_modules/react-native/Libraries/Core/Timers/JSTimers.js:359:16 callTimers (native) apply node_modules/react-native/Libraries/BatchedBridge/MessageQueue.js:427:31 __callFunction node_modules/react-native/Libraries/BatchedBridge/MessageQueue.js:113:25 anonymous node_modules/react-native/Libraries/BatchedBridge/MessageQueue.js:368:10 __guard node_modules/react-native/Libraries/BatchedBridge/MessageQueue.js:112:16 callFunctionReturnFlushedQueue ``` Reviewed By: javache Differential Revision: D47546672 Pulled By: cortinico fbshipit-source-id: 68379561d84d0ef2ed5c6d66f3f49943c5d7cb7e --- .../Libraries/ReactNative/UIManager.js | 8 ++++++++ .../react/bridge/JavaMethodWrapper.java | 18 ++++++++++++------ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/packages/react-native/Libraries/ReactNative/UIManager.js b/packages/react-native/Libraries/ReactNative/UIManager.js index e830b96971891a..98360a14bb23c5 100644 --- a/packages/react-native/Libraries/ReactNative/UIManager.js +++ b/packages/react-native/Libraries/ReactNative/UIManager.js @@ -180,6 +180,14 @@ const UIManager = { commandName: number | string, commandArgs: any[], ) { + // Sometimes, libraries directly pass in the output of `findNodeHandle` to + // this function without checking if it's null. This guards against that + // case. We throw early here in Javascript so we can get a JS stacktrace + // instead of a harder-to-debug native Java or Objective-C stacktrace. + if (typeof reactTag !== 'number') { + throw new Error('dispatchViewManagerCommand: found null reactTag'); + } + if (isFabricReactTag(reactTag)) { const FabricUIManager = nullthrows(getFabricUIManager()); const shadowNode = diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaMethodWrapper.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaMethodWrapper.java index 8a2ef7bb9fb6d2..d6fb0238caaa06 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaMethodWrapper.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaMethodWrapper.java @@ -356,7 +356,7 @@ public void invoke(JSInstance jsInstance, ReadableArray parameters) { mArgumentExtractors[i].extractArgument(jsInstance, parameters, jsArgumentsConsumed); jsArgumentsConsumed += mArgumentExtractors[i].getJSArgumentsNeeded(); } - } catch (UnexpectedNativeTypeException e) { + } catch (UnexpectedNativeTypeException | NullPointerException e) { throw new NativeArgumentsParseException( e.getMessage() + " (constructing arguments for " @@ -370,23 +370,29 @@ public void invoke(JSInstance jsInstance, ReadableArray parameters) { try { mMethod.invoke(mModuleWrapper.getModule(), mArguments); - } catch (IllegalArgumentException ie) { - throw new RuntimeException("Could not invoke " + traceName, ie); - } catch (IllegalAccessException iae) { - throw new RuntimeException("Could not invoke " + traceName, iae); + } catch (IllegalArgumentException | IllegalAccessException e) { + throw new RuntimeException(createInvokeExceptionMessage(traceName), e); } catch (InvocationTargetException ite) { // Exceptions thrown from native module calls end up wrapped in InvocationTargetException // which just make traces harder to read and bump out useful information if (ite.getCause() instanceof RuntimeException) { throw (RuntimeException) ite.getCause(); } - throw new RuntimeException("Could not invoke " + traceName, ite); + throw new RuntimeException(createInvokeExceptionMessage(traceName), ite); } } finally { SystraceMessage.endSection(TRACE_TAG_REACT_JAVA_BRIDGE).flush(); } } + /** + * Makes it easier to determine the cause of an error invoking a native method from Javascript + * code by adding the function name. + */ + private static String createInvokeExceptionMessage(String traceName) { + return "Could not invoke " + traceName; + } + /** * Determines how the method is exported in JavaScript: METHOD_TYPE_ASYNC for regular methods * METHOD_TYPE_PROMISE for methods that return a promise object to the caller. METHOD_TYPE_SYNC