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

Log a SoftException on SurfaceMountingManager.addRootView #34785

Closed
wants to merge 2 commits into from

Conversation

psionic12
Copy link
Contributor

@psionic12 psionic12 commented Sep 26, 2022

Summary

Follow the same solution (do not throw a crash when view ID is set already) used in ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java for ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java

Changelog

[Android] [Changed] - Log a SoftException on SurfaceMountingManager.addRootView

Test Plan

None

@facebook-github-bot 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 Sep 26, 2022
psionic12 referenced this pull request Sep 26, 2022
…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
Comment on lines 240 to 246
ReactSoftExceptionLogger.logSoftException(
TAG,
new IllegalViewOperationException(
"Trying to attach a ReactRootView with an explicit id already set to ["
+ rootView.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."));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we retain the original message?

@cortinico cortinico changed the title Follow the same solution in 1ca2c2493027c1b027146cd41e17dd8a4fc33a41 Log a SoftException on SurfaceMountingManager.addRootView Sep 26, 2022
@analysis-bot
Copy link

analysis-bot commented Sep 26, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,725,523 -4
android hermes armeabi-v7a 7,128,407 -4
android hermes x86 8,032,075 -8
android hermes x86_64 8,005,360 -6
android jsc arm64-v8a 9,595,866 -7
android jsc armeabi-v7a 8,362,177 +9
android jsc x86 9,540,082 -1
android jsc x86_64 10,132,493 +0

Base commit: cc13b02
Branch: main

@analysis-bot
Copy link

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

Base commit: cc13b02
Branch: main

@cortinico
Copy link
Contributor

📋 Verify Changelog Format - A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

Can you also specify the correct changelog entry?

@psionic12
Copy link
Contributor Author

@cortinico Done.

@facebook-github-bot
Copy link
Contributor

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

@react-native-bot react-native-bot added the Platform: Android Android applications. label Oct 3, 2022
@github-actions
Copy link

github-actions bot commented Apr 1, 2023

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Apr 1, 2023
@github-actions
Copy link

github-actions bot commented Apr 9, 2023

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this Apr 9, 2023
@cortinico cortinico reopened this Apr 11, 2023
@cortinico cortinico removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Apr 11, 2023
@github-actions
Copy link

Fails
🚫

📋 Missing Changelog - Please add a Changelog to your PR description. See Changelog format

Generated by 🚫 dangerJS against d189165

Copy link

github-actions bot commented Dec 9, 2023

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Dec 9, 2023
@cortinico cortinico removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Dec 9, 2023
Copy link

github-actions bot commented Jun 7, 2024

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Jun 7, 2024
Copy link

github-actions bot commented Jun 7, 2024

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@cortinico cortinico removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Jun 7, 2024
@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Jun 20, 2024
@facebook-github-bot
Copy link
Contributor

@cortinico merged this pull request in 3429dc1.

Copy link

This pull request was successfully merged by @psionic12 in 3429dc1.

When will my fix make it into a release? | How to file a pick request?

@cortinico
Copy link
Contributor

Sorry this took so long :(

cortinico added a commit to cortinico/react-native that referenced this pull request Jun 21, 2024
Summary:
This test broke after I merged
facebook#34785
yesterday.

Just fixing it in a similar way as the test above.

Changelog:
[Internal] [Changed] - Fix broken unableToAddHandledRootView

Differential Revision: D58864166
facebook-github-bot pushed a commit that referenced this pull request Jun 21, 2024
Summary:
Pull Request resolved: #45101

This test broke after I merged
#34785
yesterday.

Just fixing it in a similar way as the test above.

Changelog:
[Internal] [Changed] - Fix broken unableToAddHandledRootView

Reviewed By: rubennorte, blakef

Differential Revision: D58864166

fbshipit-source-id: 4f48dbfd5238a2811564ce02199af7fc284d39b4
thejamesgore added a commit to thejamesgore/react-native that referenced this pull request Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants