Skip to content

Commit

Permalink
Guard against setId being called on the ReactRootView outside of RN core
Browse files Browse the repository at this point in the history
Summary:
The RootView being managed by Fabric should have an id of View.NO_ID when it is "handed over" to RN. This is true for Fabric and non-Fabric
and setting a custom id on the ReactRootView has never been supported. I'm temporarily (?) adding an additional check earlier and into ReactRootView to hopefully
catch any of these issues early.

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D26077509

fbshipit-source-id: 59e1ec080504e50698acc654c29120f039238a96
  • Loading branch information
JoshuaGross authored and facebook-github-bot committed Jan 26, 2021
1 parent 335d9d8 commit 4f3b174
Showing 1 changed file with 10 additions and 0 deletions.
10 changes: 10 additions & 0 deletions ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java
Expand Up @@ -646,6 +646,16 @@ private void attachToReactInstanceManager() {
Systrace.beginSection(TRACE_TAG_REACT_JAVA_BRIDGE, "attachToReactInstanceManager");
ReactMarker.logMarker(ReactMarkerConstants.ROOT_VIEW_ATTACH_TO_REACT_INSTANCE_MANAGER_START);

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

try {
if (mIsAttachedToInstance) {
return;
Expand Down

3 comments on commit 4f3b174

@KunalFarmah98
Copy link
Contributor

Choose a reason for hiding this comment

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

This is causing numerous crashes in hybrid apps using ReactRootView to display react-native screens as Activities. Can you please explain the reason for its addition.

@cortinico
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain the reason for its addition.

Isn't the comment on the code already clear enough? What else is unclear?

@KunalFarmah98
Copy link
Contributor

@KunalFarmah98 KunalFarmah98 commented on 4f3b174 Feb 17, 2022

Choose a reason for hiding this comment

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

Can you please explain the reason for its addition.

Isn't the comment on the code already clear enough? What else is unclear?

It causing the app to crash while staying in react native while the comment says it checks when controls comes back to RN.
I think this should be downgraded to a redbox warning as this used to work fine without this check in place for years in our hybrid app having 100 million+ users.

Please refer to this issue: #33121
We had to completely stop react native upgrade for our app and remove this code to make it working again.

Please sign in to comment.