Skip to content

Commit

Permalink
Logging a soft error when ReactRootView has an id other than -1 inste…
Browse files Browse the repository at this point in the history
…ad of crashing the app in hybrid apps. (#33133)

Summary:
As per commit: 4f3b174 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: #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.

### Snippet of the change:

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

## Changelog

[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: #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
  • Loading branch information
Kunal-Airtel2022 authored and facebook-github-bot committed Mar 28, 2022
1 parent 199ac68 commit 1ca2c24
Showing 1 changed file with 15 additions and 5 deletions.
20 changes: 15 additions & 5 deletions ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java
Expand Up @@ -729,12 +729,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 {
Expand Down

3 comments on commit 1ca2c24

@psionic12
Copy link

@psionic12 psionic12 commented on 1ca2c24 Sep 24, 2022

Choose a reason for hiding this comment

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

Should this solution also applies to SurfaceMountingManager.java?

@cortinico
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this solution also applies to SurfaceMountingManager.java?

@psionic12 yes. Don't you mind sending over a PR?

@psionic12
Copy link

Choose a reason for hiding this comment

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

Done. #34785 (comment).

It seems that the main branch has problems, I can't compile the code at my workspace, due to CMake can not find files, so I didn't test if this commit has any problem.

Please sign in to comment.