add default early JS error handler#44884
Closed
alanleedev wants to merge 1 commit intofacebook:mainfrom
Closed
Conversation
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D58385767 |
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D58385767 |
039fb64 to
0e6180d
Compare
Base commit: b19bf2b |
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D58385767 |
0e6180d to
a336c51
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D58385767 |
a336c51 to
7b5bb8b
Compare
Summary: Pull Request resolved: facebook#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
alanleedev
added a commit
to alanleedev/react-native
that referenced
this pull request
Jun 13, 2024
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
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D58385767 |
7b5bb8b to
c365360
Compare
Contributor
|
This pull request has been merged in fe7e7a0. |
|
This pull request was successfully merged by @alanleedev in fe7e7a0. When will my fix make it into a release? | How to file a pick request? |
cortinico
pushed a commit
that referenced
this pull request
Jul 1, 2024
Summary: Pull Request resolved: #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 Reviewed By: javache, cortinico Differential Revision: D58385767 fbshipit-source-id: 46548677df936b7c2f584084a2c9769c27e6a963
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
Removing JavaScript error handler supplied to ReactHostImpl.java which is just a stub and creating a default handler in ReactInstance.java.
Changelog: [Internal]
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/catchstatements to catch errors but in Android the error handler callback is implemented to be passed in while instantiatingReactHostImplwhere 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 intoReactHostImpl.Updated
DefaultReactJsExceptionHandlerdefined inReactInstance.javais instantiated and passed into JNI/C++ viainitHybrid()call.ReactJsExceptionHandler.reportJsException()method is removed fromFbReactExceptionManageras 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 Errorbefore error handling code is run during JavaScript bundle setup. It also includes Android log to check the method was called.jf get --version 229685487Testing was done using
fb4aandrntester-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 whenReactInstance.loadJSScript()throws and is not covered in this diff.Differential Revision: D58385767