Bug 1461446 - Make flex layout explicitly handle integer overflow when summing up flex item hypothetical sizes. r=mats, a=RyanVM
authorDaniel Holbert <dholbert@cs.stanford.edu>
Fri, 25 May 2018 19:46:29 -0700
changeset 473532 92bbbb44e97deeca509a1eb51d2d0f76a1091b3a
parent 473531 2449a216c49ff59d6a7f45a7ca33ee8739da2fff
child 473533 1a44b26d00a582d19259018d4d467d5bf62dbbbd
push id1728
push userjlund@mozilla.com
push dateMon, 18 Jun 2018 21:12:27 +0000
treeherdermozilla-release@c296fde26f5f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats, RyanVM
bugs1461446
milestone61.0
Bug 1461446 - Make flex layout explicitly handle integer overflow when summing up flex item hypothetical sizes. r=mats, a=RyanVM This patch accomodates for the unfortunate fact that elements with "table-layout:fixed" have a max-content size of nscoord_MAX (infinity, effectively), which turns out to be an easy source of integer overflow during flex layout. Before this patch, a flex container with "table-layout:fixed" in several flex items could end up triggering integer-overflow & making the wrong judgement on its arithmetic to determine... - whether a given flex item will fit on an existing flex line. - whether we've got positive free space and need to grow our items, or have negative free space and need to shrink our items. This patch makes two changes to fix this issue. (1) This patch makes us use CheckedInt when summing up flex item hypothetical sizes, which prevents integer overflow from flipping the sign of our line's total length. (2) This patch makes us *directly* track the space reserved for flex item margin/border/padding within a flex line. Previously, we tracked this implicitly as the difference between two other quantities that we stored; but with the other changes in this patch, those two other quantities can *both* trigger overflow and get clamped, which would make us lose track of how much space to reserve for margin/border/padding. So now we simply track that space-to-reserve directly. MozReview-Commit-ID: 9izhOnlS4F1
layout/generic/nsFlexContainerFrame.cpp
layout/reftests/flexbox/flexbox-table-flex-items-4-ref.html
layout/reftests/flexbox/flexbox-table-flex-items-4.html
layout/reftests/flexbox/flexbox-table-flex-items-5-ref.html
layout/reftests/flexbox/flexbox-table-flex-items-5.html
layout/reftests/flexbox/reftest.list
--- a/layout/generic/nsFlexContainerFrame.cpp
+++ b/layout/generic/nsFlexContainerFrame.cpp
@@ -250,16 +250,26 @@ PhysicalCoordFromFlexRelativeCoord(nscoo
                                    nscoord aContainerSize,
                                    AxisOrientationType aAxis) {
   if (AxisGrowsInPositiveDirection(aAxis)) {
     return aFlexRelativeCoord;
   }
   return aContainerSize - aFlexRelativeCoord;
 }
 
+// Add two nscoord values, using CheckedInt to handle integer overflow.
+// This function returns the sum of its two args -- but if we trigger integer
+// overflow while adding them, then this function returns nscoord_MAX instead.
+static nscoord
+AddChecked(nscoord aFirst, nscoord aSecond)
+{
+  CheckedInt<nscoord> checkedResult = CheckedInt<nscoord>(aFirst) + aSecond;
+  return checkedResult.isValid() ? checkedResult.value() : nscoord_MAX;
+}
+
 // Helper-macros to let us pick one of two expressions to evaluate
 // (an inline-axis expression vs. a block-axis expression), to get a
 // main-axis or cross-axis component.
 // For code that has e.g. a LogicalSize object, the methods
 // FlexboxAxisTracker::GetMainComponent and GetCrossComponent are cleaner
 // than these macros. But in cases where we simply have two separate
 // expressions for ISize and BSize (which may be expensive to evaluate),
 // these macros can be used to ensure that only the needed expression is
