Added additional builder method receiving arguments for using jsc or hermes to correctly decide which DSO to load at app startup. #33123

KunalFarmah98 · 2022-02-16T14:18:22Z

Summary

The current implementation of getDefaultJSExecutorFactory relies solely on try catch to load the correct .so file for jsc or hermes based on the project configuration.
Relying solely on try catch block and loading jsc even when project is using hermes can lead to launch time crashes especially in monorepo architectures and hybrid apps using both native android and react native.
So we can make use of an additional ReactInstanceManager :: builder method that accepts an argument while building ReactInstanceManager which can tell which library to load at startup in ReactInstanceManagerBuilder which will now have an enhanced getDefaultJSExecutorFactory method that will combine the old logic with the new one to load the dso files.
The code snippet in ReactInstanceManager for adding a new builder method:

  /** Creates a builder that is capable of creating an instance of {@link ReactInstanceManager}. */
  public static ReactInstanceManagerBuilder builder() {
    return new ReactInstanceManagerBuilder();
  }

  /** Creates a builder that is capable of creating an instance of {@link ReactInstanceManager}.
   with argument determining which one of hermes or jsc to use */
  public static ReactInstanceManagerBuilder builder(Boolean hermesEnabled) {
    return new ReactInstanceManagerBuilder(hermesEnabled);
  }

The code snippet for the new logic in ReactInstanceManagerBuilder:

Setting up the new logic:

private String jscOrHermes = "";

  /* package protected */ ReactInstanceManagerBuilder() {}

  /* package protected (can switch between hermes {hermesEnabled: true} and jsc {hermesEnabled: false}) */
  ReactInstanceManagerBuilder(Boolean hermesEnabled) {
    // if the builder is called with arguments, set the current js engine based on the value
    if(hermesEnabled){
      jscOrHermes = "hermes";
    }
    else
      jscOrHermes = "jsc";
  }

Modifying the getDefaultJSExecutorFactory method to incorporate the new logic with the old one:

 private JavaScriptExecutorFactory getDefaultJSExecutorFactory(
      String appName, String deviceName, Context applicationContext) {
    // if the app uses old way of building ReactInstanceManager,
    // use the old buggy code

    // Relying solely on try catch block and loading jsc even when
    // project is using hermes can lead to launchtime crashes expecially in
    // monorepo architectures and hybrid apps using both native android
    // and react native.
    // So we can use the value of enableHermes receved by the constructor
    // to decide which library to load at launch
    if(jscOrHermes.isEmpty()) {
      try {
        // If JSC is included, use it as normal
        initializeSoLoaderIfNecessary(applicationContext);
        JSCExecutor.loadLibrary();
        return new JSCExecutorFactory(appName, deviceName);
      } catch (UnsatisfiedLinkError jscE) {
        // https://github.com/facebook/hermes/issues/78 shows that
        // people who aren't trying to use Hermes are having issues.
        // https://github.com/facebook/react-native/issues/25923#issuecomment-554295179
        // includes the actual JSC error in at least one case.
        //
        // So, if "__cxa_bad_typeid" shows up in the jscE exception
        // message, then we will assume that's the failure and just
        // throw now.

        if (jscE.getMessage().contains("__cxa_bad_typeid")) {
          throw jscE;
        }

        // Otherwise use Hermes
        try {
          HermesExecutor.loadLibrary();
          return new HermesExecutorFactory();
        } catch (UnsatisfiedLinkError hermesE) {
          // If we get here, either this is a JSC build, and of course
          // Hermes failed (since it's not in the APK), or it's a Hermes
          // build, and Hermes had a problem.

          // We suspect this is a JSC issue (it's the default), so we
          // will throw that exception, but we will print hermesE first,
          // since it could be a Hermes issue and we don't want to
          // swallow that.
          hermesE.printStackTrace();
          throw jscE;
        }
      }
    }
    // if the app explicitly specifies which engine to use, directly load
    // that library
    else{
      try {
        initializeSoLoaderIfNecessary(applicationContext);
        if(jscOrHermes.equals("hermes")){
          HermesExecutor.loadLibrary();
          return new HermesExecutorFactory();
        }
        else{
          JSCExecutor.loadLibrary();
          return new JSCExecutorFactory(appName, deviceName);
        }
      } catch (UnsatisfiedLinkError e) {
        // linking library failed, so we print its stacktrace and
        // throw the exception
        e.printStackTrace();
        throw e;
      }
    }
  }

