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

Load jsc or hermes lib in static method #30749

Closed
wants to merge 5 commits into from
Closed

Conversation

iqqmuT
Copy link
Contributor

@iqqmuT iqqmuT commented Jan 16, 2021

Summary

Many have reported about the misguiding error Fatal Exception: java.lang.UnsatisfiedLinkError: couldn't find DSO to load: libhermes.so even though they don't use Hermes (for example issues #26075 #25923).

The current code does not handle errors correctly when loading JSC or Hermes in ReactInstanceManagerBuilder.

ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerBuilder.java:

try {
  return new HermesExecutorFactory();
} catch (UnsatisfiedLinkError hermesE) {
  // We never get here because "new HermesExecutorFactory()" does not throw an exception!
  hermesE.printStackTrace();
  throw jscE;
}

In Java, when an exception is thrown in static block, it will be RuntimeException and it can't be caught. For example the exception from SoLoader.loadLibrary can't be caught and it will crash the app.

ReactAndroid/src/main/java/com/facebook/hermes/reactexecutor/HermesExecutor.java:

static {
  // Exception from this code block will be RuntimeException and it can't be caught!
  SoLoader.loadLibrary("hermes");
  try {
    SoLoader.loadLibrary("hermes-executor-debug");
    mode_ = "Debug";
  } catch (UnsatisfiedLinkError e) {
    SoLoader.loadLibrary("hermes-executor-release");
    mode_ = "Release";
  }
}

This PR fixes the code so that the original exception from failed JSC loading is not swallowed. It does not fix the original issue why JSC loading is failing with some devices, but it can be really helpful to know what the real error is. For example Firebase Crashlytics shows wrong stack trace with current code.

I'm sure that this fix could have been written better. It feels wrong to import JSCExecutor and HermesExecutor in ReactInstanceManagerBuilder.java. However, the main point of this PR is to give the idea what is wrong with the current code.

Changelog

[Android] [Fixed] - Fix error handling when loading JSC or Hermes

Test Plan

  • from this PR, modify ReactAndroid/src/main/java/com/facebook/react/jscexecutor/JSCExecutor.java so that JSC loading will fail:
// original
SoLoader.loadLibrary("jscexecutor");
// changed
SoLoader.loadLibrary("jscexecutor-does-not-exist");
  • Run rn-tester app
  • Check from Logcat that the app crashed with correct exception and stacktrace. It should not be java.lang.UnsatisfiedLinkError: couldn't find DSO to load: libhermes.so

@facebook-github-bot
Copy link
Contributor

Hi @iqqmuT!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code analysis results:

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 16, 2021
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@analysis-bot
Copy link

analysis-bot commented Jan 16, 2021

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 65975dd

@analysis-bot
Copy link

analysis-bot commented Jan 16, 2021

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,898,636 -932
android hermes armeabi-v7a 8,402,704 -227
android hermes x86 9,385,048 -777
android hermes x86_64 9,330,482 -831
android jsc arm64-v8a 10,350,483 -946
android jsc armeabi-v7a 9,837,655 -241
android jsc x86 10,398,399 -797
android jsc x86_64 10,984,112 -813

Base commit: 65975dd

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

iqqmuT referenced this pull request Jan 19, 2021
Summary:
See the comments for more info.

Changelog: [Android] [Changed] - Improve exception message when JSC loading fails

Reviewed By: tmikov

Differential Revision: D19917034

fbshipit-source-id: d846f542c31e9c94edcee240c2935d77d48d1f2a
@sota000 sota000 self-assigned this Aug 16, 2021
@facebook-github-bot
Copy link
Contributor

@sota000 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@sota000 sota000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @iqqmuT, apologies for taking a long time to get back to you. I've left a comment about removing the "loaded" check. Could you please check it out when you have a chance? Thank you!

@pull-bot
Copy link

pull-bot commented Sep 3, 2021

Fails
🚫

❗ Base Branch - The base branch for this PR is something other than master. Are you sure you want to target something other than the master branch?

Generated by 🚫 dangerJS against a394c7c

Copy link
Contributor

@sota000 sota000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @iqqmuT! Thanks for addressing the comment! I assume you have tested that it works without the check?

@facebook-github-bot
Copy link
Contributor

@sota000 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@iqqmuT
Copy link
Contributor Author

iqqmuT commented Sep 7, 2021

Yes I tested it and it worked perfectly.

@sota000
Copy link
Contributor

sota000 commented Sep 13, 2021

@iqqmuT, the CI seems to fail consistently with this change. Would you ming taking a look if they are relevant to your changes? (I can help fix the lint one)

@iqqmuT
Copy link
Contributor Author

iqqmuT commented Sep 13, 2021

@sota000 I couldn't check Facebook internal tests but ci/circleci test failures don't seem to be relevant to my changes.

@RodolfoGS
Copy link
Contributor

@iqqmuT I think that you must to add a new empty line between public class JSCExecutor extends JavaScriptExecutor and static { in JSCExecutor.java.

Like this:

/* package */ public class JSCExecutor extends JavaScriptExecutor {

  static {
    ...

@RodolfoGS
Copy link
Contributor

RodolfoGS commented Sep 16, 2021

@sota000 seems that the tests failed for a reason not related to this PR. shellcheck can't be installed and the test fail.

This is the output of the tests:

Install additional GitHub bot dependencies

#!/bin/bash -eo pipefail
apt update && apt install -y shellcheck jq

....

W: GPG error: https://dl.yarnpkg.com/debian stable InRelease: The following signatures were invalid: EXPKEYSIG 23E7166788B63E1E Yarn Packaging <yarn@dan.cx>
E: The repository 'https://dl.yarnpkg.com/debian stable InRelease' is not signed.
N: Updating from such a repository can't be done securely, and is therefore disabled by default.
N: See apt-secure(8) manpage for repository creation and user configuration details.

And then in the next test:

Run linters against modified files (analysis-bot)

yarn run v1.22.5
$ ./scripts/circleci/analyze_code.sh && yarn shellcheck

....

$ ./scripts/circleci/analyze_scripts.sh
shellcheck is not installed. See https://github.com/facebook/react-native/wiki/Development-Dependencies#shellcheck for instructions.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@facebook-github-bot
Copy link
Contributor

@sota000 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@sota000 merged this pull request in d839b24.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Oct 21, 2021
lunaleaps pushed a commit that referenced this pull request Oct 25, 2021
Summary:
Many have reported about the misguiding error `Fatal Exception: java.lang.UnsatisfiedLinkError: couldn't find DSO to load: libhermes.so` even though they don't use Hermes (for example issues #26075 #25923).

**The current code does not handle errors correctly when loading JSC or Hermes in `ReactInstanceManagerBuilder`**.

**ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerBuilder.java:**
```java
try {
  return new HermesExecutorFactory();
} catch (UnsatisfiedLinkError hermesE) {
  // We never get here because "new HermesExecutorFactory()" does not throw an exception!
  hermesE.printStackTrace();
  throw jscE;
}
```

In Java, when an exception is thrown in static block, it will be RuntimeException and it can't be caught. For example the exception from `SoLoader.loadLibrary` can't be caught and it will crash the app.

**ReactAndroid/src/main/java/com/facebook/hermes/reactexecutor/HermesExecutor.java:**
```java
static {
  // Exception from this code block will be RuntimeException and it can't be caught!
  SoLoader.loadLibrary("hermes");
  try {
    SoLoader.loadLibrary("hermes-executor-debug");
    mode_ = "Debug";
  } catch (UnsatisfiedLinkError e) {
    SoLoader.loadLibrary("hermes-executor-release");
    mode_ = "Release";
  }
}
```

This PR fixes the code so that the original exception from failed JSC loading is not swallowed. It does not fix the original issue why JSC loading is failing with some devices, but it can be really helpful to know what the real error is. For example Firebase Crashlytics shows wrong stack trace with current code.

I'm sure that this fix could have been written better. It feels wrong to import `JSCExecutor` and `HermesExecutor` in `ReactInstanceManagerBuilder.java`. However, the main point of this PR is to give the idea what is wrong with the current code.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[Android] [Fixed] - Fix error handling when loading JSC or Hermes

Pull Request resolved: #30749

Test Plan:
* from this PR, modify  `ReactAndroid/src/main/java/com/facebook/react/jscexecutor/JSCExecutor.java` so that JSC loading will fail:
```java
// original
SoLoader.loadLibrary("jscexecutor");
// changed
SoLoader.loadLibrary("jscexecutor-does-not-exist");
```
* Run `rn-tester` app
* Check from Logcat that the app crashed with correct exception and stacktrace. It should **not** be `java.lang.UnsatisfiedLinkError: couldn't find DSO to load: libhermes.so`

Tested with Hermes

```
    SoLoader.loadLibrary("hermes-executor-test");
```
Got this one in logcat
```
09-24 20:12:39.552  6412  6455 E AndroidRuntime: java.lang.UnsatisfiedLinkError: couldn't find DSO to load: libhermes-executor-test.so
```

Reviewed By: cortinico

Differential Revision: D30346032

Pulled By: sota000

fbshipit-source-id: 09b032a9e471af233b7ac90b571c311952ab6342
Kudo added a commit to expo/expo that referenced this pull request Jan 28, 2022
…16099)

# Why

fix crash when `enableHermes: true` with dev-menu on react-native 0.67. i tested it from bare-expo.
stacktrace:

```
FATAL EXCEPTION: main
Process: dev.expo.payments, PID: 18344
java.lang.NoClassDefFoundError: com.facebook.react.jscexecutor.JSCExecutor
    at com.facebook.react.jscexecutor.JSCExecutor.loadLibrary(JSCExecutor.java:24)
    at com.facebook.react.ReactInstanceManagerBuilder.getDefaultJSExecutorFactory(ReactInstanceManagerBuilder.java:352)
    at com.facebook.react.ReactInstanceManagerBuilder.build(ReactInstanceManagerBuilder.java:319)
    at com.facebook.react.ReactNativeHost.createReactInstanceManager(ReactNativeHost.java:95)
    at expo.modules.devmenu.DevMenuHost.createReactInstanceManager(DevMenuHost.kt:41)
    at com.facebook.react.ReactNativeHost.getReactInstanceManager(ReactNativeHost.java:42)
    at expo.modules.devmenu.DevMenuManager.maybeInitDevMenuHost$lambda-8(DevMenuManager.kt:169)
    at expo.modules.devmenu.DevMenuManager.lambda$rZDvUiNHHHvkcRmdKO265huCv6U(Unknown Source:0)
    at expo.modules.devmenu.-$$Lambda$DevMenuManager$rZDvUiNHHHvkcRmdKO265huCv6U.run(Unknown Source:0)
    at android.os.Handler.handleCallback(Handler.java:938)
    at android.os.Handler.dispatchMessage(Handler.java:99)
    at android.os.Looper.loop(Looper.java:223)
    at android.app.ActivityThread.main(ActivityThread.java:7656)
    at java.lang.reflect.Method.invoke(Native Method)
    at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592)
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947)
Caused by: java.lang.UnsatisfiedLinkError: couldn't find DSO to load: libjscexecutor.so
    SoSource 0: com.facebook.soloader.ApkSoSource[root = /data/data/dev.expo.payments/lib-main flags = 1]
    SoSource 1: com.facebook.soloader.DirectorySoSource[root = /data/app/~~ibMyBupkSRJXdt_c772rhw==/dev.expo.payments-ru7gzQ9g2MilqIhz24Lduw==/lib/arm64 flags = 0]
    SoSource 2: com.facebook.soloader.DirectorySoSource[root = /vendor/lib64 flags = 2]
    SoSource 3: com.facebook.soloader.DirectorySoSource[root = /system/lib64 flags = 2]
    Native lib dir: /data/app/~~ibMyBupkSRJXdt_c772rhw==/dev.expo.payments-ru7gzQ9g2MilqIhz24Lduw==/lib/arm64
```

the first time to create a `ReactInstanceManager`, it successes. however, the second time to create a `ReactInstanceManager` for dev-menu, it crashes with `NoClassDefFoundError`. it's also uncaughtable [here](https://github.com/facebook/react-native/blob/20b9ed91c02a0cbf2f970bf51ccc4c278c63540c/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerBuilder.java#L348-L354) 

this is because the change landed in react-native 0.67: facebook/react-native#30749

according to jvm spec v2 2.17.5, ([maybe from this spec](https://docs.oracle.com/javase/specs/jvms/se6/html/Concepts.doc.html#24237)). the second time to load a failure class, the vm throws `NoClassDefFoundError`. [there's another reference in android art vm implementation](https://android.googlesource.com/platform/art/+/ec696e5d98ae4c503966f199bac341b50bfa1a5b/runtime/class_linker.cc#478)

# How

for dev-menu, we try to locate `libjsc.so` and create specific javascript executor.

# Test Plan

tested on my react-native 0.67 upgrade branch: #16038
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants