Fix flex-shrink runaway when the sole shrinkable item has an aspectRatio basis

tests/YGFlexShrinkAspectRatioBugTest.cpp

@@ -0,0 +1,120 @@
+/*
+ * 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.
+ */
+
+// Regression test for https://github.com/react/yoga/issues/2038
+// Sibling case to YGFlexShrinkBorderBugTest (issue #1665): an item whose flex
+// basis comes from an aspectRatio, and which is the only shrinkable item on an
+// overflowing line, is measured at ~8.3e9 instead of being clamped to 0.
+//
+// Mechanism: distributeFreeSpaceFirstPass freezes the item and subtracts its
+// contribution from totalFlexShrinkScaledFactors. The compiler contracts that
+// multiply-subtract into a fused multiply-add using the unrounded product, so
+// it does not cancel against the float accumulator built in calculateFlexLine
+// and leaves a residual of 2^-19. distributeFreeSpaceSecondPass then divides
+// the remaining free space by that residual. The absolute 1e-6f epsilon guard
+// added for #1665 does not catch it, because the residual's magnitude scales
+// with the basis and shrink factor.
+
+#include <gtest/gtest.h>
+#include <yoga/Yoga.h>
+
+namespace {
+
+// root(350x350) > row(flexDirection: row) > [ box(aspectRatio, flexShrink), sibling(600x23) ]
+// Returns the box; the caller owns the tree via rootOut.
+YGNodeRef buildOverflowingRow(YGConfigRef config, YGNodeRef* rootOut, float shrink) {
+  YGNodeRef root = YGNodeNewWithConfig(config);
+  YGNodeStyleSetWidth(root, 350.0f);
+  YGNodeStyleSetHeight(root, 350.0f);
+
+  YGNodeRef row = YGNodeNewWithConfig(config);
+  YGNodeStyleSetFlexDirection(row, YGFlexDirectionRow);
+  YGNodeInsertChild(root, row, 0);
+
+  // The only shrinkable item on the line. Its flex basis is derived from the
+  // aspect ratio and the stretched cross size (23 * 22/24 = 21.083...), which is
+  // what makes the precision asymmetry observable.
+  YGNodeRef box = YGNodeNewWithConfig(config);
+  YGNodeStyleSetAspectRatio(box, 22.0f / 24.0f);
+  YGNodeStyleSetFlexShrink(box, shrink);
+  YGNodeInsertChild(row, box, 0);
+
+  // Overflows the row so remainingFreeSpace is negative. flexShrink defaults to
+  // 0, so this item cannot shrink and the box is the sole candidate.
+  YGNodeRef sibling = YGNodeNewWithConfig(config);
+  YGNodeStyleSetWidth(sibling, 600.0f);
+  YGNodeStyleSetHeight(sibling, 23.0f);
+  YGNodeInsertChild(row, sibling, 1);
+
+  *rootOut = root;
+  return box;
+}
+
+} // namespace
+
+TEST(YGFlexShrinkAspectRatioBug, aspect_ratio_sole_shrinkable_item_on_overflowing_line) {
+  YGConfigRef config = YGConfigNew();
+  YGNodeRef root = nullptr;
+  YGNodeRef box = buildOverflowingRow(config, &root, 3.0f);
+
+  YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);
+
+  // Unshrunk the box would be 21.083 wide; shrinking can only reduce it.
+  EXPECT_GE(YGNodeLayoutGetWidth(box), 0.0f);
+  EXPECT_LE(YGNodeLayoutGetWidth(box), 22.0f)
+      << "shrinking item is astronomically large (bug)";
+  // aspectRatio derives the cross axis from the main axis, so a runaway width
+  // propagates to the height as well.
+  EXPECT_LE(YGNodeLayoutGetHeight(box), 24.0f)
+      << "runaway propagated to the cross axis via aspectRatio";
+
+  YGNodeFreeRecursive(root);
+  YGConfigFree(config);
+}
+
+// flexShrink 3 is not special. Whether a given factor diverges depends on where
+// its product with the 55/3 basis lands in the float mantissa, so cover a
+// spread. Values 1 and 2 already clamp correctly on the unfixed code; the rest
+// do not.
+TEST(YGFlexShrinkAspectRatioBug, bounded_across_shrink_factors) {
+  const float shrinkFactors[] = {
+      1.0f, 2.0f, 3.0f, 6.0f, 9.0f, 11.0f, 12.0f, 19.0f, 22.0f, 999.0f};
+
+  for (float shrink : shrinkFactors) {
+    YGConfigRef config = YGConfigNew();
+    YGNodeRef root = nullptr;
+    YGNodeRef box = buildOverflowingRow(config, &root, shrink);
+
+    YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);
+
+    EXPECT_LE(YGNodeLayoutGetWidth(box), 22.0f)
+        << "flexShrink " << shrink << " produced a runaway width";
+    EXPECT_GE(YGNodeLayoutGetWidth(box), 0.0f)
+        << "flexShrink " << shrink << " produced a negative width";
+
+    YGNodeFreeRecursive(root);
+    YGConfigFree(config);
+  }
+}
+
+// Guards the basis itself: with no shrinking the box must keep its
+// aspect-ratio-derived size, so a change here would mean the basis moved rather
+// than the distribution misbehaving.
+TEST(YGFlexShrinkAspectRatioBug, unshrinkable_item_keeps_aspect_ratio_basis) {
+  YGConfigRef config = YGConfigNew();
+  YGNodeRef root = nullptr;
+  YGNodeRef box = buildOverflowingRow(config, &root, 0.0f);
+
+  YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);
+
+  // 23 * 22/24 = 21.083..., rounded to the default pixel grid.
+  EXPECT_FLOAT_EQ(21.0f, YGNodeLayoutGetWidth(box));
+  EXPECT_FLOAT_EQ(23.0f, YGNodeLayoutGetHeight(box));
+
+  YGNodeFreeRecursive(root);
+  YGConfigFree(config);
+}

