From 664f9b11ba559ae3d28a9545a69f6b4eb6a9644e Mon Sep 17 00:00:00 2001 From: Samuel Susla Date: Thu, 3 Mar 2022 06:02:02 -0800 Subject: [PATCH] Add option to delay deletion of viewState Summary: changelog: [internal] ViewState is sometimes deleted but not created again because of race condition in C++ pre-allocation and how it checks for revision to determine if createView mount item should be dispatched. In this diff, we add an option to delay deletion of ViewState until the next commit. Reviewed By: ShikaSD Differential Revision: D34553174 fbshipit-source-id: e5213f1d39137fb56fc745321a7c1b28d417ea27 --- .../react/config/ReactFeatureFlags.java | 2 + .../mounting/SurfaceMountingManager.java | 51 +++++++++++++++++-- .../mountitems/IntBufferBatchMountItem.java | 2 + 3 files changed, 50 insertions(+), 5 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java index df15b06094f..e2e7da7fb19 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java +++ b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java @@ -98,4 +98,6 @@ public class ReactFeatureFlags { public static boolean enableAggressiveEventEmitterCleanup = false; public static boolean insertZReorderBarriersOnViewGroupChildren = true; + + public static boolean enableDelayedViewStateDeletion = false; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java index 159a27e5975..b9c8a6c2358 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java @@ -45,6 +45,7 @@ import com.facebook.react.uimanager.ViewManager; import com.facebook.react.uimanager.ViewManagerRegistry; import com.facebook.react.views.view.ReactMapBufferViewManager; import com.facebook.react.views.view.ReactViewManagerWrapper; +import java.util.HashSet; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentLinkedQueue; @@ -72,6 +73,14 @@ public class SurfaceMountingManager { // This is null *until* StopSurface is called. private Set mTagSetForStoppedSurface; + // C++ layer checks for prop revision and doesn't + // dispatch createView mount item if view pre-allocation mount item was dispatched. + // This leads to missing createView and pre-mature deletion of ViewState. + // To work around this issue, ViewState deletion is delayed until subsequent commit. + // If the subsequent commit accesses ViewState, it won't be deleted. + private Set mSoftDeletedViewStateTags; + private Set mScheduledForDeletionViewStateTags; + private final int mSurfaceId; public SurfaceMountingManager( @@ -88,6 +97,11 @@ public class SurfaceMountingManager { mRootViewManager = rootViewManager; mMountItemExecutor = mountItemExecutor; mThemedReactContext = reactContext; + + if (ReactFeatureFlags.enableDelayedViewStateDeletion) { + mSoftDeletedViewStateTags = new HashSet(); + mScheduledForDeletionViewStateTags = new HashSet(); + } } public boolean isStopped() { @@ -913,6 +927,23 @@ public class SurfaceMountingManager { } } + @UiThread + public void didUpdateViews() { + if (ReactFeatureFlags.enableDelayedViewStateDeletion) { + for (Integer reactTag : mScheduledForDeletionViewStateTags) { + // To delete we simply remove the tag from the registry. + // We want to rely on the correct set of MountInstructions being sent to the platform, + // or StopSurface being called, so we do not handle deleting descendents of the View. + ViewState viewState = mTagToViewState.remove(reactTag); + if (viewState != null) { + onViewStateDeleted(viewState); + } + } + mScheduledForDeletionViewStateTags = mSoftDeletedViewStateTags; + mSoftDeletedViewStateTags = new HashSet(); + } + } + @UiThread public void deleteView(int reactTag) { UiThreadUtil.assertOnUiThread(); @@ -930,12 +961,16 @@ public class SurfaceMountingManager { return; } - // To delete we simply remove the tag from the registry. - // We want to rely on the correct set of MountInstructions being sent to the platform, - // or StopSurface being called, so we do not handle deleting descendents of the View. - mTagToViewState.remove(reactTag); + if (ReactFeatureFlags.enableDelayedViewStateDeletion) { + mSoftDeletedViewStateTags.add(reactTag); + } else { + // To delete we simply remove the tag from the registry. + // We want to rely on the correct set of MountInstructions being sent to the platform, + // or StopSurface being called, so we do not handle deleting descendents of the View. + mTagToViewState.remove(reactTag); - onViewStateDeleted(viewState); + onViewStateDeleted(viewState); + } } @UiThread @@ -984,6 +1019,9 @@ public class SurfaceMountingManager { if (viewState == null) { throw new RetryableMountingLayerException("Unable to find viewState for tag " + tag); } + if (ReactFeatureFlags.enableDelayedViewStateDeletion) { + mScheduledForDeletionViewStateTags.remove(tag); + } return viewState; } @@ -991,6 +1029,9 @@ public class SurfaceMountingManager { if (mTagToViewState == null) { return null; } + if (ReactFeatureFlags.enableDelayedViewStateDeletion) { + mScheduledForDeletionViewStateTags.remove(tag); + } return mTagToViewState.get(tag); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java index 76154a08578..41d17f384ec 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java @@ -188,6 +188,8 @@ public class IntBufferBatchMountItem implements MountItem { } } + surfaceMountingManager.didUpdateViews(); + endMarkers(); }