Suggested changes in any Android App's MainApplication that extends ReactApplication to take advantage of this fix

builder = ReactInstanceManager.builder(BuildConfig.HERMES_ENABLED)
                .setApplication(this)
                .setBundleAssetName("index.android.bundle")
                .setJSMainModulePath("index")

where HERMES_ENABLED is a buildConfigField based on the enableHermes flag in build.gradle:

def enableHermes = project.ext.react.get("enableHermes", true)
and then

defaultConfig{
if(enableHermes) {
            buildConfigField("boolean", "HERMES_ENABLED", "true")
        }
        else{
            buildConfigField("boolean", "HERMES_ENABLED", "false")
        }
}

Our app was facing a similar issue as listed in this list: https://github.com/facebook/react-native/issues?q=is%3Aissue+is%3Aopen+DSO. Which was react-native trying to load jsc even when our project used hermes when a debug build was deployed on a device using android studio play button.

This change can possibly solve many of the issues listed in the list as it solved ours.

Changelog

[GENERAL] [ADDED] - A second builder method in ReactInstanceManager.java:

  /** Creates a builder that is capable of creating an instance of {@link ReactInstanceManager}.
   with argument determining which one of hermes or jsc to use */
  public static ReactInstanceManagerBuilder builder(Boolean hermesEnabled) {
    return new ReactInstanceManagerBuilder(hermesEnabled);
  }

[GENERAL] [ADDED] - A string varaible and a parametrized constructor in ReactInstanceManagerBuilder.java:

private String jscOrHermes = "";

  /* package protected (can switch between hermes {hermesEnabled: true} and jsc {hermesEnabled: false}) */
  ReactInstanceManagerBuilder(Boolean hermesEnabled) {
    // if the builder is called with arguments, set the current js engine based on the value
    if(hermesEnabled){
      jscOrHermes = "hermes";
    }
    else
      jscOrHermes = "jsc";
  }

[GENERAL] [ADDED] - Modified getDefaultJSExecutorFactory method

private JavaScriptExecutorFactory getDefaultJSExecutorFactory(
      String appName, String deviceName, Context applicationContext) {
    // if the app uses old way of building ReactInstanceManager,
    // use the old buggy code

    // Relying solely on try catch block and loading jsc even when
    // project is using hermes can lead to launchtime crashes expecially in
    // monorepo architectures and hybrid apps using both native android
    // and react native.
    // So we can use the value of enableHermes receved by the constructor
    // to decide which library to load at launch
    if(jscOrHermes.isEmpty()) {
      try {
        // If JSC is included, use it as normal
        initializeSoLoaderIfNecessary(applicationContext);
        JSCExecutor.loadLibrary();
        return new JSCExecutorFactory(appName, deviceName);
      } catch (UnsatisfiedLinkError jscE) {
        // https://github.com/facebook/hermes/issues/78 shows that
        // people who aren't trying to use Hermes are having issues.
        // https://github.com/facebook/react-native/issues/25923#issuecomment-554295179
        // includes the actual JSC error in at least one case.
        //
        // So, if "__cxa_bad_typeid" shows up in the jscE exception
        // message, then we will assume that's the failure and just
        // throw now.

        if (jscE.getMessage().contains("__cxa_bad_typeid")) {
          throw jscE;
        }

        // Otherwise use Hermes
        try {
          HermesExecutor.loadLibrary();
          return new HermesExecutorFactory();
        } catch (UnsatisfiedLinkError hermesE) {
          // If we get here, either this is a JSC build, and of course
          // Hermes failed (since it's not in the APK), or it's a Hermes
          // build, and Hermes had a problem.

          // We suspect this is a JSC issue (it's the default), so we
          // will throw that exception, but we will print hermesE first,
          // since it could be a Hermes issue and we don't want to
          // swallow that.
          hermesE.printStackTrace();
          throw jscE;
        }
      }
    }
    // if the app explicitly specifies which engine to use, directly load
    // that library
    else{
      try {
        initializeSoLoaderIfNecessary(applicationContext);
        if(jscOrHermes.equals("hermes")){
          HermesExecutor.loadLibrary();
          return new HermesExecutorFactory();
        }
        else{
          JSCExecutor.loadLibrary();
          return new JSCExecutorFactory(appName, deviceName);
        }
      } catch (UnsatisfiedLinkError e) {
        // linking library failed, so we print its stacktrace and
        // throw the exception
        e.printStackTrace();
        throw e;
      }
    }
  }

Test Plan

