From 40c188129d5d404bc7bfb2555fc8dcb2f83987b8 Mon Sep 17 00:00:00 2001 From: Joe Vilches Date: Tue, 2 Jul 2024 11:12:42 -0700 Subject: [PATCH] Fix case where absolute nodes would sometimes not be cloned (#1675) Summary: X-link: https://github.com/facebook/react-native/pull/45240 Pull Request resolved: https://github.com/facebook/yoga/pull/1675 There was a bug where some crash would happen if a tree was cloned that had static/absolute parent/child pair inside it. This was because we were no longer calling `cloneChildrenIfNeeded` on the static parent, but would still layout the absolute child. So that child's owner would be stale and have new layout. In React Native this would lead to a failed assert which causes the crash. The fix here is to clone the children of static nodes during `layoutAbsoluteDescendants` so that we guarantee the node is either cloned if it is going to have new layout. Changelog: [Internal] Reviewed By: NickGerleman Differential Revision: D59175629 --- tests/YGCloneNodeTest.cpp | 89 +++++++++++++++++++++++++++++++ tests/YGRelayoutTest.cpp | 40 ++++++++++++++ yoga/algorithm/AbsoluteLayout.cpp | 42 ++++++++++----- yoga/algorithm/AbsoluteLayout.h | 3 +- 4 files changed, 160 insertions(+), 14 deletions(-) create mode 100644 tests/YGCloneNodeTest.cpp diff --git a/tests/YGCloneNodeTest.cpp b/tests/YGCloneNodeTest.cpp new file mode 100644 index 0000000000..d029fbe027 --- /dev/null +++ b/tests/YGCloneNodeTest.cpp @@ -0,0 +1,89 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#include +#include + +static void recursivelyAssertProperNodeOwnership(YGNodeRef node) { + for (size_t i = 0; i < YGNodeGetChildCount(node); ++i) { + const auto child = YGNodeGetChild(node, i); + ASSERT_EQ(node, YGNodeGetOwner(child)); + recursivelyAssertProperNodeOwnership(child); + } +} + +TEST(YogaTest, absolute_node_cloned_with_static_parent) { + YGNodeRef root = YGNodeNew(); + YGNodeStyleSetWidth(root, 100); + YGNodeStyleSetHeight(root, 100); + + YGNodeRef root_child0 = YGNodeNew(); + YGNodeStyleSetPositionType(root_child0, YGPositionTypeStatic); + YGNodeStyleSetWidth(root_child0, 10); + YGNodeStyleSetHeight(root_child0, 10); + YGNodeInsertChild(root, root_child0, 0); + + YGNodeRef root_child0_child0 = YGNodeNew(); + YGNodeStyleSetPositionType(root_child0_child0, YGPositionTypeAbsolute); + YGNodeStyleSetWidthPercent(root_child0_child0, 1); + YGNodeStyleSetHeight(root_child0_child0, 1); + YGNodeInsertChild(root_child0, root_child0_child0, 0); + + YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR); + + YGNodeRef clonedRoot = YGNodeClone(root); + YGNodeStyleSetWidth(clonedRoot, 110); + YGNodeCalculateLayout(clonedRoot, YGUndefined, YGUndefined, YGDirectionLTR); + + recursivelyAssertProperNodeOwnership(clonedRoot); + + YGNodeFreeRecursive(root); + YGNodeFreeRecursive(clonedRoot); +} + +TEST(YogaTest, absolute_node_cloned_with_static_ancestors) { + YGNodeRef root = YGNodeNew(); + YGNodeStyleSetWidth(root, 100); + YGNodeStyleSetHeight(root, 100); + + YGNodeRef root_child0 = YGNodeNew(); + YGNodeStyleSetPositionType(root_child0, YGPositionTypeStatic); + YGNodeStyleSetWidth(root_child0, 50); + YGNodeStyleSetHeight(root_child0, 50); + YGNodeInsertChild(root, root_child0, 0); + + YGNodeRef root_child0_child0 = YGNodeNew(); + YGNodeStyleSetPositionType(root_child0_child0, YGPositionTypeStatic); + YGNodeStyleSetWidth(root_child0_child0, 40); + YGNodeStyleSetHeight(root_child0_child0, 40); + YGNodeInsertChild(root_child0, root_child0_child0, 0); + + YGNodeRef root_child0_child0_child0 = YGNodeNew(); + YGNodeStyleSetPositionType(root_child0_child0_child0, YGPositionTypeStatic); + YGNodeStyleSetWidth(root_child0_child0_child0, 30); + YGNodeStyleSetHeight(root_child0_child0_child0, 30); + YGNodeInsertChild(root_child0_child0, root_child0_child0_child0, 0); + + YGNodeRef root_child0_child0_child0_child0 = YGNodeNew(); + YGNodeStyleSetPositionType( + root_child0_child0_child0_child0, YGPositionTypeAbsolute); + YGNodeStyleSetWidthPercent(root_child0_child0_child0_child0, 1); + YGNodeStyleSetHeight(root_child0_child0_child0_child0, 1); + YGNodeInsertChild( + root_child0_child0_child0, root_child0_child0_child0_child0, 0); + + YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR); + + YGNodeRef clonedRoot = YGNodeClone(root); + YGNodeStyleSetWidth(clonedRoot, 110); + YGNodeCalculateLayout(clonedRoot, YGUndefined, YGUndefined, YGDirectionLTR); + + recursivelyAssertProperNodeOwnership(clonedRoot); + + YGNodeFreeRecursive(root); + YGNodeFreeRecursive(clonedRoot); +} diff --git a/tests/YGRelayoutTest.cpp b/tests/YGRelayoutTest.cpp index cb60a3d26c..cbdd066d79 100644 --- a/tests/YGRelayoutTest.cpp +++ b/tests/YGRelayoutTest.cpp @@ -207,3 +207,43 @@ TEST(YogaTest, relayout_containing_block_size_changes) { YGConfigFree(config); } + +TEST(YogaTest, has_new_layout_flag_set_static) { + YGNodeRef root = YGNodeNew(); + YGNodeStyleSetWidth(root, 100); + YGNodeStyleSetHeight(root, 100); + + YGNodeRef root_child0 = YGNodeNew(); + YGNodeStyleSetPositionType(root_child0, YGPositionTypeStatic); + YGNodeStyleSetWidth(root_child0, 10); + YGNodeStyleSetHeight(root_child0, 10); + YGNodeInsertChild(root, root_child0, 0); + + YGNodeRef root_child0_child0 = YGNodeNew(); + YGNodeStyleSetPositionType(root_child0_child0, YGPositionTypeStatic); + YGNodeStyleSetWidth(root_child0_child0, 5); + YGNodeStyleSetHeight(root_child0_child0, 5); + YGNodeInsertChild(root_child0, root_child0_child0, 0); + + YGNodeRef root_child0_child0_child0 = YGNodeNew(); + YGNodeStyleSetPositionType(root_child0_child0_child0, YGPositionTypeAbsolute); + YGNodeStyleSetWidthPercent(root_child0_child0_child0, 1); + YGNodeStyleSetHeight(root_child0_child0_child0, 1); + YGNodeInsertChild(root_child0_child0, root_child0_child0_child0, 0); + + YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR); + YGNodeSetHasNewLayout(root, false); + YGNodeSetHasNewLayout(root_child0, false); + YGNodeSetHasNewLayout(root_child0_child0, false); + YGNodeSetHasNewLayout(root_child0_child0_child0, false); + + YGNodeStyleSetWidth(root, 110); + YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR); + + ASSERT_TRUE(YGNodeGetHasNewLayout(root)); + ASSERT_TRUE(YGNodeGetHasNewLayout(root_child0)); + ASSERT_TRUE(YGNodeGetHasNewLayout(root_child0_child0)); + ASSERT_TRUE(YGNodeGetHasNewLayout(root_child0_child0_child0)); + + YGNodeFreeRecursive(root); +} diff --git a/yoga/algorithm/AbsoluteLayout.cpp b/yoga/algorithm/AbsoluteLayout.cpp index 81dd9128ad..d68b4fa91c 100644 --- a/yoga/algorithm/AbsoluteLayout.cpp +++ b/yoga/algorithm/AbsoluteLayout.cpp @@ -474,7 +474,7 @@ void layoutAbsoluteChild( containingBlockHeight); } -void layoutAbsoluteDescendants( +bool layoutAbsoluteDescendants( yoga::Node* containingNode, yoga::Node* currentNode, SizingMode widthSizingMode, @@ -486,6 +486,7 @@ void layoutAbsoluteDescendants( float currentNodeTopOffsetFromContainingBlock, float containingNodeAvailableInnerWidth, float containingNodeAvailableInnerHeight) { + bool hasNewLayout = false; for (auto child : currentNode->getChildren()) { if (child->style().display() == Display::None) { continue; @@ -514,6 +515,8 @@ void layoutAbsoluteDescendants( currentDepth, generationCount); + hasNewLayout = hasNewLayout || child->getHasNewLayout(); + /* * At this point the child has its position set but only on its the * parent's flexStart edge. Additionally, this position should be @@ -571,6 +574,13 @@ void layoutAbsoluteDescendants( } else if ( child->style().positionType() == PositionType::Static && !child->alwaysFormsContainingBlock()) { + // We may write new layout results for absolute descendants of "child" + // which are positioned relative to the current containing block instead + // of their parent. "child" may not be dirty, or have new constraints, so + // absolute positioning may be the first time during this layout pass that + // we need to mutate these descendents. Make sure the path of + // nodes to them is mutable before positioning. + child->cloneChildrenIfNeeded(); const Direction childDirection = child->resolveDirection(currentNodeDirection); // By now all descendants of the containing block that are not absolute @@ -582,19 +592,25 @@ void layoutAbsoluteDescendants( currentNodeTopOffsetFromContainingBlock + child->getLayout().position(PhysicalEdge::Top); - layoutAbsoluteDescendants( - containingNode, - child, - widthSizingMode, - childDirection, - layoutMarkerData, - currentDepth + 1, - generationCount, - childLeftOffsetFromContainingBlock, - childTopOffsetFromContainingBlock, - containingNodeAvailableInnerWidth, - containingNodeAvailableInnerHeight); + hasNewLayout = hasNewLayout || + layoutAbsoluteDescendants( + containingNode, + child, + widthSizingMode, + childDirection, + layoutMarkerData, + currentDepth + 1, + generationCount, + childLeftOffsetFromContainingBlock, + childTopOffsetFromContainingBlock, + containingNodeAvailableInnerWidth, + containingNodeAvailableInnerHeight); + + if (hasNewLayout) { + child->setHasNewLayout(hasNewLayout); + } } } + return hasNewLayout; } } // namespace facebook::yoga diff --git a/yoga/algorithm/AbsoluteLayout.h b/yoga/algorithm/AbsoluteLayout.h index ae84e6da9f..584a586a04 100644 --- a/yoga/algorithm/AbsoluteLayout.h +++ b/yoga/algorithm/AbsoluteLayout.h @@ -24,7 +24,8 @@ void layoutAbsoluteChild( uint32_t depth, uint32_t generationCount); -void layoutAbsoluteDescendants( +// Returns if some absolute descendant has new layout +bool layoutAbsoluteDescendants( yoga::Node* containingNode, yoga::Node* currentNode, SizingMode widthSizingMode,