facebook
Wrap NullPointerExceptions when thrown and throw new Error in dispatchViewManagerCommand for readability
Summary:
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 #23126, #19413, #27633, #23378, #29250, #28262, #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
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
| Platform | Engine | Arch | Size (bytes) | Diff |
|---|---|---|---|---|
| android | hermes | arm64-v8a | 9,001,193 | -44,034 |
| android | hermes | armeabi-v7a | 8,261,776 | -32,723 |
| android | hermes | x86 | 9,514,587 | -46,833 |
| android | hermes | x86_64 | 9,357,405 | -46,348 |
| android | jsc | arm64-v8a | 9,560,440 | -43,979 |
| android | jsc | armeabi-v7a | 8,698,321 | -32,813 |
| android | jsc | x86 | 9,644,705 | -46,760 |
| android | jsc | x86_64 | 9,891,565 | -46,243 |
Base commit: c803a5bfa12a4305daa846c94e8112a7661a8dfc Branch: main
@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
Please hold off on merging this! I accidentally wrapped the wrong spot. I'll send a follow-up
@cortinico We've tested this in production for about 1.5 weeks now and the errors thrown are much better. This should be good to merge!
@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@cortinico merged this pull request in facebook/react-native@0519c11acd0c347db378bbc9238c7dabfd38f6fa.
Sorry, having to revert this as we have concerns about this leaking user information to logcat. Consider making these extra logs available in development builds only so they're not shipped in release.
This pull request has been reverted by 52698e758d1454afc74541e5507ac16dd6154386.
@hsource don't you mind sending the same PR without printing parameters to the log?
