From 4df70dcf1f91e65f1f4565c7a174ba615310ac3e Mon Sep 17 00:00:00 2001
From: Sparsha Saha <saha.sparsha [at] gmail.com>
Date: Tue, 2 Aug 2022 11:29:20 +0530
Subject: [PATCH 1/2] Logging a soft error when ReactRootView has an id other
 than -1 instead of crashing the app in hybrid apps. (#33133)

Summary:
As per commit: https://github.com/facebook/react-native/commit/4f3b17412018a10f9293247c802598d2b94a844b which states that "React Native requires that the RootView id be managed entirely by React Native, and will crash in addRootView/startSurface if the native View id isn't set to NO_ID."

This behaviour can not be guaranteed in **hybrid** apps that have a native android layer over which ReactRootViews are added and the native views need to have ids on them in order to work. **The control of views can jump back and forth between native android and react-native (fabric). As the ReactRootView is added to ViewGroups (or layouts) in Android Fragments and Activities, they contain ids on their views which might get passed down to the reactRootView by features like DataBinding**

Hence this can cause unnecessary crashes at runtime for hybrid apps even when they are not changing the id of the reactRootView object they are adding to their ViewGroups.

Our app is a hybrid app that uses both native android and react-native on different screens and on one such screen that has a Fragment adding a ReactRootView to its FrameLayout to render native android views to render in ReactNative, this crash occurs on pressing the back button as well as on unlocking the screen while staying on the same screen.

The app was running fine on more than a 100 million devices on React Native 0.63.4 but after updating to 0.67.2, that features this commit, it crashes on the very first device it was tested on.

Refer to the issue: https://github.com/facebook/react-native/issues/33121 for more information on the crash

The fragment in which this issues arises is like this:

 ```binding.frameLayout.addView(getReactRootView())```

where getReactRootView() is like this:

```
    private var mReactRootView: ReactRootView? = null
    private var mReactInstanceManager: ReactInstanceManager? = null

    mReactRootView = ReactRootView(context)

        if (activity != null) {
            val application = activity?.application
            if (application is MainApplication) {
                mReactInstanceManager = application.reactInstanceManager
            }
        }

      fun getReactRootView():View?{
         return  mReactRootView
      }
```

So converting this to a soft exception such that pure react-native devs can still see the error while hybrid apps continue to run without crashes.

```
if (getId() != View.NO_ID) {
        ReactSoftExceptionLogger.logSoftException(
            TAG,
            new IllegalViewOperationException(
              "Trying to attach a ReactRootView with an explicit id already set to ["
                  + getId()
                  + "]. React Native uses the id field to track react tags and will overwrite this"
                  + " field. If that is fine, explicitly overwrite the id field to View.NO_ID."));
    }
```

[GENERAL] [ADDED] - A ReactSoftException log instead of a direct exception being thrown:

```
if (getId() != View.NO_ID) {
        ReactSoftExceptionLogger.logSoftException(
            TAG,
            new IllegalViewOperationException(
              "Trying to attach a ReactRootView with an explicit id already set to ["
                  + getId()
                  + "]. React Native uses the id field to track react tags and will overwrite this"
                  + " field. If that is fine, explicitly overwrite the id field to View.NO_ID."));
    }
```

[GENERAL] [REMOVED]- Directly throwing an exception even when the code is not responsible for this issue:

```
if (getId() != View.NO_ID) {
      throw new IllegalViewOperationException(
          "Trying to attach a ReactRootView with an explicit id already set to ["
              + getId()
              + "]. React Native uses the id field to track react tags and will overwrite this"
              + " field. If that is fine, explicitly overwrite the id field to View.NO_ID.");
    }

```

Pull Request resolved: https://github.com/facebook/react-native/pull/33133

Test Plan:
This crash is hard to reproduce but when it occurs, this is the only way to fix it.
If any app used to crash with this exact error, it will no longer crash but show an error log in Logcat for developers to be informed about the issue.

Reviewed By: ShikaSD

Differential Revision: D34304212

Pulled By: cortinico

fbshipit-source-id: f0eaeef2e905a6e0587df088b43cc49cabda397a
---
 .../com/facebook/react/ReactRootView.java     | 20 ++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java b/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java
index 3d5ee968202a..1cadadac30f8 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java
@@ -719,12 +719,22 @@ private void attachToReactInstanceManager() {
 
     // React Native requires that the RootView id be managed entirely by React Native, and will
     // crash in addRootView/startSurface if the native View id isn't set to NO_ID.
+
+    // This behavior can not be guaranteed in hybrid apps that have a native android layer over
+    // which reactRootViews are added and the native views need to have ids on them in order to
+    // work.
+    // Hence this can cause unnecessary crashes at runtime for hybrid apps.
+    // So converting this to a soft exception such that pure react-native devs can still see the
+    // warning while hybrid apps continue to run without crashes
+
     if (getId() != View.NO_ID) {
-      throw new IllegalViewOperationException(
-          "Trying to attach a ReactRootView with an explicit id already set to ["
-              + getId()
-              + "]. React Native uses the id field to track react tags and will overwrite this"
-              + " field. If that is fine, explicitly overwrite the id field to View.NO_ID.");
+      ReactSoftExceptionLogger.logSoftException(
+          TAG,
+          new IllegalViewOperationException(
+              "Trying to attach a ReactRootView with an explicit id already set to ["
+                  + getId()
+                  + "]. React Native uses the id field to track react tags and will overwrite this"
+                  + " field. If that is fine, explicitly overwrite the id field to View.NO_ID."));
     }
 
     try {

From 9df0f69dc357434634259b9cc04d643e20550acc Mon Sep 17 00:00:00 2001
From: Sparsha Saha <saha.sparsha@gmail.com>
Date: Tue, 2 Aug 2022 11:35:34 +0530
Subject: [PATCH 2/2] Added additional builder method receiving arguments for
 using jsc or hermes to correctly decide which DSO to load at app startup.
 (#33952)

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);
    }
  }
```

```
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.

[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);
    }
  }
