Skip to content

Commit 7e270a4

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 6ef7f7d commit 7e270a4

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
@@ -528,7 +528,6 @@ bool layoutAbsoluteDescendants(
528528
// we need to mutate these descendents. Make sure the path of
529529
// nodes to them is mutable before positioning.
530530
child->cloneChildrenIfNeeded();
531-
cleanupContentsNodesRecursively(child);
532531
const Direction childDirection =
533532
child->resolveDirection(currentNodeDirection);
534533
// By now all descendants of the containing block that are not absolute
@@ -554,6 +553,8 @@ bool layoutAbsoluteDescendants(
554553
containingNodeAvailableInnerHeight) ||
555554
hasNewLayout;
556555

556+
cleanupContentsNodesRecursively(
557+
child, /* didPerformLayout */ hasNewLayout);
557558
if (hasNewLayout) {
558559
child->setHasNewLayout(hasNewLayout);
559560
}

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

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -480,19 +480,23 @@ static void zeroOutLayoutRecursively(yoga::Node* const node) {
480480
}
481481
}
482482

483-
void cleanupContentsNodesRecursively(yoga::Node* const node) {
483+
void cleanupContentsNodesRecursively(
484+
yoga::Node* const node,
485+
bool didPerformLayout) {
484486
if (node->hasContentsChildren()) [[unlikely]] {
485487
node->cloneContentsChildrenIfNeeded();
486488
for (auto child : node->getChildren()) {
487489
if (child->style().display() == Display::Contents) {
488490
child->getLayout() = {};
489491
child->setLayoutDimension(0, Dimension::Width);
490492
child->setLayoutDimension(0, Dimension::Height);
491-
child->setHasNewLayout(true);
493+
if (didPerformLayout) {
494+
child->setHasNewLayout(true);
495+
}
492496
child->setDirty(false);
493497
child->cloneChildrenIfNeeded();
494498

495-
cleanupContentsNodesRecursively(child);
499+
cleanupContentsNodesRecursively(child, didPerformLayout);
496500
}
497501
}
498502
}
@@ -1317,7 +1321,7 @@ static void calculateLayoutImpl(
13171321

13181322
// Clean and update all display: contents nodes with a direct path to the
13191323
// current node as they will not be traversed
1320-
cleanupContentsNodesRecursively(node);
1324+
cleanupContentsNodesRecursively(node, performLayout);
13211325
return;
13221326
}
13231327

@@ -1335,7 +1339,7 @@ static void calculateLayoutImpl(
13351339

13361340
// Clean and update all display: contents nodes with a direct path to the
13371341
// current node as they will not be traversed
1338-
cleanupContentsNodesRecursively(node);
1342+
cleanupContentsNodesRecursively(node, performLayout);
13391343
return;
13401344
}
13411345

@@ -1353,7 +1357,7 @@ static void calculateLayoutImpl(
13531357
ownerHeight)) {
13541358
// Clean and update all display: contents nodes with a direct path to the
13551359
// current node as they will not be traversed
1356-
cleanupContentsNodesRecursively(node);
1360+
cleanupContentsNodesRecursively(node, /* didPerformLayout */ false);
13571361
return;
13581362
}
13591363

@@ -1365,7 +1369,7 @@ static void calculateLayoutImpl(
13651369

13661370
// Clean and update all display: contents nodes with a direct path to the
13671371
// current node as they will not be traversed
1368-
cleanupContentsNodesRecursively(node);
1372+
cleanupContentsNodesRecursively(node, performLayout);
13691373

13701374
// STEP 1: CALCULATE VALUES FOR REMAINDER OF ALGORITHM
13711375
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
@@ -35,6 +35,6 @@ bool calculateLayoutInternal(
3535
uint32_t depth,
3636
uint32_t generationCount);
3737

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

4040
} // namespace facebook::yoga

0 commit comments

Comments
 (0)