From 5ae8e84dc7c24a1bed80a00224fdfa0a8ef4f41f Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Fri, 5 Feb 2021 00:54:02 -0800 Subject: [PATCH] Fix IntBufferMountItem optimization + LayoutAnimation bug Summary: The new IntBufferMountItem queueing actually enforces a global ordering of mutation types: CREATEs, then INSERT, then REMOVE, then UPDATE, then DELETE. See comments for more details. In general this ordering is fine, but if a DELETE animation is in progress and (due to view unflattening) the same view is recreated, the CREATE will be executed and then the in-process DELETE (since conflicting animations get flushed). To mitigate this, in Binding we detect this and simply remove DELETE operations queued when we detect that we want to CREATE the node. `DELETE...CREATE` is valid but `CREATE...DELETE` in a single frame is not, so this is safe. This does complicate and add more assumptions to Binding than I would like, but it should unblock us for now. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D26271299 fbshipit-source-id: 9453fe17b8b541e484a047dc9637674dbdcc8e9a --- .../com/facebook/react/fabric/jni/Binding.cpp | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp index 5d4ff6ae297..88def6a0081 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp @@ -560,6 +560,24 @@ void Binding::schedulerDidFinishTransaction( newChildShadowView.props->revision > 1) { cppCommonMountItems.push_back( CppMountItem::CreateMountItem(newChildShadowView)); + + // Generally, DELETE operations can always safely execute at the end + // of a MountItem batch. The usual expected order would be REMOVE and + // then DELETE, for instance. However... in specific cases with + // LayoutAnimations especially, a DELETE and CREATE may happen for a + // View - in that order. The inverse is NOT possible - for example, we + // do not expect a CREATE...DELETE in the same batch. That would + // contradict itself - a node cannot be in the tree (CREATE) and + // removed from the tree (DELETE) at the same time. + cppDeleteMountItems.erase( + std::remove_if( + cppDeleteMountItems.begin(), + cppDeleteMountItems.end(), + [&](const CppMountItem &mountItem) { + return mountItem.oldChildShadowView.tag == + newChildShadowView.tag; + }), + cppDeleteMountItems.end()); } break; }