From 7ac5d48341d602ddf130fb74a578a8dddb1bd378 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Mon, 15 Mar 2021 13:48:07 -0700 Subject: [PATCH] Generalize "isVirtualView" logic to make debug asserts consistent with platform Summary: On Android we have the notion of "virtual views", which are defined consistently but the logic is scattered and duplicated throughout the codebase. The logic exists to mark nodes that exist in the ShadowTree, but not the View tree. We want to CREATE, UPDATE, and DELETE them on the platform, but not INSERT or REMOVE them. They basically exist as EventEmitter objects. The only issue with this is (1) duplicated code, which opened the possibility for inconsistent definition (2) StubViewTree did not account for virtualized views, which caused assert crashes in debug mode for certain LayoutAnimations on Android. By moving the definition to ShadowViewMutation and accounting for it in StubViewTree, asserts are correct and consistent on all platforms. This was not caught until recently, because, until recently, no asserts actually ran on Android. Changelog: [Internal] Reviewed By: sammy-SC Differential Revision: D27001199 fbshipit-source-id: eb29085317037ba8a286d7813bdd57095ad4746f --- .../com/facebook/react/fabric/jni/Binding.cpp | 3 +- .../LayoutAnimationKeyFrameManager.cpp | 31 ++--- .../LayoutAnimationKeyFrameManager.h | 2 - .../renderer/mounting/ShadowViewMutation.cpp | 16 +++ .../renderer/mounting/ShadowViewMutation.h | 8 ++ .../react/renderer/mounting/StubViewTree.cpp | 109 +++++++++--------- 6 files changed, 90 insertions(+), 79 deletions(-) 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 1e4a912384f..8ba327f1590 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 @@ -618,8 +618,7 @@ void Binding::schedulerDidFinishTransaction( auto &mutationType = mutation.type; auto &index = mutation.index; - bool isVirtual = newChildShadowView.layoutMetrics == EmptyLayoutMetrics && - oldChildShadowView.layoutMetrics == EmptyLayoutMetrics; + bool isVirtual = mutation.mutatedViewIsVirtual(); switch (mutationType) { case ShadowViewMutation::Create: { diff --git a/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp b/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp index bfe92a75dd1..4937bab9777 100644 --- a/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp +++ b/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp @@ -406,7 +406,7 @@ void LayoutAnimationKeyFrameManager:: isRemoveMutation || mutation.type == ShadowViewMutation::Type::Insert); // TODO: turn all of this into a lambda and share code? - if (mutatedViewIsVirtual(mutation)) { + if (mutation.mutatedViewIsVirtual()) { PrintMutationInstruction( "[IndexAdjustment] Not calling adjustImmediateMutationIndicesForDelayedMutations, is virtual, for:", mutation); @@ -457,7 +457,7 @@ void LayoutAnimationKeyFrameManager:: if (delayedMutation.type != ShadowViewMutation::Type::Remove) { continue; } - if (mutatedViewIsVirtual(delayedMutation)) { + if (delayedMutation.mutatedViewIsVirtual()) { continue; } if (delayedMutation.oldChildShadowView.tag == @@ -519,7 +519,7 @@ void LayoutAnimationKeyFrameManager::adjustDelayedMutationIndicesForMutation( bool isInsertMutation = mutation.type == ShadowViewMutation::Type::Insert; react_native_assert(isRemoveMutation || isInsertMutation); - if (mutatedViewIsVirtual(mutation)) { + if (mutation.mutatedViewIsVirtual()) { PrintMutationInstruction( "[IndexAdjustment] Not calling adjustDelayedMutationIndicesForMutation, is virtual, for:", mutation); @@ -573,7 +573,7 @@ void LayoutAnimationKeyFrameManager::adjustDelayedMutationIndicesForMutation( if (finalAnimationMutation.type != ShadowViewMutation::Type::Remove) { continue; } - if (mutatedViewIsVirtual(*animatedKeyFrame.finalMutationForKeyFrame)) { + if (animatedKeyFrame.finalMutationForKeyFrame->mutatedViewIsVirtual()) { continue; } @@ -674,8 +674,8 @@ LayoutAnimationKeyFrameManager::getAndEraseConflictingAnimations( // for example, "insert" mutations where the final update needs to set // opacity to "1", even if there's no final ShadowNode update. if (!(animatedKeyFrame.finalMutationForKeyFrame.has_value() && - mutatedViewIsVirtual( - *animatedKeyFrame.finalMutationForKeyFrame))) { + animatedKeyFrame.finalMutationForKeyFrame + ->mutatedViewIsVirtual())) { conflictingAnimations.push_back(animatedKeyFrame); } @@ -777,7 +777,7 @@ LayoutAnimationKeyFrameManager::pullTransaction( continue; } if (keyframe.finalMutationForKeyFrame && - !mutatedViewIsVirtual(*keyframe.finalMutationForKeyFrame)) { + !keyframe.finalMutationForKeyFrame->mutatedViewIsVirtual()) { std::string msg = "Animation " + std::to_string(i) + " keyframe " + std::to_string(j) + ": Final Animation"; PrintMutationInstruction(msg, *keyframe.finalMutationForKeyFrame); @@ -1594,7 +1594,7 @@ LayoutAnimationKeyFrameManager::pullTransaction( continue; } if (keyframe.finalMutationForKeyFrame && - !mutatedViewIsVirtual(*keyframe.finalMutationForKeyFrame)) { + !keyframe.finalMutationForKeyFrame->mutatedViewIsVirtual()) { std::string msg = "Animation " + std::to_string(i) + " keyframe " + std::to_string(j) + ": Final Animation"; PrintMutationInstruction(msg, *keyframe.finalMutationForKeyFrame); @@ -1626,21 +1626,6 @@ LayoutAnimationKeyFrameManager::pullTransaction( surfaceId, transactionNumber, std::move(mutations), telemetry}; } -bool LayoutAnimationKeyFrameManager::mutatedViewIsVirtual( - ShadowViewMutation const &mutation) const { - bool viewIsVirtual = false; - - // TODO: extract this into an Android platform-specific class? - // Explanation: for "Insert" mutations, oldChildShadowView is always empty. - // for "Remove" mutations, newChildShadowView is always empty. -#ifdef ANDROID - viewIsVirtual = - mutation.newChildShadowView.layoutMetrics == EmptyLayoutMetrics && - mutation.oldChildShadowView.layoutMetrics == EmptyLayoutMetrics; -#endif - return viewIsVirtual; -} - bool LayoutAnimationKeyFrameManager::hasComponentDescriptorForShadowView( ShadowView const &shadowView) const { return componentDescriptorRegistry_->hasComponentDescriptorAt( diff --git a/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.h b/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.h index 83c3014ea8f..2186c2431a4 100644 --- a/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.h +++ b/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.h @@ -226,8 +226,6 @@ class LayoutAnimationKeyFrameManager : public UIManagerAnimationDelegate, mutable std::vector surfaceIdsToStop_{}; protected: - bool mutatedViewIsVirtual(ShadowViewMutation const &mutation) const; - bool hasComponentDescriptorForShadowView(ShadowView const &shadowView) const; ComponentDescriptor const &getComponentDescriptorForShadowView( ShadowView const &shadowView) const; diff --git a/ReactCommon/react/renderer/mounting/ShadowViewMutation.cpp b/ReactCommon/react/renderer/mounting/ShadowViewMutation.cpp index dbf4afd227c..9f0c61d2746 100644 --- a/ReactCommon/react/renderer/mounting/ShadowViewMutation.cpp +++ b/ReactCommon/react/renderer/mounting/ShadowViewMutation.cpp @@ -68,6 +68,22 @@ ShadowViewMutation ShadowViewMutation::UpdateMutation( }; } +bool ShadowViewMutation::mutatedViewIsVirtual() const { + bool viewIsVirtual = false; + +#ifdef ANDROID + // Explanation: Even for non-virtual views, + // for "Insert" mutations, oldChildShadowView is always empty. + // for "Remove" mutations, newChildShadowView is always empty. + // Thus, to see if a view is virtual, we need to always check both the old and + // new View. + viewIsVirtual = newChildShadowView.layoutMetrics == EmptyLayoutMetrics && + oldChildShadowView.layoutMetrics == EmptyLayoutMetrics; +#endif + + return viewIsVirtual; +} + #if RN_DEBUG_STRING_CONVERTIBLE std::string getDebugName(ShadowViewMutation const &mutation) { diff --git a/ReactCommon/react/renderer/mounting/ShadowViewMutation.h b/ReactCommon/react/renderer/mounting/ShadowViewMutation.h index 3bd0067d5fd..6eb7f8f1d3a 100644 --- a/ReactCommon/react/renderer/mounting/ShadowViewMutation.h +++ b/ReactCommon/react/renderer/mounting/ShadowViewMutation.h @@ -69,6 +69,14 @@ struct ShadowViewMutation final { ShadowView oldChildShadowView = {}; ShadowView newChildShadowView = {}; int index = -1; + + public: + // Some platforms can have the notion of virtual views - views that are in the + // ShadowTree hierarchy but never are on the platform. Generally this is used + // so notify the platform that a view exists so that we can keep EventEmitters + // around, to notify JS of something. This mechanism is DEPRECATED and it is + // highly recommended that you NOT make use of this in your platform! + bool mutatedViewIsVirtual() const; }; using ShadowViewMutationList = std::vector; diff --git a/ReactCommon/react/renderer/mounting/StubViewTree.cpp b/ReactCommon/react/renderer/mounting/StubViewTree.cpp index cdcae7e1c28..577e88e366b 100644 --- a/ReactCommon/react/renderer/mounting/StubViewTree.cpp +++ b/ReactCommon/react/renderer/mounting/StubViewTree.cpp @@ -73,62 +73,67 @@ void StubViewTree::mutate(ShadowViewMutationList const &mutations) { } case ShadowViewMutation::Insert: { - react_native_assert(mutation.oldChildShadowView == ShadowView{}); - auto parentTag = mutation.parentShadowView.tag; - react_native_assert(registry.find(parentTag) != registry.end()); - auto parentStubView = registry[parentTag]; - auto childTag = mutation.newChildShadowView.tag; - react_native_assert(registry.find(childTag) != registry.end()); - auto childStubView = registry[childTag]; - react_native_assert(childStubView->parentTag == NO_VIEW_TAG); - childStubView->update(mutation.newChildShadowView); - STUB_VIEW_LOG({ - LOG(ERROR) << "StubView: Insert [" << childTag << "] into [" - << parentTag << "] @" << mutation.index << "(" - << parentStubView->children.size() << " children)"; - }); - react_native_assert(parentStubView->children.size() >= mutation.index); - childStubView->parentTag = parentTag; - parentStubView->children.insert( - parentStubView->children.begin() + mutation.index, childStubView); + if (!mutation.mutatedViewIsVirtual()) { + react_native_assert(mutation.oldChildShadowView == ShadowView{}); + auto parentTag = mutation.parentShadowView.tag; + react_native_assert(registry.find(parentTag) != registry.end()); + auto parentStubView = registry[parentTag]; + auto childTag = mutation.newChildShadowView.tag; + react_native_assert(registry.find(childTag) != registry.end()); + auto childStubView = registry[childTag]; + react_native_assert(childStubView->parentTag == NO_VIEW_TAG); + childStubView->update(mutation.newChildShadowView); + STUB_VIEW_LOG({ + LOG(ERROR) << "StubView: Insert [" << childTag << "] into [" + << parentTag << "] @" << mutation.index << "(" + << parentStubView->children.size() << " children)"; + }); + react_native_assert( + parentStubView->children.size() >= mutation.index); + childStubView->parentTag = parentTag; + parentStubView->children.insert( + parentStubView->children.begin() + mutation.index, childStubView); + } break; } case ShadowViewMutation::Remove: { - react_native_assert(mutation.newChildShadowView == ShadowView{}); - auto parentTag = mutation.parentShadowView.tag; - react_native_assert(registry.find(parentTag) != registry.end()); - auto parentStubView = registry[parentTag]; - auto childTag = mutation.oldChildShadowView.tag; - STUB_VIEW_LOG({ - LOG(ERROR) << "StubView: Remove [" << childTag << "] from [" - << parentTag << "] @" << mutation.index << " with " - << parentStubView->children.size() << " children"; - }); - react_native_assert(parentStubView->children.size() > mutation.index); - react_native_assert(registry.find(childTag) != registry.end()); - auto childStubView = registry[childTag]; - react_native_assert(childStubView->parentTag == parentTag); - STUB_VIEW_LOG({ - std::string strChildList = ""; - int i = 0; - for (auto const &child : parentStubView->children) { - strChildList.append(std::to_string(i)); - strChildList.append(":"); - strChildList.append(std::to_string(child->tag)); - strChildList.append(", "); - i++; - } - LOG(ERROR) << "StubView: BEFORE REMOVE: Children of " << parentTag - << ": " << strChildList; - }); - react_native_assert( - parentStubView->children.size() > mutation.index && - parentStubView->children[mutation.index]->tag == - childStubView->tag); - childStubView->parentTag = NO_VIEW_TAG; - parentStubView->children.erase( - parentStubView->children.begin() + mutation.index); + if (!mutation.mutatedViewIsVirtual()) { + react_native_assert(mutation.newChildShadowView == ShadowView{}); + auto parentTag = mutation.parentShadowView.tag; + react_native_assert(registry.find(parentTag) != registry.end()); + auto parentStubView = registry[parentTag]; + auto childTag = mutation.oldChildShadowView.tag; + STUB_VIEW_LOG({ + LOG(ERROR) << "StubView: Remove [" << childTag << "] from [" + << parentTag << "] @" << mutation.index << " with " + << parentStubView->children.size() << " children"; + }); + react_native_assert(parentStubView->children.size() > mutation.index); + react_native_assert(registry.find(childTag) != registry.end()); + auto childStubView = registry[childTag]; + react_native_assert(childStubView->parentTag == parentTag); + STUB_VIEW_LOG({ + std::string strChildList = ""; + int i = 0; + for (auto const &child : parentStubView->children) { + strChildList.append(std::to_string(i)); + strChildList.append(":"); + strChildList.append(std::to_string(child->tag)); + strChildList.append(", "); + i++; + } + LOG(ERROR) << "StubView: BEFORE REMOVE: Children of " << parentTag + << ": " << strChildList; + }); + react_native_assert( + parentStubView->children.size() > mutation.index && + parentStubView->children[mutation.index]->tag == + childStubView->tag); + childStubView->parentTag = NO_VIEW_TAG; + parentStubView->children.erase( + parentStubView->children.begin() + mutation.index); + } break; }