The testing for this change might be tricky but can be done by following the reproduction steps in the issues related to DSO loading here: https://github.com/facebook/react-native/issues?q=is%3Aissue+is%3Aopen+DSO

Generally, the app will not crash anymore on deploying debug using android studio if we are removing libjsc and its related libraries in packagingOptions in build.gradle and using hermes in the project.
It can be like:

packagingOptions {
        if (enableHermes) {
            exclude "**/libjsc*.so"
        }
    }

…hermes. Based on the arguments, the correct .so file can be loaded without relying on try catch to select the correct one.

analysis-bot · 2022-02-16T14:45:19Z

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

Base commit: 7e8cce3
Branch: main

analysis-bot · 2022-02-16T14:54:37Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,189,094	+916
android	hermes	armeabi-v7a	7,789,530	+1,227
android	hermes	x86	8,559,181	+1,056
android	hermes	x86_64	8,511,564	+399
android	jsc	arm64-v8a	9,857,613	+217
android	jsc	armeabi-v7a	8,842,921	+867
android	jsc	x86	9,824,304	+867
android	jsc	x86_64	10,420,771	+162

Base commit: 7e8cce3
Branch: main

cortinico · 2022-02-17T12:07:04Z

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

@@ -66,9 +66,20 @@
  private @Nullable Map<String, RequestHandler> mCustomPackagerCommandHandlers;
  private @Nullable ReactPackageTurboModuleManagerDelegate.Builder mTMMDelegateBuilder;
  private @Nullable SurfaceDelegateFactory mSurfaceDelegateFactory;
+  private String jscOrHermes = "";


Why are you using an extra string here? A boolean would be sufficient

I am using it to keep the old logic intact.
In this way, apps already using the old logic and running fine won't need to change anything and those who were facing crashes can update based on the method I mentioned in the summary.
The empty case uses the old try catch logic and when the string is set, it uses the new logic.

It can be updated to use a boolean only if you want to mandate all apps to use the new approach and pass the value for hermesEnabled in the ReactInstanceManager :: builder method.
The old logic would be removed in that case. That is what we have done in our fork of react-native.
I have kept it as a string for legacy compatibility.

Let's update this to be a Boolean hermesEnabled = null, with the current logic:

null -> Use the previous logic

true -> Load Hermes or fail

false -> Load JSC or fail

KunalFarmah98 · 2022-02-21T15:30:30Z

@cortinico, are any updates required in the code?

KunalFarmah98 · 2022-03-17T18:08:34Z

Hi @cortinico . Can you please check this PR as well.

cortinico · 2022-03-17T19:09:52Z

Not sure what was going on here. Could you try to rebase? I'll look into this PR next week 👍

KunalFarmah98 · 2022-03-28T14:14:34Z

Is the review done @cortinico

KunalFarmah98 · 2022-03-30T03:46:58Z

Not sure what was going on here. Could you try to rebase? I'll look into this PR next week 👍

Any update @cortinico?

KunalFarmah98 · 2022-04-13T17:48:53Z

He @cortinico, are you reviewing this?

cortinico · 2022-04-14T18:55:19Z

He @cortinico, are you reviewing this?

Yup it's on me, I'll get back to this next week 👍

KunalFarmah98 · 2022-05-02T15:17:25Z

still not reviewed?

KunalFarmah98 · 2022-05-31T17:23:17Z

Hey @cortinico, is this getting reviewed or not?

cortinico · 2022-05-31T18:16:09Z

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

@@ -66,9 +66,20 @@
  private @Nullable Map<String, RequestHandler> mCustomPackagerCommandHandlers;
  private @Nullable ReactPackageTurboModuleManagerDelegate.Builder mTMMDelegateBuilder;
  private @Nullable SurfaceDelegateFactory mSurfaceDelegateFactory;
+  private String jscOrHermes = "";


Let's update this to be a Boolean hermesEnabled = null, with the current logic:

null -> Use the previous logic

true -> Load Hermes or fail

false -> Load JSC or fail

cortinico · 2022-05-31T18:18:28Z

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

