Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add default early JS error handler #44884

Closed
wants to merge 1 commit into from

Conversation

alanleedev
Copy link
Contributor

Summary:
Removing JavaScript error handler supplied to ReactHostImpl.java which is just a stub and creating a default handler in ReactInstance.java.

Changelog: [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

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner labels Jun 11, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D58385767

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D58385767

alanleedev added a commit to alanleedev/react-native that referenced this pull request Jun 11, 2024
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.

Changelog: [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
@analysis-bot
Copy link

analysis-bot commented Jun 11, 2024

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 20,364,346 -2
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 23,569,560 -14
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: b19bf2b
Branch: main

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. 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 handler in ReactInstance.java which uses NativeExceptionHandler TurboModule to handle error.

Changelog: [Android][BREAKING]

Differential Revision: D58385767
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. 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 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
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
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D58385767

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in fe7e7a0.

Copy link

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants