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

[KunalFarmah98]

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 :: setJsEngineAsHermes method that accepts a Boolean argument from the host app 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 setter method:

  /**
   * Sets the jsEngine as JSC or HERMES as per the setJsEngineAsHermes call
   * Uses the enum {@link JSInterpreter}
   * @param jsEngine
   */
  private void setJSEngine(JSInterpreter jsEngine){
    this.jsEngine = jsEngine;
  }

  /**
   * Utility setter to set the required JSEngine as HERMES or JSC
   * Defaults to OLD_LOGIC if not called by the host app
   * @param hermesEnabled
   * hermesEnabled = true sets the JS Engine as HERMES and JSC otherwise
   */
  public ReactInstanceManagerBuilder setJsEngineAsHermes(boolean hermesEnabled){
    if(hermesEnabled){
      setJSEngine(JSInterpreter.HERMES);
    }
    else{
      setJSEngine(JSInterpreter.JSC);
    }
    return this;
  }

The code snippet for the new logic in ReactInstanceManagerBuilder:

1. Setting up the new logic:
   Adding a new enum class :

  public enum JSInterpreter {
    OLD_LOGIC,
    JSC,
    HERMES
  }

A setter getting boolean value telling whether to use hermes or not and calling a private setter to update the enum variable.

 /**
   * Sets the jsEngine as JSC or HERMES as per the setJsEngineAsHermes call
   * Uses the enum {@link JSInterpreter}
   * @param jsEngine
   */
  private void setJSEngine(JSInterpreter jsEngine){
    this.jsEngine = jsEngine;
  }

  /**
   * Utility setter to set the required JSEngine as HERMES or JSC
   * Defaults to OLD_LOGIC if not called by the host app
   * @param hermesEnabled
   * hermesEnabled = true sets the JS Engine as HERMES and JSC otherwise
   */
  public ReactInstanceManagerBuilder setJsEngineAsHermes(boolean hermesEnabled){
    if(hermesEnabled){
      setJSEngine(JSInterpreter.HERMES);
    }
    else{
      setJSEngine(JSInterpreter.JSC);
    }
    return this;
  }

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

   private JavaScriptExecutorFactory getDefaultJSExecutorFactory(
    String appName, String deviceName, Context applicationContext) {

    // 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 use the value of enableHermes received by the constructor
    // to decide which library to load at launch

    // if nothing is specified, use old loading method
    // else load the required engine
    if (jsEngine == JSInterpreter.OLD_LOGIC) {
      try {
        // If JSC is included, use it as normal
        initializeSoLoaderIfNecessary(applicationContext);
        JSCExecutor.loadLibrary();
        return new JSCExecutorFactory(appName, deviceName);
      } catch (UnsatisfiedLinkError jscE) {
        if (jscE.getMessage().contains("__cxa_bad_typeid")) {
          throw jscE;
        }
        HermesExecutor.loadLibrary();
        return new HermesExecutorFactory();
      }
    } else if (jsEngine == JSInterpreter.HERMES) {
      HermesExecutor.loadLibrary();
      return new HermesExecutorFactory();
    } else {
      JSCExecutor.loadLibrary();
      return new JSCExecutorFactory(appName, deviceName);
    }
  }

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

builder = ReactInstanceManager.builder()
                .setApplication(this)
                .setJsEngineAsHermes(BuildConfig.HERMES_ENABLED)
                .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] - An enum JSInterpreter in com.facebook.react package:

/**
 * An enum that specifies the JS Engine to be used in the app
 * Old Logic uses the legacy code
 * JSC/HERMES loads the respective engine using the revamped logic
 */
public enum JSInterpreter {
  OLD_LOGIC,
  JSC,
  HERMES
}

[GENERAL] [ADDED] - An enum variable storing the default value of Js Engine loading mechanism in ReactInstanceManagerBuilder:

   private JSInterpreter  jsEngine = JSInterpreter.OLD_LOGIC;

[GENERAL] [ADDED] - A new setter method and a helper method to set the js engine in ReactInstanceManagerBuilder:

  /**
   * Sets the jsEngine as JSC or HERMES as per the setJsEngineAsHermes call
   * Uses the enum {@link JSInterpreter}
   * @param jsEngine
   */
  private void setJSEngine(JSInterpreter jsEngine){
    this.jsEngine = jsEngine;
  }

  /**
   * Utility setter to set the required JSEngine as HERMES or JSC
   * Defaults to OLD_LOGIC if not called by the host app
   * @param hermesEnabled
   * hermesEnabled = true sets the JS Engine as HERMES and JSC otherwise
   */
  public ReactInstanceManagerBuilder setJsEngineAsHermes(boolean hermesEnabled){
    if(hermesEnabled){
      setJSEngine(JSInterpreter.HERMES);
    }
    else{
      setJSEngine(JSInterpreter.JSC);
    }
    return this;
  }