```

Pull Request resolved: https://github.com/facebook/react-native/pull/33952

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

Reviewed By: lunaleaps

Differential Revision: D37191981

Pulled By: cortinico

fbshipit-source-id: c528ead126939f1d788af7523f3798ed2a14f36e
---
 .../com/facebook/react/JSInterpreter.java     | 18 +++++
 .../react/ReactInstanceManagerBuilder.java    | 80 ++++++++++++-------
 2 files changed, 68 insertions(+), 30 deletions(-)
 create mode 100644 ReactAndroid/src/main/java/com/facebook/react/JSInterpreter.java

diff --git a/ReactAndroid/src/main/java/com/facebook/react/JSInterpreter.java b/ReactAndroid/src/main/java/com/facebook/react/JSInterpreter.java
new file mode 100644
index 000000000000..7e960263cc55
--- /dev/null
+++ b/ReactAndroid/src/main/java/com/facebook/react/JSInterpreter.java
@@ -0,0 +1,18 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+package com.facebook.react;
+
+/**
+ * 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
+}
diff --git a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerBuilder.java b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerBuilder.java
index 199e4033cf01..3d140cd36a99 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerBuilder.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerBuilder.java
@@ -66,6 +66,7 @@ public class ReactInstanceManagerBuilder {
   private @Nullable Map<String, RequestHandler> mCustomPackagerCommandHandlers;
   private @Nullable ReactPackageTurboModuleManagerDelegate.Builder mTMMDelegateBuilder;
   private @Nullable SurfaceDelegateFactory mSurfaceDelegateFactory;
+  private JSInterpreter jsInterpreter = JSInterpreter.OLD_LOGIC;
 
   /* package protected */ ReactInstanceManagerBuilder() {}
 
@@ -125,6 +126,31 @@ public ReactInstanceManagerBuilder setJSBundleLoader(JSBundleLoader jsBundleLoad
     return this;
   }
 
+  /**
+   * Sets the jsEngine as JSC or HERMES as per the setJsEngineAsHermes call Uses the enum {@link
+   * JSInterpreter}
+   *
+   * @param jsInterpreter
+   */
+  private void setJSEngine(JSInterpreter jsInterpreter) {
+    this.jsInterpreter = jsInterpreter;
+  }
+
+  /**
+   * 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;
+  }
+
   /**
    * Path to your app's main module on Metro. This is used when reloading JS during development. All
    * paths are relative to the root folder the packager is serving files from. Examples: {@code
@@ -345,41 +371,35 @@ public ReactInstanceManager build() {
 
   private JavaScriptExecutorFactory getDefaultJSExecutorFactory(
       String appName, String deviceName, Context applicationContext) {
-    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
+    // 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 (jsInterpreter == 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();
-      } 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;
       }
+    } else if (jsInterpreter == JSInterpreter.HERMES) {
+      HermesExecutor.loadLibrary();
+      return new HermesExecutorFactory();
+    } else {
+      JSCExecutor.loadLibrary();
+      return new JSCExecutorFactory(appName, deviceName);
     }
   }
 }
