Skip to content

Commit 2b56860

Browse files
j-piaseckifacebook-github-bot
authored andcommitted
Fix display: contents nodes having hasNewLayout set incorrectly
Summary: 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 Differential Revision: D107854528 Pulled By: j-piasecki
1 parent 2ff3b81 commit 2b56860

3 files changed

Lines changed: 15 additions & 9 deletions

File tree

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

Lines changed: 1 addition & 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,7 @@ bool layoutAbsoluteDescendants(
584583
containingNodeAvailableInnerHeight) ||
585584
hasNewLayout;
586585

586+
cleanupContentsNodesRecursively(child, /* performLayout */ hasNewLayout);
587587
if (hasNewLayout) {
588588
child->setHasNewLayout(hasNewLayout);
589589
}

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

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -516,19 +516,23 @@ void zeroOutLayoutRecursively(yoga::Node* const node) {
516516
}
517517
}
518518

519-
void cleanupContentsNodesRecursively(yoga::Node* const node) {
519+
void cleanupContentsNodesRecursively(
520+
yoga::Node* const node,
521+
bool performLayout) {
520522
if (node->hasContentsChildren()) [[unlikely]] {
521523
node->cloneContentsChildrenIfNeeded();
522524
for (auto child : node->getChildren()) {
523525
if (child->style().display() == Display::Contents) {
524526
child->getLayout() = {};
525527
child->setLayoutDimension(0, Dimension::Width);
526528
child->setLayoutDimension(0, Dimension::Height);
527-
child->setHasNewLayout(true);
529+
if (performLayout) {
530+
child->setHasNewLayout(true);
531+
}
528532
child->setDirty(false);
529533
child->cloneChildrenIfNeeded();
530534

531-
cleanupContentsNodesRecursively(child);
535+
cleanupContentsNodesRecursively(child, performLayout);
532536
}
533537
}
534538
}
@@ -1617,7 +1621,7 @@ static void calculateLayoutImpl(
16171621

16181622
// Clean and update all display: contents nodes with a direct path to the
16191623
// current node as they will not be traversed
1620-
cleanupContentsNodesRecursively(node);
1624+
cleanupContentsNodesRecursively(node, performLayout);
16211625
return;
16221626
}
16231627

@@ -1635,7 +1639,7 @@ static void calculateLayoutImpl(
16351639

16361640
// Clean and update all display: contents nodes with a direct path to the
16371641
// current node as they will not be traversed
1638-
cleanupContentsNodesRecursively(node);
1642+
cleanupContentsNodesRecursively(node, performLayout);
16391643
return;
16401644
}
16411645

@@ -1653,7 +1657,7 @@ static void calculateLayoutImpl(
16531657
ownerHeight)) {
16541658
// Clean and update all display: contents nodes with a direct path to the
16551659
// current node as they will not be traversed
1656-
cleanupContentsNodesRecursively(node);
1660+
cleanupContentsNodesRecursively(node, /* performLayout */ false);
16571661
return;
16581662
}
16591663

@@ -1665,7 +1669,7 @@ static void calculateLayoutImpl(
16651669

16661670
// Clean and update all display: contents nodes with a direct path to the
16671671
// current node as they will not be traversed
1668-
cleanupContentsNodesRecursively(node);
1672+
cleanupContentsNodesRecursively(node, performLayout);
16691673

16701674
// STEP 1: CALCULATE VALUES FOR REMAINDER OF ALGORITHM
16711675
const FlexDirection mainAxis =

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

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

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

58-
void cleanupContentsNodesRecursively(yoga::Node* const node);
58+
void cleanupContentsNodesRecursively(
59+
yoga::Node* const node,
60+
bool performLayout);
5961

6062
} // namespace facebook::yoga

0 commit comments

Comments
 (0)