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

fbshipit-source-id: 4d110a08ba5368704327d5ab69a8695b28e746f4
This commit is contained in:
Joe Vilches
2024-07-02 15:14:33 -07:00
committed by Facebook GitHub Bot
parent b9e335ebfd
commit a1e9abb9b3
4 changed files with 160 additions and 14 deletions

89
tests/YGCloneNodeTest.cpp Normal file
View File

@@ -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 <gtest/gtest.h>
#include <yoga/Yoga.h>
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);
}

View File

@@ -207,3 +207,43 @@ TEST(YogaTest, relayout_containing_block_size_changes) {
YGConfigFree(config); 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);
}

View File

@@ -474,7 +474,7 @@ void layoutAbsoluteChild(
containingBlockHeight); containingBlockHeight);
} }
void layoutAbsoluteDescendants( bool layoutAbsoluteDescendants(
yoga::Node* containingNode, yoga::Node* containingNode,
yoga::Node* currentNode, yoga::Node* currentNode,
SizingMode widthSizingMode, SizingMode widthSizingMode,
@@ -486,6 +486,7 @@ void layoutAbsoluteDescendants(
float currentNodeTopOffsetFromContainingBlock, float currentNodeTopOffsetFromContainingBlock,
float containingNodeAvailableInnerWidth, float containingNodeAvailableInnerWidth,
float containingNodeAvailableInnerHeight) { float containingNodeAvailableInnerHeight) {
bool hasNewLayout = false;
for (auto child : currentNode->getChildren()) { for (auto child : currentNode->getChildren()) {
if (child->style().display() == Display::None) { if (child->style().display() == Display::None) {
continue; continue;
@@ -514,6 +515,8 @@ void layoutAbsoluteDescendants(
currentDepth, currentDepth,
generationCount); generationCount);
hasNewLayout = hasNewLayout || child->getHasNewLayout();
/* /*
* At this point the child has its position set but only on its the * At this point the child has its position set but only on its the
* parent's flexStart edge. Additionally, this position should be * parent's flexStart edge. Additionally, this position should be
@@ -571,6 +574,13 @@ void layoutAbsoluteDescendants(
} else if ( } else if (
child->style().positionType() == PositionType::Static && child->style().positionType() == PositionType::Static &&
!child->alwaysFormsContainingBlock()) { !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 = const Direction childDirection =
child->resolveDirection(currentNodeDirection); child->resolveDirection(currentNodeDirection);
// By now all descendants of the containing block that are not absolute // By now all descendants of the containing block that are not absolute
@@ -582,19 +592,25 @@ void layoutAbsoluteDescendants(
currentNodeTopOffsetFromContainingBlock + currentNodeTopOffsetFromContainingBlock +
child->getLayout().position(PhysicalEdge::Top); child->getLayout().position(PhysicalEdge::Top);
layoutAbsoluteDescendants( hasNewLayout = hasNewLayout ||
containingNode, layoutAbsoluteDescendants(
child, containingNode,
widthSizingMode, child,
childDirection, widthSizingMode,
layoutMarkerData, childDirection,
currentDepth + 1, layoutMarkerData,
generationCount, currentDepth + 1,
childLeftOffsetFromContainingBlock, generationCount,
childTopOffsetFromContainingBlock, childLeftOffsetFromContainingBlock,
containingNodeAvailableInnerWidth, childTopOffsetFromContainingBlock,
containingNodeAvailableInnerHeight); containingNodeAvailableInnerWidth,
containingNodeAvailableInnerHeight);
if (hasNewLayout) {
child->setHasNewLayout(hasNewLayout);
}
} }
} }
return hasNewLayout;
} }
} // namespace facebook::yoga } // namespace facebook::yoga

View File

@@ -24,7 +24,8 @@ void layoutAbsoluteChild(
uint32_t depth, uint32_t depth,
uint32_t generationCount); uint32_t generationCount);
void layoutAbsoluteDescendants( // Returns if some absolute descendant has new layout
bool layoutAbsoluteDescendants(
yoga::Node* containingNode, yoga::Node* containingNode,
yoga::Node* currentNode, yoga::Node* currentNode,
SizingMode widthSizingMode, SizingMode widthSizingMode,