Fix display: contents nodes having hasNewLayout set incorrectly (#1970)

j-piasecki · facebook-github-bot · commit 43336dc45110 · 2026-06-08T03:13:30.000-07:00
Summary: X-link: react/react-native#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. 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
diff --git a/tests/YGContentsNodeHasNewLayoutTest.cpp b/tests/YGContentsNodeHasNewLayoutTest.cpp
@@ -0,0 +1,190 @@
+/*
+ * 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>
+
+namespace facebook::yoga {
+
+// Regression test for `cleanupContentsNodesRecursively` stamping
+// `hasNewLayout=true` on a display:contents child during a measurement-only
+// visit.
+//
+// Setup: Root (overflow=visible, flex column) -> Parent (flex-grow=1)
+//        -> Contents (display:contents) -> Leaf.
+// Flipping root's overflow between the two passes invalidates Parent's
+// measurement cache (computeFlexBasisForChild's `applyHeightFitContent`
+// branch flips) but leaves Parent's layout cache intact (its allotment is
+// unchanged). So in pass 2, Parent's calculateLayoutImpl runs only with
+// performLayout=false - the layout-phase visit is served from cache and
+// `cleanupContentsNodesRecursively` never runs at performLayout=true.
+TEST(YogaTest, contents_child_hasNewLayout_not_stamped_on_measure_only_visit) {
+  YGNodeRef leaf = YGNodeNew();
+  YGNodeStyleSetWidth(leaf, 20);
+  YGNodeStyleSetHeight(leaf, 20);
+
+  YGNodeRef contents = YGNodeNew();
+  YGNodeStyleSetDisplay(contents, YGDisplayContents);
+  YGNodeInsertChild(contents, leaf, 0);
+
+  YGNodeRef parent = YGNodeNew();
+  YGNodeStyleSetFlexGrow(parent, 1);
+  YGNodeInsertChild(parent, contents, 0);
+
+  YGNodeRef root = YGNodeNew();
+  YGNodeStyleSetFlexDirection(root, YGFlexDirectionColumn);
+  YGNodeStyleSetWidth(root, 200);
+  YGNodeStyleSetHeight(root, 200);
+  YGNodeStyleSetOverflow(root, YGOverflowVisible);
+  YGNodeInsertChild(root, parent, 0);
+
+  YGNodeCalculateLayout(root, 200, 200, YGDirectionLTR);
+
+  // Simulate a consumer (e.g. React Native's layout pass) reading and
+  // clearing the hasNewLayout flags.
+  YGNodeSetHasNewLayout(root, false);
+  YGNodeSetHasNewLayout(parent, false);
+  YGNodeSetHasNewLayout(contents, false);
+  YGNodeSetHasNewLayout(leaf, false);
+
+  YGNodeStyleSetOverflow(root, YGOverflowScroll);
+  YGNodeCalculateLayout(root, 200, 200, YGDirectionLTR);
+
+  EXPECT_FALSE(YGNodeGetHasNewLayout(contents))
+      << "contents.hasNewLayout was stamped during a measure-only visit "
+         "(cleanupContentsNodesRecursively ran with performLayout=false but "
+         "no matching performLayout=true visit occurred this pass)";
+
+  YGNodeFreeRecursive(root);
+}
+
+// Regression test for `cleanupContentsNodesRecursively` invoked from
+// `layoutAbsoluteDescendants`: it must stamp `hasNewLayout=true` on
+// display:contents children on the path to an absolute descendant whose
+// position changed this pass. Otherwise consumers traversing the tree via
+// hasNewLayout would skip the contents subtree and miss the update.
+//
+// Setup: root (containing block) -> staticChild (fixed 50x50)
+//        -> contents (display:contents) -> absoluteChild (right/bottom-
+//        anchored so its position depends on the containing block).
+// Growing root in pass 2 dirties only root. staticChild's fixed dimensions
+// make its layout cache hit, so its main-path cleanup never runs.
+// absoluteChild depends on the containing block and is repositioned by
+// `layoutAbsoluteDescendants`, which is the only path that can stamp
+// contents along the way.
+TEST(
+    YogaTest,
+    absolute_descendant_through_contents_is_reachable_via_hasNewLayout) {
+  YGNodeRef absoluteChild = YGNodeNew();
+  YGNodeStyleSetPositionType(absoluteChild, YGPositionTypeAbsolute);
+  YGNodeStyleSetPosition(absoluteChild, YGEdgeRight, 0);
+  YGNodeStyleSetPosition(absoluteChild, YGEdgeBottom, 0);
+  YGNodeStyleSetWidth(absoluteChild, 10);
+  YGNodeStyleSetHeight(absoluteChild, 10);
+
+  YGNodeRef contents = YGNodeNew();
+  YGNodeStyleSetDisplay(contents, YGDisplayContents);
+  YGNodeInsertChild(contents, absoluteChild, 0);
+
+  YGNodeRef staticChild = YGNodeNew();
+  YGNodeStyleSetPositionType(staticChild, YGPositionTypeStatic);
+  YGNodeStyleSetWidth(staticChild, 50);
+  YGNodeStyleSetHeight(staticChild, 50);
+  YGNodeInsertChild(staticChild, contents, 0);
+
+  YGNodeRef root = YGNodeNew();
+  YGNodeStyleSetWidth(root, 100);
+  YGNodeStyleSetHeight(root, 100);
+  YGNodeInsertChild(root, staticChild, 0);
+
+  YGNodeCalculateLayout(root, 100, 100, YGDirectionLTR);
+
+  // Simulate a consumer (e.g. React Native's layout pass) reading and
+  // clearing the hasNewLayout flags.
+  YGNodeSetHasNewLayout(root, false);
+  YGNodeSetHasNewLayout(staticChild, false);
+  YGNodeSetHasNewLayout(contents, false);
+  YGNodeSetHasNewLayout(absoluteChild, false);
+
+  YGNodeStyleSetWidth(root, 150);
+  YGNodeCalculateLayout(root, 150, 100, YGDirectionLTR);
+
+  ASSERT_TRUE(YGNodeGetHasNewLayout(absoluteChild));
+  EXPECT_TRUE(YGNodeGetHasNewLayout(staticChild));
+  EXPECT_TRUE(YGNodeGetHasNewLayout(contents))
+      << "contents node on the path to a freshly-positioned absolute "
+         "descendant must have hasNewLayout=true so consumers can traverse "
+         "to it";
+
+  YGNodeFreeRecursive(root);
+}
+
+// Regression test for `cleanupContentsNodesRecursively` invoked from
+// `layoutAbsoluteDescendants`: it must not stamp `hasNewLayout=true` on
+// display:contents children when no new layout was produced for their
+// parent this pass. Otherwise the stale flag survives across passes and
+// can be observed by a later cache-hit on the parent.
+//
+// Setup: root -> a (fixed 50x50) -> b (fixed 30x30)
+//        -> contents (display:contents) -> leaf.
+// Flipping root's overflow in pass 2 dirties only root. a and b have fixed
+// sizes so their layout caches hit; a.calculateLayoutImpl is skipped, so
+// b.calculateLayoutInternal is never invoked. `layoutAbsoluteDescendants`
+// still walks down through a and b looking for absolute descendants, but
+// there are none beneath b - so b.hasNewLayout stays false and the
+// cleanup along that walk must leave contents unflagged.
+TEST(
+    YogaTest,
+    absolute_phase_cleanup_does_not_stamp_when_parent_layout_skipped) {
+  YGNodeRef leaf = YGNodeNew();
+  YGNodeStyleSetWidth(leaf, 10);
+  YGNodeStyleSetHeight(leaf, 10);
+
+  YGNodeRef contents = YGNodeNew();
+  YGNodeStyleSetDisplay(contents, YGDisplayContents);
+  YGNodeInsertChild(contents, leaf, 0);
+
+  YGNodeRef b = YGNodeNew();
+  YGNodeStyleSetPositionType(b, YGPositionTypeStatic);
+  YGNodeStyleSetWidth(b, 30);
+  YGNodeStyleSetHeight(b, 30);
+  YGNodeInsertChild(b, contents, 0);
+
+  YGNodeRef a = YGNodeNew();
+  YGNodeStyleSetPositionType(a, YGPositionTypeStatic);
+  YGNodeStyleSetWidth(a, 50);
+  YGNodeStyleSetHeight(a, 50);
+  YGNodeInsertChild(a, b, 0);
+
+  YGNodeRef root = YGNodeNew();
+  YGNodeStyleSetWidth(root, 200);
+  YGNodeStyleSetHeight(root, 200);
+  YGNodeStyleSetOverflow(root, YGOverflowVisible);
+  YGNodeInsertChild(root, a, 0);
+
+  YGNodeCalculateLayout(root, 200, 200, YGDirectionLTR);
+
+  // Simulate a consumer (e.g. React Native's layout pass) reading and
+  // clearing the hasNewLayout flags.
+  YGNodeSetHasNewLayout(root, false);
+  YGNodeSetHasNewLayout(a, false);
+  YGNodeSetHasNewLayout(b, false);
+  YGNodeSetHasNewLayout(contents, false);
+  YGNodeSetHasNewLayout(leaf, false);
+
+  YGNodeStyleSetOverflow(root, YGOverflowScroll);
+  YGNodeCalculateLayout(root, 200, 200, YGDirectionLTR);
+
+  EXPECT_FALSE(YGNodeGetHasNewLayout(b));
+  EXPECT_FALSE(YGNodeGetHasNewLayout(contents))
+      << "contents.hasNewLayout was stamped during a walk where its "
+         "parent's hasNewLayout remained false this pass";
+
+  YGNodeFreeRecursive(root);
+}
+
+} // namespace facebook::yoga
diff --git a/yoga/algorithm/AbsoluteLayout.cpp b/yoga/algorithm/AbsoluteLayout.cpp
@@ -558,7 +558,6 @@ bool layoutAbsoluteDescendants(
       // we need to mutate these descendents. Make sure the path of
       // nodes to them is mutable before positioning.
       child->cloneChildrenIfNeeded();
-      cleanupContentsNodesRecursively(child);
       const Direction childDirection =
           child->resolveDirection(currentNodeDirection);
       // By now all descendants of the containing block that are not absolute
@@ -584,6 +583,8 @@ bool layoutAbsoluteDescendants(
                          containingNodeAvailableInnerHeight) ||
           hasNewLayout;
 
+      cleanupContentsNodesRecursively(
+          child, /* didPerformLayout */ hasNewLayout);
       if (hasNewLayout) {
         child->setHasNewLayout(hasNewLayout);
       }