@@ -875,17 +885,17 @@ protected:
  * Manages a linked list of the FlexItems that are in the line.
  */
 class nsFlexContainerFrame::FlexLine : public LinkedListElement<FlexLine>
 {
 public:
   FlexLine()
   : mNumItems(0),
     mNumFrozenItems(0),
-    mTotalInnerHypotheticalMainSize(0),
+    mTotalItemMBP(0),
     mTotalOuterHypotheticalMainSize(0),
     mLineCrossSize(0),
     mFirstBaselineOffset(nscoord_MIN),
     mLastBaselineOffset(nscoord_MIN)
   {}
 
   // Returns the sum of our FlexItems' outer hypothetical main sizes.
   // ("outer" = margin-box, and "hypothetical" = before flexing)
@@ -951,18 +961,34 @@ public:
       mItems.insertBack(aItem);
     }
 
     // Update our various bookkeeping member-vars:
     mNumItems++;
     if (aItem->IsFrozen()) {
       mNumFrozenItems++;
     }
-    mTotalInnerHypotheticalMainSize += aItemInnerHypotheticalMainSize;
-    mTotalOuterHypotheticalMainSize += aItemOuterHypotheticalMainSize;
+
+    nscoord itemMBP =
+      aItemOuterHypotheticalMainSize - aItemInnerHypotheticalMainSize;
+
+    // Note: If our flex item is (or contains) a table with
+    // "table-layout:fixed", it may have a value near nscoord_MAX as its
+    // hypothetical main size. This means we can run into absurdly large sizes
+    // here, even when the author didn't explicitly specify anything huge.
+    // We'd really rather not allow that to cause integer overflow (e.g. we
+    // don't want that to make mTotalOuterHypotheticalMainSize overflow to a
+    // negative value), because that'd make us incorrectly think that we should
+    // grow our flex items rather than shrink them when it comes time to
+    // resolve flexible items. Hence, we sum up the hypothetical sizes using a
+    // helper function AddChecked() to avoid overflow.
+    mTotalItemMBP = AddChecked(mTotalItemMBP, itemMBP);
+
+    mTotalOuterHypotheticalMainSize =
+      AddChecked(mTotalOuterHypotheticalMainSize, aItemOuterHypotheticalMainSize);
   }
 
   // Computes the cross-size and baseline position of this FlexLine, based on
   // its FlexItems.
   void ComputeCrossSizeAndBaseline(const FlexboxAxisTracker& aAxisTracker);
 
   // Returns the cross-size of this line.
   nscoord GetLineCrossSize() const { return mLineCrossSize; }
@@ -1030,18 +1056,24 @@ private:
                       // for splitting lines across continuations. Then we can
                       // update this count carefully.)
 
   // Number of *frozen* FlexItems in this line, based on FlexItem::IsFrozen().
   // Mostly used for optimization purposes, e.g. to bail out early from loops
   // when we can tell they have nothing left to do.
   uint32_t mNumFrozenItems;
 
-  nscoord mTotalInnerHypotheticalMainSize;
+  // Sum of margin/border/padding for the FlexItems in this FlexLine.
+  nscoord mTotalItemMBP;
+
+  // Sum of FlexItems' outer hypothetical main sizes.
+  // (i.e. their flex base sizes, clamped via their min/max-size properties,
+  // plus their main-axis margin/border/padding.)
   nscoord mTotalOuterHypotheticalMainSize;
