Skip to content

Commit a1e9abb

Browse files
joevilchesfacebook-github-bot
joevilches
authored and
facebook-github-bot
committed
Fix case where absolute nodes would sometimes not be cloned (#1675)
Summary: X-link: facebook/react-native#45240 Pull Request resolved: #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
1 parent b9e335e commit a1e9abb

File tree

4 files changed

+160
-14
lines changed

4 files changed

+160
-14
lines changed

‎tests/YGCloneNodeTest.cpp

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
/*
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*/
7+
8+
#include <gtest/gtest.h>
9+
#include <yoga/Yoga.h>
10+
11+
static void recursivelyAssertProperNodeOwnership(YGNodeRef node) {
12+
for (size_t i = 0; i < YGNodeGetChildCount(node); ++i) {
13+
const auto child = YGNodeGetChild(node, i);
14+
ASSERT_EQ(node, YGNodeGetOwner(child));
15+
recursivelyAssertProperNodeOwnership(child);
16+
}
17+
}
18+
19+
TEST(YogaTest, absolute_node_cloned_with_static_parent) {
20+
YGNodeRef root = YGNodeNew();
21+
YGNodeStyleSetWidth(root, 100);
22+
YGNodeStyleSetHeight(root, 100);
23+
24+
YGNodeRef root_child0 = YGNodeNew();
25+
YGNodeStyleSetPositionType(root_child0, YGPositionTypeStatic);
26+
YGNodeStyleSetWidth(root_child0, 10);
27+
YGNodeStyleSetHeight(root_child0, 10);
28+
YGNodeInsertChild(root, root_child0, 0);
29+
30+
YGNodeRef root_child0_child0 = YGNodeNew();
31+
YGNodeStyleSetPositionType(root_child0_child0, YGPositionTypeAbsolute);
32+
YGNodeStyleSetWidthPercent(root_child0_child0, 1);
33+
YGNodeStyleSetHeight(root_child0_child0, 1);
34+
YGNodeInsertChild(root_child0, root_child0_child0, 0);
35+
36+
YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);
37+
38+
YGNodeRef clonedRoot = YGNodeClone(root);
39+
YGNodeStyleSetWidth(clonedRoot, 110);
40+
YGNodeCalculateLayout(clonedRoot, YGUndefined, YGUndefined, YGDirectionLTR);
41+
42+
recursivelyAssertProperNodeOwnership(clonedRoot);
43+
44+
YGNodeFreeRecursive(root);
45+
YGNodeFreeRecursive(clonedRoot);
46+
}
47+
48+
TEST(YogaTest, absolute_node_cloned_with_static_ancestors) {
49+
YGNodeRef root = YGNodeNew();
50+
YGNodeStyleSetWidth(root, 100);
51+
YGNodeStyleSetHeight(root, 100);
52+
53+
YGNodeRef root_child0 = YGNodeNew();
54+
YGNodeStyleSetPositionType(root_child0, YGPositionTypeStatic);
55+
YGNodeStyleSetWidth(root_child0, 50);
56+
YGNodeStyleSetHeight(root_child0, 50);
57+
YGNodeInsertChild(root, root_child0, 0);
58+
59+
YGNodeRef root_child0_child0 = YGNodeNew();
60+
YGNodeStyleSetPositionType(root_child0_child0, YGPositionTypeStatic);
61+
YGNodeStyleSetWidth(root_child0_child0, 40);
62+
YGNodeStyleSetHeight(root_child0_child0, 40);
63+
YGNodeInsertChild(root_child0, root_child0_child0, 0);
64+
65+
YGNodeRef root_child0_child0_child0 = YGNodeNew();
66+
YGNodeStyleSetPositionType(root_child0_child0_child0, YGPositionTypeStatic);
67+
YGNodeStyleSetWidth(root_child0_child0_child0, 30);
68+
YGNodeStyleSetHeight(root_child0_child0_child0, 30);
69+
YGNodeInsertChild(root_child0_child0, root_child0_child0_child0, 0);
70+
71+
YGNodeRef root_child0_child0_child0_child0 = YGNodeNew();
72+
YGNodeStyleSetPositionType(
73+
root_child0_child0_child0_child0, YGPositionTypeAbsolute);
74+
YGNodeStyleSetWidthPercent(root_child0_child0_child0_child0, 1);
75+
YGNodeStyleSetHeight(root_child0_child0_child0_child0, 1);
76+
YGNodeInsertChild(
77+
root_child0_child0_child0, root_child0_child0_child0_child0, 0);
78+
79+
YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);
80+
81+
YGNodeRef clonedRoot = YGNodeClone(root);
82+
YGNodeStyleSetWidth(clonedRoot, 110);
83+
YGNodeCalculateLayout(clonedRoot, YGUndefined, YGUndefined, YGDirectionLTR);
84+
85+
recursivelyAssertProperNodeOwnership(clonedRoot);
86+
87+
YGNodeFreeRecursive(root);
88+
YGNodeFreeRecursive(clonedRoot);
89+
}