[GENERAL] [ADDED] - Modified getDefaultJSExecutorFactory method

private JavaScriptExecutorFactory getDefaultJSExecutorFactory(
    String appName, String deviceName, Context applicationContext) {

    // 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 use the value of enableHermes received by the constructor
    // to decide which library to load at launch

    // if nothing is specified, use old loading method
    // else load the required engine
    if (jsEngine == JSInterpreter.OLD_LOGIC) {
      try {
        // If JSC is included, use it as normal
        initializeSoLoaderIfNecessary(applicationContext);
        JSCExecutor.loadLibrary();
        return new JSCExecutorFactory(appName, deviceName);
      } catch (UnsatisfiedLinkError jscE) {
        if (jscE.getMessage().contains("__cxa_bad_typeid")) {
          throw jscE;
        }
        HermesExecutor.loadLibrary();
        return new HermesExecutorFactory();
      }
    } else if (jsEngine == JSInterpreter.HERMES) {
      HermesExecutor.loadLibrary();
      return new HermesExecutorFactory();
    } else {
      JSCExecutor.loadLibrary();
      return new JSCExecutorFactory(appName, deviceName);
    }
  }

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"
        }
    }

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,789,422	-34,900
android	hermes	armeabi-v7a	7,193,018	-18,608
android	hermes	x86	8,100,825	-34,016
android	hermes	x86_64	8,078,578	-36,821
android	jsc	arm64-v8a	9,656,783	-33,842
android	jsc	armeabi-v7a	8,429,273	-17,561
android	jsc	x86	9,608,957	-32,959
android	jsc	x86_64	10,203,494	-35,777

Base commit: 97291bf
Branch: main

[analysis-bot]

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

Base commit: 97291bf
Branch: main

[facebook-github-bot]

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

[cortinico]

Left a minor nit, but it's good to go otherwise

[cortinico]

Suggested change

	* Constructor that takes a Boolean specifying weather to load hermes or jsc
	* Constructor that takes a Boolean specifying whether to load hermes or jsc

[KunalFarmah98]

oops. Fixed it :)

[cipolleschi]

Hi @KunalFarmah98, thanks a lot for this PR.

I left a comment to improve the readability and maintainability of the code. Let me know if it makes sense and what do you think about it!

[cipolleschi]

Isn't better if we use an enum instead of a boolean flag?

We can have an:

enum JSInterpreter {
  OLD_LOGIC,
  JSC,
  HERMES
}

And switch over it. It will improve readability, it avoids possible bugs due to the use of a null value which has unclear semantic and it is ready if we would ever add a new interpreter later on.

[KunalFarmah98]

Hi @cipolleschi , the Boolean is passed via the host app's BuildConfig. Hence keeping it null makes much more sense as no argument would automatically imply to use the old logic.

You can refer to the recommended changes I have mentioned in the host app to make use of this feature until it is made mandatory to be passed.

[cortinico]

@KunalFarmah98 I understand your point, though using an enum here is a bit more future proof.

Can I ask you to update the ReactInstanceManagerBuilder to:

1. Remove the constructor which accepts a boolean in input
2. Add a setJsEngine(JSEngine) method that accepts the enum
3. Add a setJsEngineAsHermes(boolean) parameter which will if true setJsEngine(HERMES) else setJsEngine(JSC)?

[KunalFarmah98]

Hi @cortinico.
I have made the requested changes. I will update the PR description in some time. Meanwhile you can review the new changes.

[KunalFarmah98]

PR description is updated. @cortinico @cipolleschi you can review it now

[cortinico]

Thanks for doing it @KunalFarmah98 👍 Left a minor comment, and we should be good to import/merge it

[cortinico]

Suggested change

	private JSInterpreter jsEngine = JSInterpreter.OLD_LOGIC;
	private JSInterpreter jsInterpreter = JSInterpreter.OLD_LOGIC;

Let's use jsInterpreter everywhere to keep it aligned with the enum class name

[KunalFarmah98]

done

[cortinico]

Let's keep this in a separate file as it was before

[KunalFarmah98]

Android CI tests are failing in that case, I tried 3 times.

[cortinico]

Tests were failing as you used a import com.facebook.react.util.JSInterpreter;. You should be good placing JSInterpreter inside the com.facebook.react package

[KunalFarmah98]

done @cortinico please review

[facebook-github-bot]

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

[cortinico]

Thanks for sendign this over @KunalFarmah98. Sorry it took longer than expect to merge, but I'm glad we have it in 👍

[react-native-bot]

This pull request was successfully merged by @KunalFarmah98 in 87cfd38.

^{When will my fix make it into a release? | Upcoming Releases}

[KunalFarmah98]

Thanks for sendign this over @KunalFarmah98. Sorry it took longer than expect to merge, but I'm glad we have it in 👍

Thanks @cortinico. I regularly fix react native source code crashes in our app. Will keep contributing to maintain its stability. 👍