+    else{
      try {
-        HermesExecutor.loadLibrary();
-        return new HermesExecutorFactory();
-      } catch (UnsatisfiedLinkError hermesE) {
-        // If we get here, either this is a JSC build, and of course
-        // Hermes failed (since it's not in the APK), or it's a Hermes
-        // build, and Hermes had a problem.
-
-        // We suspect this is a JSC issue (it's the default), so we
-        // will throw that exception, but we will print hermesE first,
-        // since it could be a Hermes issue and we don't want to
-        // swallow that.
-        hermesE.printStackTrace();
-        throw jscE;
+        initializeSoLoaderIfNecessary(applicationContext);
+        if(jscOrHermes.equals("hermes")){
+          HermesExecutor.loadLibrary();
+          return new HermesExecutorFactory();
+        }
+        else{
+          JSCExecutor.loadLibrary();
+          return new JSCExecutorFactory(appName, deviceName);
+        }
+      } catch (UnsatisfiedLinkError e) {
+        // linking library failed, so we print its stacktrace and
+        // throw the exception
+        e.printStackTrace();
+        throw e;


Let's try to refactor this a bit:

if (enableHermes == null || enableHermes == false) { JSCExecutor.loadLibrary(); else if (enableHermes == null || enableHermes == true) { HermesExecutor.loadLibrary(); }

There is no need to duplicate code

This seems fine, however, I seem to have lost the original repository from where I raised the PR. I would have to raise this PR again. I will ping the link to that PR here before closing this one.

Sure take your time 👍

Hey @cortinico. I have raised a new and updated PR for the same here: #33952
Kindly review that.

I will close this PR once that one is reviewed.

cortinico · 2022-05-31T18:19:01Z

Hey @cortinico, is this getting reviewed or not?

Sorry for the late review. This fell through my desk.
Left a couple of comments which I'd like you to address if possible

KunalFarmah98 · 2022-06-10T19:13:56Z

Hey @cortinico, is this getting reviewed or not?

Sorry for the late review. This fell through my desk. Left a couple of comments which I'd like you to address if possible

Hey @cortinico can you review the new PR #33952

cortinico · 2022-06-15T22:47:00Z

Yup I'll close this

Added additional builder method receiving arguments for using jsc or …

49d648c

…hermes. Based on the arguments, the correct .so file can be loaded without relying on try catch to select the correct one.

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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Feb 16, 2022

KunalFarmah98 mentioned this pull request Feb 17, 2022

[Android] Updated logic for loading correct jsc or hermes engine .so files at app startup. #33122

Closed

cortinico reviewed Feb 17, 2022

View reviewed changes

KunalFarmah98 mentioned this pull request Mar 17, 2022

Logging a soft error when ReactRootView has an id other than -1 instead of crashing the app in hybrid apps. #33133

Closed

cortinico reopened this Mar 17, 2022

cortinico reviewed May 31, 2022

View reviewed changes

cortinico closed this Jun 15, 2022

Added additional builder method receiving arguments for using jsc or hermes to correctly decide which DSO to load at app startup. #33123

Added additional builder method receiving arguments for using jsc or hermes to correctly decide which DSO to load at app startup. #33123

Uh oh!

Conversation

KunalFarmah98 commented Feb 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Suggested changes in any Android App's MainApplication that extends ReactApplication to take advantage of this fix

Changelog

Test Plan

Uh oh!

analysis-bot commented Feb 16, 2022

Uh oh!

analysis-bot commented Feb 16, 2022

Uh oh!

cortinico Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KunalFarmah98 Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KunalFarmah98 Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cortinico May 31, 2022

Choose a reason for hiding this comment

Uh oh!

KunalFarmah98 commented Feb 21, 2022

Uh oh!

KunalFarmah98 commented Mar 17, 2022

Uh oh!

cortinico commented Mar 17, 2022

Uh oh!

KunalFarmah98 commented Mar 28, 2022

Uh oh!

KunalFarmah98 commented Mar 30, 2022

Uh oh!

KunalFarmah98 commented Apr 13, 2022

Uh oh!

cortinico commented Apr 14, 2022

Uh oh!

KunalFarmah98 commented May 2, 2022

Uh oh!

KunalFarmah98 commented May 31, 2022

Uh oh!

cortinico May 31, 2022

Choose a reason for hiding this comment

Uh oh!

cortinico May 31, 2022

Choose a reason for hiding this comment

Uh oh!

KunalFarmah98 Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

cortinico Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

KunalFarmah98 Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

cortinico commented May 31, 2022

Uh oh!

KunalFarmah98 commented Jun 10, 2022

Uh oh!

cortinico commented Jun 15, 2022

Uh oh!

Uh oh!

KunalFarmah98 commented Feb 16, 2022 •

edited

Loading

cortinico Feb 17, 2022 •

edited

Loading

KunalFarmah98 Feb 17, 2022 •

edited

Loading

KunalFarmah98 Feb 17, 2022 •

edited

Loading