From cf9c7d51efd99b65527f9b9d2fef0334b972a461 Mon Sep 17 00:00:00 2001 From: Xin Chen Date: Tue, 13 Dec 2022 01:17:37 -0800 Subject: [PATCH] Fix edge case when delete is queued with conflict layout animation Summary: This is a two step (1/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, yet it happened at least in the existence of layout animation and when certain commits happen while animation is active. This race condition could cause a view to get deleted at the end of one UI frame, yet the mount instructions generated from animation in the next frame still need to access the deleted view and caused crashes like T112157805. Note that even though such crash is recorded as `RetryableMountingLayerException` and is a soft crash (which only gets logged but not crash in production), the out-of-order mount instructions could lead to illegal view state and make the surface unusable, like what's shown here: {F820669000} The diff fixes this issue by removing the `DELETE` [conflict animation](https://fburl.com/code/5ctckvz3) keyframe, as well as the `CREATE` [immediate mutations](https://fburl.com/code/txyomytd) from the layout animation. The Fabric mounting layer assumes no combination of `DELETE...CREATE` in the same frame from differ + [layout animation overrides](https://fburl.com/code/zn17uqch). Reviewed By: sammy-SC Differential Revision: D41895427 fbshipit-source-id: d6df02663ba707af6db4a63a325ac776ca54d18e --- .../react/config/ReactFeatureFlags.java | 6 +++ .../LayoutAnimationKeyFrameManager.cpp | 39 +++++++++++++++++++ .../LayoutAnimationKeyFrameManager.h | 3 ++ .../react/renderer/scheduler/Scheduler.cpp | 17 +++++--- .../react/renderer/scheduler/Scheduler.h | 1 + .../uimanager/UIManagerAnimationDelegate.h | 8 ++++ 6 files changed, 68 insertions(+), 6 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 40ff4f4fba3..843bc6acc2d 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java +++ b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java @@ -109,4 +109,10 @@ public class ReactFeatureFlags { * border. */ public static boolean enableCloseVisibleGapBetweenPaths = true; + + /** + * Allow fix in layout animation to drop delete...create mutations which could cause missing view + * state in Fabric SurfaceMountingManager. + */ + public static boolean reduceDeleteCreateMutationLayoutAnimation = true; } diff --git a/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp b/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp index b4caab94d44..2d16a00596a 100644 --- a/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp +++ b/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp @@ -144,6 +144,11 @@ void LayoutAnimationKeyFrameManager::setComponentDescriptorRegistry( componentDescriptorRegistry_ = componentDescriptorRegistry; } +void LayoutAnimationKeyFrameManager::setReduceDeleteCreateMutation( + const bool reduceDeleteCreateMutation) { + reduceDeleteCreateMutation_ = reduceDeleteCreateMutation; +} + bool LayoutAnimationKeyFrameManager::shouldAnimateFrame() const { std::lock_guard lock(currentAnimationMutex_); return currentAnimation_ || !inflightAnimations_.empty(); @@ -732,6 +737,40 @@ LayoutAnimationKeyFrameManager::pullTransaction( auto finalConflictingMutations = ShadowViewMutationList{}; for (auto &keyFrame : conflictingAnimations) { + // Special-case: if the next conflicting animation contain "delete", + // while the final mutation has the same tag with "create", we should + // remove both the delete and create as they have no effect when + // combined in the same frame. The Fabric mount layer assumes no such + // combinations in the final mutations either. + if (reduceDeleteCreateMutation_) { + for (auto itMutation = immediateMutations.begin(); + itMutation != immediateMutations.end();) { + auto &mutation = *itMutation; + bool hasCreateMutationDeletedWithSameTag = false; + if (mutation.newChildShadowView.tag == keyFrame.tag && + mutation.type == ShadowViewMutation::Create) { + for (auto itKeyFrame = keyFrame.finalMutationsForKeyFrame.begin(); + itKeyFrame != keyFrame.finalMutationsForKeyFrame.end();) { + auto &conflictFinalMutation = *itKeyFrame; + if (conflictFinalMutation.type == ShadowViewMutation::Delete) { + itKeyFrame = + keyFrame.finalMutationsForKeyFrame.erase(itKeyFrame); + hasCreateMutationDeletedWithSameTag = true; + break; + } else { + itKeyFrame++; + } + } + } + + if (hasCreateMutationDeletedWithSameTag) { + itMutation = immediateMutations.erase(itMutation); + } else { + itMutation++; + } + } + } + // Special-case: if we have some (1) ongoing UPDATE animation, // (2) it conflicted with a new MOVE operation (REMOVE+INSERT) // without another corresponding UPDATE, we should re-queue the diff --git a/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.h b/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.h index 56d283a3f22..5bfea6f1b6e 100644 --- a/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.h +++ b/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.h @@ -56,6 +56,8 @@ class LayoutAnimationKeyFrameManager : public UIManagerAnimationDelegate, void setComponentDescriptorRegistry(SharedComponentDescriptorRegistry const & componentDescriptorRegistry) override; + void setReduceDeleteCreateMutation(bool reduceDeleteCreateMutation) override; + // TODO: add SurfaceId to this API as well bool shouldAnimateFrame() const override; @@ -151,6 +153,7 @@ class LayoutAnimationKeyFrameManager : public UIManagerAnimationDelegate, mutable std::mutex surfaceIdsToStopMutex_; mutable butter::set surfaceIdsToStop_{}; bool skipInvalidatedKeyFrames_{false}; + bool reduceDeleteCreateMutation_{false}; /* * Feature flag that forces a crash if component descriptor for shadow view diff --git a/ReactCommon/react/renderer/scheduler/Scheduler.cpp b/ReactCommon/react/renderer/scheduler/Scheduler.cpp index b9746e605fc..787b9733807 100644 --- a/ReactCommon/react/renderer/scheduler/Scheduler.cpp +++ b/ReactCommon/react/renderer/scheduler/Scheduler.cpp @@ -118,18 +118,23 @@ Scheduler::Scheduler( uiManager->registerCommitHook(*commitHook); } - if (animationDelegate != nullptr) { - animationDelegate->setComponentDescriptorRegistry( - componentDescriptorRegistry_); - } - uiManager_->setAnimationDelegate(animationDelegate); - #ifdef ANDROID removeOutstandingSurfacesOnDestruction_ = true; + reduceDeleteCreateMutationLayoutAnimation_ = reactNativeConfig_->getBool( + "react_fabric:reduce_delete_create_mutation_layout_animation_android"); #else removeOutstandingSurfacesOnDestruction_ = reactNativeConfig_->getBool( "react_fabric:remove_outstanding_surfaces_on_destruction_ios"); + reduceDeleteCreateMutationLayoutAnimation_ = true; #endif + + if (animationDelegate != nullptr) { + animationDelegate->setComponentDescriptorRegistry( + componentDescriptorRegistry_); + animationDelegate->setReduceDeleteCreateMutation( + reduceDeleteCreateMutationLayoutAnimation_); + } + uiManager_->setAnimationDelegate(animationDelegate); } Scheduler::~Scheduler() { diff --git a/ReactCommon/react/renderer/scheduler/Scheduler.h b/ReactCommon/react/renderer/scheduler/Scheduler.h index 23199d4bff5..466dd8b9e40 100644 --- a/ReactCommon/react/renderer/scheduler/Scheduler.h +++ b/ReactCommon/react/renderer/scheduler/Scheduler.h @@ -146,6 +146,7 @@ class Scheduler final : public UIManagerDelegate { * Temporary flags. */ bool removeOutstandingSurfacesOnDestruction_{false}; + bool reduceDeleteCreateMutationLayoutAnimation_{false}; }; } // namespace react diff --git a/ReactCommon/react/renderer/uimanager/UIManagerAnimationDelegate.h b/ReactCommon/react/renderer/uimanager/UIManagerAnimationDelegate.h index d20f43c2350..780346cb6cb 100644 --- a/ReactCommon/react/renderer/uimanager/UIManagerAnimationDelegate.h +++ b/ReactCommon/react/renderer/uimanager/UIManagerAnimationDelegate.h @@ -36,6 +36,14 @@ class UIManagerAnimationDelegate { virtual void setComponentDescriptorRegistry( const SharedComponentDescriptorRegistry &componentDescriptorRegistry) = 0; + /** + * Set Animation flags for dropping delete and create mutations + * + * @param reduceDeleteCreateMutation + */ + virtual void setReduceDeleteCreateMutation( + bool reduceDeleteCreateMutation) = 0; + /** * Only needed on Android to drive animations. */