Skip to content

Commit 297b571

Browse files
JoshuaGrossfacebook-github-bot
authored andcommitted
Deferred tree deletion needs to take non-managed trees into account
Summary: Crashes can occur if we try to disassemble trees not managed by React Native - for example a native component tree, Litho hierarchies, etc. As we disassemble the tree, ensure that children are managed before disassembling a subtree. Changelog: [Internal] Reviewed By: ryancat Differential Revision: D37493854 fbshipit-source-id: fee4d303133edcef663abfe8637bce6b24627724
1 parent 3a7170c commit 297b571

File tree

1 file changed

+33
-8
lines changed

1 file changed

+33
-8
lines changed

ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java

+33-8
Original file line numberDiff line numberDiff line change
@@ -1430,23 +1430,48 @@ public void doFrameGuarded(long frameTimeNanos) {
14301430
if (thisViewState != null) {
14311431
View thisView = thisViewState.mView;
14321432
int numChildren = 0;
1433+
1434+
// Children are managed by React Native if both of the following are true:
1435+
// 1) There are 1 or more children of this View, which must be a ViewGroup
1436+
// 2) Those children are managed by RN (this is not the case for certain native
1437+
// components, like embedded Litho hierarchies)
1438+
boolean childrenAreManaged = false;
1439+
14331440
if (thisView instanceof ViewGroup) {
14341441
View nextChild = null;
14351442
// For reasons documented elsewhere in this class, getChildCount is not
1436-
// necessarily
1437-
// reliable, and so we rely instead on requesting children directly.
1443+
// necessarily reliable, and so we rely instead on requesting children directly.
14381444
while ((nextChild = ((ViewGroup) thisView).getChildAt(numChildren)) != null) {
1439-
if (numChildren == 0) {
1440-
// Push tag onto the stack so we reprocess it after all children
1441-
mReactTagsToRemove.push(reactTag);
1442-
}
1445+
int childId = nextChild.getId();
1446+
childrenAreManaged = childrenAreManaged || getNullableViewState(childId) != null;
14431447
mReactTagsToRemove.push(nextChild.getId());
14441448
numChildren++;
14451449
}
14461450
// Removing all at once is more efficient than removing one-by-one
1447-
((ViewGroup) thisView).removeAllViews();
1451+
// If the children are not managed by RN, we simply drop the entire
1452+
// subtree instead of recursing further.
1453+
if (childrenAreManaged) {
1454+
try {
1455+
// This can happen if the removeAllViews method is overriden to throw,
1456+
// which it is explicitly in some cases (for example embedded Litho views,
1457+
// but there could be other cases). In those cases, we want to fail silently
1458+
// and then assume the subtree is /not/ managed by React Native.
1459+
// In this case short-lived memory-leaks could occur if we aren't clearing
1460+
// out the ViewState map properly; but the risk should be small.
1461+
// In debug mode, the SoftException will cause a crash. In production it
1462+
// will not. This should give good visibility into whether or not this is
1463+
// a problem without causing user-facing errors.
1464+
((ViewGroup) thisView).removeAllViews();
1465+
} catch (RuntimeException e) {
1466+
childrenAreManaged = false;
1467+
ReactSoftExceptionLogger.logSoftException(TAG, e);
1468+
}
1469+
}
14481470
}
1449-
if (numChildren == 0) {
1471+
if (childrenAreManaged) {
1472+
// Push tag onto the stack so we reprocess it after all children
1473+
mReactTagsToRemove.push(reactTag);
1474+
} else {
14501475
mTagToViewState.remove(reactTag);
14511476
onViewStateDeleted(thisViewState);
14521477
}

0 commit comments

Comments
 (0)