diff --git a/yoga/algorithm/CalculateLayout.cpp b/yoga/algorithm/CalculateLayout.cpp
@@ -516,19 +516,23 @@ void zeroOutLayoutRecursively(yoga::Node* const node) {
   }
 }
 
-void cleanupContentsNodesRecursively(yoga::Node* const node) {
+void cleanupContentsNodesRecursively(
+    yoga::Node* const node,
+    bool didPerformLayout) {
   if (node->hasContentsChildren()) [[unlikely]] {
     node->cloneContentsChildrenIfNeeded();
     for (auto child : node->getChildren()) {
       if (child->style().display() == Display::Contents) {
         child->getLayout() = {};
         child->setLayoutDimension(0, Dimension::Width);
         child->setLayoutDimension(0, Dimension::Height);
-        child->setHasNewLayout(true);
+        if (didPerformLayout) {
+          child->setHasNewLayout(true);
+        }
         child->setDirty(false);
         child->cloneChildrenIfNeeded();
 
-        cleanupContentsNodesRecursively(child);
+        cleanupContentsNodesRecursively(child, didPerformLayout);
       }
     }
   }
@@ -1617,7 +1621,7 @@ static void calculateLayoutImpl(
 
     // Clean and update all display: contents nodes with a direct path to the
     // current node as they will not be traversed
-    cleanupContentsNodesRecursively(node);
+    cleanupContentsNodesRecursively(node, performLayout);
     return;
   }
 
@@ -1635,7 +1639,7 @@ static void calculateLayoutImpl(
 
     // Clean and update all display: contents nodes with a direct path to the
     // current node as they will not be traversed
-    cleanupContentsNodesRecursively(node);
+    cleanupContentsNodesRecursively(node, performLayout);
     return;
   }
 
@@ -1653,7 +1657,7 @@ static void calculateLayoutImpl(
           ownerHeight)) {
     // Clean and update all display: contents nodes with a direct path to the
     // current node as they will not be traversed
-    cleanupContentsNodesRecursively(node);
+    cleanupContentsNodesRecursively(node, /* didPerformLayout */ false);
     return;
   }
 
@@ -1665,7 +1669,7 @@ static void calculateLayoutImpl(
 
   // Clean and update all display: contents nodes with a direct path to the
   // current node as they will not be traversed
-  cleanupContentsNodesRecursively(node);
+  cleanupContentsNodesRecursively(node, performLayout);
 
   // STEP 1: CALCULATE VALUES FOR REMAINDER OF ALGORITHM
   const FlexDirection mainAxis =
diff --git a/yoga/algorithm/CalculateLayout.h b/yoga/algorithm/CalculateLayout.h
@@ -55,6 +55,6 @@ float calculateAvailableInnerDimension(
 
 void zeroOutLayoutRecursively(yoga::Node* const node);
 
-void cleanupContentsNodesRecursively(yoga::Node* const node);
+void cleanupContentsNodesRecursively(yoga::Node* node, bool didPerformLayout);
 
 } // namespace facebook::yoga