+
   nscoord mLineCrossSize;
   nscoord mFirstBaselineOffset;
   nscoord mLastBaselineOffset;
 };
 
 // Information about a strut left behind by a FlexItem that's been collapsed
 // using "visibility:collapse".
 struct nsFlexContainerFrame::StrutInfo {
@@ -2525,21 +2557,18 @@ FlexLine::ResolveFlexibleLengths(nscoord
     return;
   }
   MOZ_ASSERT(!IsEmpty() || aLineInfo,
              "empty lines should take the early-return above");
 
   // Subtract space occupied by our items' margins/borders/padding, so we can
   // just be dealing with the space available for our flex items' content
   // boxes.
-  nscoord spaceReservedForMarginBorderPadding =
-    mTotalOuterHypotheticalMainSize - mTotalInnerHypotheticalMainSize;
-
   nscoord spaceAvailableForFlexItemsContentBoxes =
-    aFlexContainerMainSize - spaceReservedForMarginBorderPadding;
+    aFlexContainerMainSize - mTotalItemMBP;
 
   nscoord origAvailableFreeSpace;
   bool isOrigAvailFreeSpaceInitialized = false;
 
   // NOTE: I claim that this chunk of the algorithm (the looping part) needs to
   // run the loop at MOST mNumItems times.  This claim should hold up
   // because we'll freeze at least one item on each loop iteration, and once
   // we've run out of items to freeze, there's nothing left to do.  However,
@@ -3837,21 +3866,31 @@ nsFlexContainerFrame::GenerateFlexLines(
 
     nscoord itemInnerHypotheticalMainSize = item->GetMainSize();
     nscoord itemOuterHypotheticalMainSize =
       item->GetOuterMainSize(aAxisTracker.GetMainAxis());
 
     // Check if we need to wrap |item| to a new line
     // (i.e. check if its outer hypothetical main size pushes our line over
     // the threshold)
-    if (wrapThreshold != NS_UNCONSTRAINEDSIZE &&
-        !curLine->IsEmpty() && // No need to wrap at start of a line.
-        wrapThreshold < (curLine->GetTotalOuterHypotheticalMainSize() +
-                         itemOuterHypotheticalMainSize)) {
-      curLine = AddNewFlexLineToList(aLines, shouldInsertAtFront);
+    if (wrapThreshold != NS_UNCONSTRAINEDSIZE && // Don't wrap if unconstrained.
+        !curLine->IsEmpty()) { // Don't wrap if this will be line's first item.
+      // If the line will be longer than wrapThreshold after adding this item,
+      // then wrap to a new line before inserting this item.
+      // NOTE: We have to account for the fact that
+      // itemOuterHypotheticalMainSize might be huge, if our item is (or
+      // contains) a table with "table-layout:fixed". So we use AddChecked()
+      // rather than (possibly-overflowing) normal addition, to be sure we don't
+      // make the wrong judgement about whether the item fits on this line.
+      nscoord newOuterSize =
+        AddChecked(curLine->GetTotalOuterHypotheticalMainSize(),
+                   itemOuterHypotheticalMainSize);
+      if (newOuterSize == nscoord_MAX || newOuterSize > wrapThreshold) {
+        curLine = AddNewFlexLineToList(aLines, shouldInsertAtFront);
+      }
     }
 
     // Add item to current flex line (and update the line's bookkeeping about
     // how large its items collectively are).
     curLine->AddItem(item.release(), shouldInsertAtFront,
                      itemInnerHypotheticalMainSize,
                      itemOuterHypotheticalMainSize);
 
new file mode 100644
--- /dev/null
+++ b/layout/reftests/flexbox/flexbox-table-flex-items-4-ref.html
@@ -0,0 +1,50 @@
+<!DOCTYPE html>
+<!--
+     Any copyright is dedicated to the Public Domain.
+     http://creativecommons.org/publicdomain/zero/1.0/
+-->
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>
+    CSS Reference Case
+  </title>
+  <style>
+    .container {
+      display: flex;
+      flex-wrap: wrap;
+      width: 300px;
+      border: 1px solid black;
+      font: 12px monospace;
+      margin-bottom: 5px;
+    }
+    .container > * {
+      flex: 1 400px; /* Bigger than flex container;
+                        should linewrap and cooperatively shrink to fit. */
+      padding: 5px 10px;
+    }
+    table {
+      width: 100%;
+      background: lightgray;
+    }
+  </style>
+</head>
+<body>
+  <div class="container">
+    <div>
+      <table><td>a</td></table>
+    </div>
+    <div>
+      <table><td>b</td></table>
+    </div>
+  </div>
+  <div class="container">
+    <div>
+      <table><td>a</td></table>
+    </div>
+    <div>
+      <table><td>b</td></table>
+    </div>
+  </div>
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/flexbox/flexbox-table-flex-items-4.html
@@ -0,0 +1,55 @@
+<!DOCTYPE html>
+<!--
+     Any copyright is dedicated to the Public Domain.
+     http://creativecommons.org/publicdomain/zero/1.0/
+-->
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>
+    CSS Test: Testing that tables with "table-layout:fixed" can shrink
+    from their default flex base size to fit their multi-line flex container.
+  </title>
+  <style>
+    .container {
+      display: flex;
+      flex-wrap: wrap;
+      width: 300px;
+      border: 1px solid black;
+      font: 12px monospace;
+      margin-bottom: 5px;
+    }
+    .forceMaxContent > * {
+      width: -moz-max-content;
+      width: max-content;
+    }
+    .container > * {
+      display: flex;
+      padding: 5px 10px;
+    }
+    table {
+      width: 100%;
+      table-layout: fixed;
+      background: lightgray;
+    }
+  </style>
+</head>
+<body>
+  <div class="container">
+    <div>
+      <table><td>a</td></table>
+    </div>
+    <div>
+      <table><td>b</td></table>
+    </div>
+  </div>
+  <div class="container forceMaxContent">
+    <div>
+      <table><td>a</td></table>
+    </div>
+    <div>
+      <table><td>b</td></table>
+    </div>
+  </div>
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/flexbox/flexbox-table-flex-items-5-ref.html
@@ -0,0 +1,45 @@
+<!DOCTYPE html>
+<!--
+     Any copyright is dedicated to the Public Domain.
+     http://creativecommons.org/publicdomain/zero/1.0/
+-->
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>
+    CSS Reference Case
+  </title>
+  <style>
+    .container {
+      display: flex;
+      width: 300px;
+      border: 1px solid black;
+      margin-bottom: 5px;
+    }
+    .container > * {
+      margin: 5px;
+      background: lightgray;
+      flex: 1 400px; /* Bigger than flex container; should cooperatively shrink to fit. */
+      height: 30px;
+    }
+  </style>
+</head>
+<body>
+  <div class="container">
+    <div>
+      <table><td></td></table>
+    </div>
+    <div>
+      <table><td></td></table>
+    </div>
+  </div>
+  <div class="container">
+    <div>
+      <table><td></td></table>
+    </div>
+    <div>
+      <table><td></td></table>
+    </div>
+  </div>
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/flexbox/flexbox-table-flex-items-5.html
@@ -0,0 +1,53 @@
+<!DOCTYPE html>
+<!--
+     Any copyright is dedicated to the Public Domain.
+     http://creativecommons.org/publicdomain/zero/1.0/
+-->
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>
+    CSS Test: Testing that tables with "table-layout:fixed" can shrink
+    from their default flex base size to fit their flex container.
+  </title>
+  <style>
+    .container {
+      display: flex;
+      width: 300px;
+      border: 1px solid black;
+      margin-bottom: 5px;
+    }
+    .forceMaxContent > * {
+      width: -moz-max-content;
+      width: max-content;
+    }
+    .container > * {
+      margin: 5px;
+    }
+    table {
+      width: 100%;
+      height: 30px;
+      table-layout: fixed;
+      background: lightgray;
+    }
+  </style>
+</head>
+<body>
+  <div class="container">
+    <div>
+      <table><td></td></table>
+    </div>
+    <div>
+      <table><td></td></table>
+    </div>
+  </div>
+  <div class="container forceMaxContent">
+    <div>
+      <table><td></td></table>
+    </div>
+    <div>
+      <table><td></td></table>
+    </div>
+  </div>
+</body>
+</html>
--- a/layout/reftests/flexbox/reftest.list
+++ b/layout/reftests/flexbox/reftest.list
@@ -121,8 +121,10 @@ fuzzy-if(gtkWidget,1,66) == flexbox-widg
 fuzzy-if(gtkWidget,1,74) == flexbox-widget-flex-items-2.html flexbox-widget-flex-items-2-ref.html
 skip-if(gtkWidget) == flexbox-widget-flex-items-3.html flexbox-widget-flex-items-3-ref.html # bug 1260965
 fuzzy-if(gtkWidget,1,31) == flexbox-widget-flex-items-4.html flexbox-widget-flex-items-4-ref.html
 
 # Tests for table flex items
 == flexbox-table-flex-items-1.html flexbox-table-flex-items-1-ref.html
 == flexbox-table-flex-items-2.html flexbox-table-flex-items-2-ref.html
 == flexbox-table-flex-items-3.html flexbox-table-flex-items-3-ref.html
+== flexbox-table-flex-items-4.html flexbox-table-flex-items-4-ref.html
+== flexbox-table-flex-items-5.html flexbox-table-flex-items-5-ref.html