Skip to content

Commit

Permalink
Fix crash in removeViewAt by fixing incorrect error-detection
Browse files Browse the repository at this point in the history
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
  • Loading branch information
JoshuaGross authored and facebook-github-bot committed May 28, 2020
1 parent 7e55946 commit 8da92d7
Showing 1 changed file with 23 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -244,18 +244,36 @@ public void removeViewAt(int parentTag, int index) {

ViewGroupManager<ViewGroup> 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
Expand Down

0 comments on commit 8da92d7

Please sign in to comment.