Skip to content

Commit 8121dac

Browse files
committed
Fix display: contents nodes having hasNewLayout set incorrectly (#57103)
Summary: Pull Request resolved: #57103 Changelog: [General][Fixed] Fixed `display: contents` nodes having `hasNewLayout` set incorrectly `cleanupContentsNodesRecursively` unconditionally sets `hasNewLayout=true` on `display: contents` children, including on code paths where their parent's layout was not actually performed in this pass. The stale flag can survive across layout passes and, in clone-on-write renderers (e.g. React Native Fabric), be observed by a subsequent pass whose parent was cloned but whose layout was served from cache, leaving the contents child's owner pointing at the previous parent revision. There are two paths through which the cleanup could stamp a contents child whose parent's `hasNewLayout` would end up false: 1. Measure-phase visit. Inside `calculateLayoutImpl`, the cleanup ran with no knowledge of `performLayout`. When the parent's `calculateLayoutImpl` was invoked only with `performLayout=false` (cache miss on measure, cache hit on layout), the cleanup stamped contents children even though the parent itself never had its `hasNewLayout` set. 2. Absolute-layout walk. `layoutAbsoluteDescendants` walks every static layout descendant of the containing block - including ones whose own `calculateLayoutImpl` was skipped via the layout-phase cache. The cleanup invoked along that walk unconditionally stamped contents children, but the parent's `hasNewLayout` was only updated when the recursion actually found new layout downstream. In both cases, the result is the same invariant violation: a contents node with `hasNewLayout=true` whose parent has `hasNewLayout=false`. A consumer iterating the tree via `hasNewLayout` skips the parent and never clears the stale flag. X-link: react/yoga#1970 Test Plan: Added `YGContentsNodeHasNewLayoutTest.cpp` with regression tests: - `contents_child_hasNewLayout_not_stamped_on_measure_only_visit` - pins the measure-phase fix - `absolute_descendant_through_contents_is_reachable_via_hasNewLayout` - pins the positive case for absolute-layout path - `absolute_phase_cleanup_does_not_stamp_when_parent_layout_skipped` - pins the negative case for absolute-layout path Reviewed By: javache Differential Revision: D107854528 Pulled By: j-piasecki fbshipit-source-id: cae5e889622296e8b6380a6428509b5ffea3e9ae
1 parent a632f9e commit 8121dac

3 files changed

Lines changed: 14 additions & 9 deletions

File tree

packages/react-native/ReactCommon/yoga/yoga/algorithm/AbsoluteLayout.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -558,7 +558,6 @@ bool layoutAbsoluteDescendants(
558558
// we need to mutate these descendents. Make sure the path of
559559
// nodes to them is mutable before positioning.
560560
child->cloneChildrenIfNeeded();
561-
cleanupContentsNodesRecursively(child);
562561
const Direction childDirection =
563562
child->resolveDirection(currentNodeDirection);
564563
// By now all descendants of the containing block that are not absolute
@@ -584,6 +583,8 @@ bool layoutAbsoluteDescendants(
584583
containingNodeAvailableInnerHeight) ||
585584
hasNewLayout;
586585

586+
cleanupContentsNodesRecursively(
587+
child, /* didPerformLayout */ hasNewLayout);
587588
if (hasNewLayout) {
588589
child->setHasNewLayout(hasNewLayout);
589590
}

packages/react-native/ReactCommon/yoga/yoga/algorithm/CalculateLayout.cpp

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -506,19 +506,23 @@ void zeroOutLayoutRecursively(yoga::Node* const node) {
506506
}
507507
}
508508

509-
void cleanupContentsNodesRecursively(yoga::Node* const node) {
509+
void cleanupContentsNodesRecursively(
510+
yoga::Node* const node,
511+
bool didPerformLayout) {
510512
if (node->hasContentsChildren()) [[unlikely]] {
511513
node->cloneContentsChildrenIfNeeded();
512514
for (auto child : node->getChildren()) {
513515
if (child->style().display() == Display::Contents) {
514516
child->getLayout() = {};
515517
child->setLayoutDimension(0, Dimension::Width);
516518
child->setLayoutDimension(0, Dimension::Height);
517-
child->setHasNewLayout(true);
519+
if (didPerformLayout) {
520+
child->setHasNewLayout(true);
521+
}
518522
child->setDirty(false);
519523
child->cloneChildrenIfNeeded();
520524

521-
cleanupContentsNodesRecursively(child);
525+
cleanupContentsNodesRecursively(child, didPerformLayout);
522526
}
523527
}
524528
}
@@ -1360,7 +1364,7 @@ static void calculateLayoutImpl(
13601364

13611365
// Clean and update all display: contents nodes with a direct path to the
13621366
// current node as they will not be traversed
1363-
cleanupContentsNodesRecursively(node);
1367+
cleanupContentsNodesRecursively(node, performLayout);
13641368
return;
13651369
}
13661370

@@ -1378,7 +1382,7 @@ static void calculateLayoutImpl(
13781382

13791383
// Clean and update all display: contents nodes with a direct path to the
13801384
// current node as they will not be traversed
1381-
cleanupContentsNodesRecursively(node);
1385+
cleanupContentsNodesRecursively(node, performLayout);
13821386
return;
13831387
}
13841388

@@ -1396,7 +1400,7 @@ static void calculateLayoutImpl(
13961400
ownerHeight)) {
13971401
// Clean and update all display: contents nodes with a direct path to the
13981402
// current node as they will not be traversed
1399-
cleanupContentsNodesRecursively(node);
1403+
cleanupContentsNodesRecursively(node, /* didPerformLayout */ false);
14001404
return;
14011405
}
14021406

@@ -1408,7 +1412,7 @@ static void calculateLayoutImpl(
14081412

14091413
// Clean and update all display: contents nodes with a direct path to the
14101414
// current node as they will not be traversed
1411-
cleanupContentsNodesRecursively(node);
1415+
cleanupContentsNodesRecursively(node, performLayout);
14121416

14131417
// STEP 1: CALCULATE VALUES FOR REMAINDER OF ALGORITHM
14141418
const FlexDirection mainAxis =

packages/react-native/ReactCommon/yoga/yoga/algorithm/CalculateLayout.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,6 @@ float calculateAvailableInnerDimension(
5555

5656
void zeroOutLayoutRecursively(yoga::Node* const node);
5757

58-
void cleanupContentsNodesRecursively(yoga::Node* const node);
58+
void cleanupContentsNodesRecursively(yoga::Node* node, bool didPerformLayout);
5959

6060
} // namespace facebook::yoga

0 commit comments

Comments
 (0)