Skip to content

Commit

Permalink
Throw soft-error and continue when removing view from parent with inc…
Browse files Browse the repository at this point in the history
…orrect index

Summary:
iOS Fabric actually ignores the `index` property and just uses parent and child tags to remove the child from a parent. This brings Android slightly closer to iOS: we try to use the index, but if the index is incorrect, we either (1) throw if the child isn't contained in the parent, or (2) find the correct index, and continue.

In debug, this will still crash, so we'll get more signal about why this happens.

Changelog: [Internal]

Reviewed By: shergin

Differential Revision: D24056375

fbshipit-source-id: 07507cc32ad02505d3271fc95ecb45d080109078
  • Loading branch information
JoshuaGross authored and facebook-github-bot committed Oct 2, 2020
1 parent 3b6b039 commit a78a716
Showing 1 changed file with 32 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ public void sendAccessibilityEvent(int reactTag, int eventType) {
}

@UiThread
public void removeViewAt(final int tag, final int parentTag, final int index) {
public void removeViewAt(final int tag, final int parentTag, int index) {
UiThreadUtil.assertOnUiThread();
ViewState viewState = getNullableViewState(parentTag);

Expand Down Expand Up @@ -411,18 +411,30 @@ public void removeViewAt(final int tag, final int parentTag, final int index) {
return;
}

// Here we are guaranteed that the view is still in the View hierarchy, just
// at a different index. In debug mode we'll crash here; in production, we'll remove
// the child from the parent and move on.
// This is an issue that is safely recoverable 95% of the time. If this allows corruption
// of the view hierarchy and causes bugs or a crash after this point, there will be logs
// indicating that this happened.
// This is likely *only* necessary because of Fabric's LayoutAnimations implementation.
// If we can fix the bug there, or remove the need for LayoutAnimation index adjustment
// entirely, we can just throw this exception without regression user experience.
logViewHierarchy(parentView, true);
throw new IllegalStateException(
"Tried to remove view ["
+ tag
+ "] of parent ["
+ parentTag
+ "] at index "
+ index
+ ", but got view tag "
+ actualTag
+ " - actual index of view: "
+ tagActualIndex);
ReactSoftException.logSoftException(
TAG,
new IllegalStateException(
"Tried to remove view ["
+ tag
+ "] of parent ["
+ parentTag
+ "] at index "
+ index
+ ", but got view tag "
+ actualTag
+ " - actual index of view: "
+ tagActualIndex));
index = tagActualIndex;
}

try {
Expand Down Expand Up @@ -460,13 +472,20 @@ public void removeViewAt(final int tag, final int parentTag, final int index) {

// Display children after deleting any
if (SHOW_CHANGED_VIEW_HIERARCHIES) {
final int finalIndex = index;
UiThreadUtil.runOnUiThread(
new Runnable() {
@Override
public void run() {
FLog.e(
TAG,
"removeViewAt: [" + tag + "] -> [" + parentTag + "] idx: " + index + " AFTER");
"removeViewAt: ["
+ tag
+ "] -> ["
+ parentTag
+ "] idx: "
+ finalIndex
+ " AFTER");
logViewHierarchy(parentView, false);
}
});
Expand Down

0 comments on commit a78a716

Please sign in to comment.