Log a soft exception if RootView has id other than View.NO_ID in 0.66-stable #33991

SparshaSaha · 2022-06-10T10:22:54Z

…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

Summary

This PR will cherry-pick the above mentioned fix to 0.66-stable

Changelog

Cherry-picking the above commit to 0.66-stable

[CATEGORY] [TYPE] - Message

…ad of crashing the app in hybrid apps. (facebook#33133) Summary: As per commit: facebook@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: facebook#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: facebook#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

facebook-github-bot · 2022-06-10T10:22:59Z

Hi @SparshaSaha!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

pull-bot · 2022-06-10T10:32:34Z

	Warnings
⚠️	❔ Base Branch - The base branch for this PR is something other than `main`. Are you sure you want to merge these changes into a stable release? If you are interested in backporting updates to an older release, the suggested approach is to land those changes on `main` first and then cherry-pick the commits into the branch for that release. The Releases Guide has more information.

	Messages
📖	📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.

Generated by 🚫 dangerJS against 7db7a14

analysis-bot · 2022-06-10T10:53:44Z

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

Base commit: 6d2872d

analysis-bot · 2022-06-10T11:00:32Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,942,996	+1,155,626
android	hermes	armeabi-v7a	8,477,891	+1,301,085
android	hermes	x86	9,362,012	+1,263,568
android	hermes	x86_64	9,329,243	+1,253,553
android	jsc	arm64-v8a	10,626,423	+972,482
android	jsc	armeabi-v7a	9,548,823	+1,136,554
android	jsc	x86	10,639,714	+1,033,920
android	jsc	x86_64	11,250,634	+1,050,588

Base commit: 6d2872d

SparshaSaha changed the title ~~Log a sift exception~~ Log a soft exception if RootView has id other than View.NO_ID Jun 10, 2022

SparshaSaha changed the title ~~Log a soft exception if RootView has id other than View.NO_ID~~ Log a soft exception if RootView has id other than View.NO_ID in 0.66-stable Jun 10, 2022

SparshaSaha closed this Jun 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Log a soft exception if RootView has id other than View.NO_ID in 0.66-stable #33991

Log a soft exception if RootView has id other than View.NO_ID in 0.66-stable #33991

Uh oh!

SparshaSaha commented Jun 10, 2022

Uh oh!

facebook-github-bot commented Jun 10, 2022

Uh oh!

pull-bot commented Jun 10, 2022

Uh oh!

analysis-bot commented Jun 10, 2022

Uh oh!

analysis-bot commented Jun 10, 2022

Uh oh!

Uh oh!

Log a soft exception if RootView has id other than View.NO_ID in 0.66-stable #33991

Log a soft exception if RootView has id other than View.NO_ID in 0.66-stable #33991

Uh oh!

Conversation

SparshaSaha commented Jun 10, 2022

Snippet of the change:

Changelog

Summary

Changelog

Uh oh!

facebook-github-bot commented Jun 10, 2022

Action Required

Process

Uh oh!

pull-bot commented Jun 10, 2022

Uh oh!

analysis-bot commented Jun 10, 2022

Uh oh!

analysis-bot commented Jun 10, 2022

Uh oh!

Uh oh!