From 8da92d746b0fa78ce1a1ac1ad2563000eb188588 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Wed, 27 May 2020 22:50:27 -0700 Subject: [PATCH] Fix crash in removeViewAt by fixing incorrect error-detection Summary: As a followup to D21750097 and D21735940, it seems that ANY uses of childCount will be incorrect as they are "often" reported, incorrectly, as just 0. This is likely due to exotic cases like (1) the ViewManager childCount being overridden, or (2) special ViewManagers like BottomSheet where the childCount may actually be 1/0 even though many children are inserted. Anyway, as a more generic fix, let's only rethrow an existing Exception with additional diagnostics instead of trying to detect when this /would/ crash. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D21756178 fbshipit-source-id: 17ffb2ed531978bae8d4db19d7b87ec62397e44b --- .../fabric/mounting/MountingManager.java | 28 +++++++++++++++---- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java index 7d89e34ebf7..e48192e8963 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java @@ -244,18 +244,36 @@ public class MountingManager { ViewGroupManager viewGroupManager = getViewGroupManager(viewState); - if (viewGroupManager.getChildCount(parentView) <= index) { + try { + viewGroupManager.removeViewAt(parentView, index); + } catch (RuntimeException e) { + // Note: `getChildCount` may not always be accurate! + // We don't currently have a good explanation other than, in situations where you + // would empirically expect to see childCount > 0, the childCount is reported as 0. + // This is likely due to a ViewManager overriding getChildCount or some other methods + // in a way that is strictly incorrect, but potentially only visible here. + // The failure mode is actually that in `removeViewAt`, a NullPointerException is + // thrown when we try to perform an operation on a View that doesn't exist, and + // is therefore null. + // We try to add some extra diagnostics here, but we always try to remove the View + // from the hierarchy first because detecting by looking at childCount will not work. + // + // Note that the lesson here is that `getChildCount` is not /required/ to adhere to + // any invariants. If you add 9 children to a parent, the `getChildCount` of the parent + // may not be equal to 9. This apparently causes no issues with Android and is common + // enough that we shouldn't try to change this invariant, without a lot of thought. + int childCount = viewGroupManager.getChildCount(parentView); + throw new IllegalStateException( "Cannot remove child at index " + index + " from parent ViewGroup [" + parentView.getId() + "], only " - + parentView.getChildCount() - + " children in parent"); + + childCount + + " children in parent. Warning: childCount may be incorrect!", + e); } - - viewGroupManager.removeViewAt(parentView, index); } @UiThread