yoga/algorithm/CalculateLayout.cpp

@@ -1104,15 +1104,13 @@ static float distributeFreeSpaceSecondPass(
       if (flexShrinkScaledFactor != 0) {
         float childSize = YGUndefined;
 
-        // Use a relative epsilon guard instead of exact equality: after the
-        // first pass removes all constrained items,
-        // totalFlexShrinkScaledFactors may be near-zero rather than exactly 0
-        // due to floating-point cancellation.  Dividing by a near-zero value
-        // produces a gigantic childSize that overwhelms the min/max clamp.
-        const float shrinkFactorMagnitude =
-            std::abs(flexLine.layout.totalFlexShrinkScaledFactors);
-        if (yoga::isDefined(flexLine.layout.totalFlexShrinkScaledFactors) &&
-            shrinkFactorMagnitude < 1e-6f) {
+        // Once the first pass has frozen every shrinkable item there is nothing
+        // left to distribute proportionally. Test that with the item count
+        // rather than by comparing totalFlexShrinkScaledFactors against zero.
+        // Float accumulation is not exact -- (a + b) - a - b need not be 0 even
+        // for identical operands -- so any absolute epsilon here is a guess;
+        // the previous 1e-6f guard missed a real 1.9e-6 residual (#2038).
+        if (flexLine.layout.numberOfShrinkableItems == 0) {
           childSize = childFlexBasis + flexShrinkScaledFactor;
         } else {
           childSize = childFlexBasis +
@@ -1336,9 +1334,22 @@ static void distributeFreeSpaceFirstPass(
           // resulting in the item's size calculation being identical in the
           // first and second passes.
           deltaFreeSpace += boundMainSize - childFlexBasis;
+          // Round to float before subtracting. Left inline, the compiler may
+          // contract this multiply-subtract into a fused multiply-add that
+          // uses the unrounded product, so it does not cancel against the
+          // float accumulator built in calculateFlexLine and leaves a residual
+          // (2^-19 for an aspect-ratio basis) which
+          // distributeFreeSpaceSecondPass then divides the remaining free space
+          // by.
+          const float excludedShrinkScaledFactor =
+              -currentLineChild->resolveFlexShrink() *
+              currentLineChild->getLayout().computedFlexBasis.unwrap();
           flexLine.layout.totalFlexShrinkScaledFactors -=
-              (-currentLineChild->resolveFlexShrink() *
-               currentLineChild->getLayout().computedFlexBasis.unwrap());
+              excludedShrinkScaledFactor;
+          if (excludedShrinkScaledFactor != 0 &&
+              flexLine.layout.numberOfShrinkableItems > 0) {
+            flexLine.layout.numberOfShrinkableItems--;
+          }
         }
       }
     } else if (

yoga/algorithm/FlexLine.cpp

@@ -28,6 +28,7 @@ FlexLine calculateFlexLine(
   float sizeConsumed = 0.0f;
   float totalFlexGrowFactors = 0.0f;
   float totalFlexShrinkScaledFactors = 0.0f;
+  size_t numberOfShrinkableItems = 0;
   size_t numberOfAutoMargins = 0;
   yoga::Node* firstElementInLine = nullptr;
 
@@ -94,8 +95,12 @@ FlexLine calculateFlexLine(
 
       // Unlike the grow factor, the shrink factor is scaled relative to the
       // child dimension.
-      totalFlexShrinkScaledFactors += -child->resolveFlexShrink() *
+      const float shrinkScaledFactor = -child->resolveFlexShrink() *
           child->getLayout().computedFlexBasis.unwrap();
+      totalFlexShrinkScaledFactors += shrinkScaledFactor;
+      if (shrinkScaledFactor != 0) {
+        numberOfShrinkableItems++;
+      }
     }
 
     itemsInFlow.push_back(child);
@@ -118,6 +123,7 @@ FlexLine calculateFlexLine(
       .layout = FlexLineRunningLayout{
           totalFlexGrowFactors,
           totalFlexShrinkScaledFactors,
+          numberOfShrinkableItems,
       }};
 }
 

yoga/algorithm/FlexLine.h

@@ -23,6 +23,13 @@ struct FlexLineRunningLayout {
   // line. This is decremented as free space is distributed.
   float totalFlexShrinkScaledFactors{0.0f};
 
+  // Number of items on the line which can still shrink, i.e. whose scaled
+  // shrink factor is non-zero. Kept in lockstep with
+  // totalFlexShrinkScaledFactors as items are frozen so that "no shrinkable
+  // items remain" can be tested exactly rather than by comparing an accumulated
+  // float against zero.
+  size_t numberOfShrinkableItems{0};
+
   // The amount of available space within inner dimensions of the line which may
   // still be distributed.
   float remainingFreeSpace{0.0f};