Skip to content

Commit d9f2491

Browse files
Xin Chenfacebook-github-bot
authored andcommittedFeb 7, 2023
Fix edge case when layout animation caused delete and create mutations in the same batch
Summary: This is a two step (2/2) fix to a race that could caused a DELETE...CREATE mutations being sent over to the fabric mounting layer. Such combination was assumed not possible from the differ (https://fburl.com/code/kg8z9t4w), yet it happened at least in the existence of layout animation and when certain commits happen while animation is active. This diff fixes all potential races in the Fabric mounting layer directly. It captures all the `DELETE...CREATE` combinations and stop those from passing down to the native platforms. This should fix all such races should them not captured by the fix in the layout animation. To help understand other races better, I also logged here to indicate such race so that future crashes will have more context. Changelog: [General][Fixed] - Fix edge case when layout animation caused delete and create mutations in the same batch Reviewed By: javache Differential Revision: D41900201 fbshipit-source-id: 280502ca32ce87a9e483cd859b11bcd3e5c4a435
·
v0.81.0-rc.0latest
1 parent 5b8faae commit d9f2491

File tree

3 files changed

+36
-3
lines changed

3 files changed

+36
-3
lines changed
 

‎ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,4 +115,10 @@ public class ReactFeatureFlags {
115115
* state in Fabric SurfaceMountingManager.
116116
*/
117117
public static boolean reduceDeleteCreateMutationLayoutAnimation = true;
118+
119+
/**
120+
* Allow fix to drop delete...create mutations which could cause missing view state in Fabric
121+
* SurfaceMountingManager.
122+
*/
123+
public static boolean reduceDeleteCreateMutation = false;
118124
}

‎ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.cpp

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,9 @@ FabricMountingManager::FabricMountingManager(
4040
std::shared_ptr<const ReactNativeConfig> &config,
4141
global_ref<jobject> &javaUIManager)
4242
: javaUIManager_(javaUIManager),
43-
useOverflowInset_(getFeatureFlagValue("useOverflowInset")) {
43+
useOverflowInset_(getFeatureFlagValue("useOverflowInset")),
44+
reduceDeleteCreateMutation_(
45+
getFeatureFlagValue("reduceDeleteCreateMutation")) {
4446
CoreFeatures::enableMapBuffer = getFeatureFlagValue("useMapBufferProps");
4547
}
4648

@@ -314,9 +316,33 @@ void FabricMountingManager::executeMount(
314316
bool isVirtual = mutation.mutatedViewIsVirtual();
315317
switch (mutationType) {
316318
case ShadowViewMutation::Create: {
317-
bool allocationCheck =
319+
bool shouldCreateView =
318320
!allocatedViewTags.contains(newChildShadowView.tag);
319-
bool shouldCreateView = allocationCheck;
321+
if (reduceDeleteCreateMutation_) {
322+
// Detect DELETE...CREATE situation on the same node and do NOT push
323+
// back to the mount items. This is an edge case that may happen
324+
// when for example animation runs while commit happened, and we
325+
// want to filter them out here to capture all possible sources of
326+
// such mutations. The re-ordering logic here assumes no
327+
// DELETE...CREATE in the mutations, as we will re-order mutations
328+
// and batch all DELETE instructions in the end.
329+
auto it = std::remove_if(
330+
cppDeleteMountItems.begin(),
331+
cppDeleteMountItems.end(),
332+
[&](auto &deletedMountItem) -> bool {
333+
return deletedMountItem.oldChildShadowView.tag ==
334+
newChildShadowView.tag;
335+
});
336+
bool hasDeletedViewsWithSameTag = it != cppDeleteMountItems.end();
337+
cppDeleteMountItems.erase(it, cppDeleteMountItems.end());
338+
339+
if (hasDeletedViewsWithSameTag) {
340+
shouldCreateView = false;
341+
LOG(ERROR)
342+
<< "XIN: Detect DELETE...CREATE on the same tag from mutations in the same batch. The DELETE and CREATE mutations are removed before sending to the native platforms";
343+
}
344+
}
345+
320346
if (shouldCreateView) {
321347
cppCommonMountItems.push_back(
322348
CppMountItem::CreateMountItem(newChildShadowView));

‎ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ class FabricMountingManager final {
7070
std::recursive_mutex allocatedViewsMutex_;
7171

7272
bool const useOverflowInset_{false};
73+
bool const reduceDeleteCreateMutation_{false};
7374

7475
jni::local_ref<jobject> getProps(
7576
ShadowView const &oldShadowView,

0 commit comments

Comments
 (0)
Please sign in to comment.