‎tests/YGRelayoutTest.cpp

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -207,3 +207,43 @@ TEST(YogaTest, relayout_containing_block_size_changes) {
207207

208208
YGConfigFree(config);
209209
}
210+
211+
TEST(YogaTest, has_new_layout_flag_set_static) {
212+
YGNodeRef root = YGNodeNew();
213+
YGNodeStyleSetWidth(root, 100);
214+
YGNodeStyleSetHeight(root, 100);
215+
216+
YGNodeRef root_child0 = YGNodeNew();
217+
YGNodeStyleSetPositionType(root_child0, YGPositionTypeStatic);
218+
YGNodeStyleSetWidth(root_child0, 10);
219+
YGNodeStyleSetHeight(root_child0, 10);
220+
YGNodeInsertChild(root, root_child0, 0);
221+
222+
YGNodeRef root_child0_child0 = YGNodeNew();
223+
YGNodeStyleSetPositionType(root_child0_child0, YGPositionTypeStatic);
224+
YGNodeStyleSetWidth(root_child0_child0, 5);
225+
YGNodeStyleSetHeight(root_child0_child0, 5);
226+
YGNodeInsertChild(root_child0, root_child0_child0, 0);
227+
228+
YGNodeRef root_child0_child0_child0 = YGNodeNew();
229+
YGNodeStyleSetPositionType(root_child0_child0_child0, YGPositionTypeAbsolute);
230+
YGNodeStyleSetWidthPercent(root_child0_child0_child0, 1);
231+
YGNodeStyleSetHeight(root_child0_child0_child0, 1);
232+
YGNodeInsertChild(root_child0_child0, root_child0_child0_child0, 0);
233+
234+
YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);
235+
YGNodeSetHasNewLayout(root, false);
236+
YGNodeSetHasNewLayout(root_child0, false);
237+
YGNodeSetHasNewLayout(root_child0_child0, false);
238+
YGNodeSetHasNewLayout(root_child0_child0_child0, false);
239+
240+
YGNodeStyleSetWidth(root, 110);
241+
YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);
242+
243+
ASSERT_TRUE(YGNodeGetHasNewLayout(root));
244+
ASSERT_TRUE(YGNodeGetHasNewLayout(root_child0));
245+
ASSERT_TRUE(YGNodeGetHasNewLayout(root_child0_child0));
246+
ASSERT_TRUE(YGNodeGetHasNewLayout(root_child0_child0_child0));
247+
248+
YGNodeFreeRecursive(root);
249+
}

‎yoga/algorithm/AbsoluteLayout.cpp

Lines changed: 29 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -474,7 +474,7 @@ void layoutAbsoluteChild(
474474
containingBlockHeight);
475475
}
476476

477-
void layoutAbsoluteDescendants(
477+
bool layoutAbsoluteDescendants(
478478
yoga::Node* containingNode,
479479
yoga::Node* currentNode,
480480
SizingMode widthSizingMode,
@@ -486,6 +486,7 @@ void layoutAbsoluteDescendants(
486486
float currentNodeTopOffsetFromContainingBlock,
487487
float containingNodeAvailableInnerWidth,
488488
float containingNodeAvailableInnerHeight) {
489+
bool hasNewLayout = false;
489490
for (auto child : currentNode->getChildren()) {
490491
if (child->style().display() == Display::None) {
491492
continue;
@@ -514,6 +515,8 @@ void layoutAbsoluteDescendants(
514515
currentDepth,
515516
generationCount);
516517

518+
hasNewLayout = hasNewLayout || child->getHasNewLayout();
519+
517520
/*
518521
* At this point the child has its position set but only on its the
519522
* parent's flexStart edge. Additionally, this position should be
@@ -571,6 +574,13 @@ void layoutAbsoluteDescendants(
571574
} else if (
572575
child->style().positionType() == PositionType::Static &&
573576
!child->alwaysFormsContainingBlock()) {
577+
// We may write new layout results for absolute descendants of "child"
578+
// which are positioned relative to the current containing block instead
579+
// of their parent. "child" may not be dirty, or have new constraints, so
580+
// absolute positioning may be the first time during this layout pass that
581+
// we need to mutate these descendents. Make sure the path of
582+
// nodes to them is mutable before positioning.
583+
child->cloneChildrenIfNeeded();
574584
const Direction childDirection =
575585
child->resolveDirection(currentNodeDirection);
576586
// By now all descendants of the containing block that are not absolute
@@ -582,19 +592,25 @@ void layoutAbsoluteDescendants(
582592
currentNodeTopOffsetFromContainingBlock +
583593
child->getLayout().position(PhysicalEdge::Top);
584594

585-
layoutAbsoluteDescendants(
586-
containingNode,
587-
child,
588-
widthSizingMode,
589-
childDirection,
590-
layoutMarkerData,
591-
currentDepth + 1,
592-
generationCount,
593-
childLeftOffsetFromContainingBlock,
594-
childTopOffsetFromContainingBlock,
595-
containingNodeAvailableInnerWidth,
596-
containingNodeAvailableInnerHeight);
595+
hasNewLayout = hasNewLayout ||
596+
layoutAbsoluteDescendants(
597+
containingNode,
598+
child,
599+
widthSizingMode,
600+
childDirection,
601+
layoutMarkerData,
602+
currentDepth + 1,
603+
generationCount,
604+
childLeftOffsetFromContainingBlock,
605+
childTopOffsetFromContainingBlock,
606+
containingNodeAvailableInnerWidth,
607+
containingNodeAvailableInnerHeight);
608+
609+
if (hasNewLayout) {
610+
child->setHasNewLayout(hasNewLayout);
611+
}
597612
}
598613
}
614+
return hasNewLayout;
599615
}
600616
} // namespace facebook::yoga

‎yoga/algorithm/AbsoluteLayout.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,8 @@ void layoutAbsoluteChild(
2424
uint32_t depth,
2525
uint32_t generationCount);
2626

27-
void layoutAbsoluteDescendants(
27+
// Returns if some absolute descendant has new layout
28+
bool layoutAbsoluteDescendants(
2829
yoga::Node* containingNode,
2930
yoga::Node* currentNode,
3031
SizingMode widthSizingMode,

0 commit comments

Comments
 (0)