From 30a1f1e16184e2e1620c8a87261a4a143b77fd76 Mon Sep 17 00:00:00 2001 From: Ting-Yu Lin Date: Fri, 3 Sep 2021 21:23:52 +0000 Subject: [PATCH] Bug 1728319 - Tweak an assertion to recognize integer overflow due to huge margin/border/padding. r=dholbert We could fix the testcase in this patch by promoting `FlexLine::mTotalItemMBP` to `AuCoord64`, but then `layout/generic/crashtests/1488762-1.html` will start crashing. It is because `FlexItem::OuterMainSize()` can return negative size due to integer overflow, and the negative sizes will accumulate in `mTotalOuterHypotheticalMainSize`. Instead of promoting more variables to `AuCoord64` to workaround huge margin/border/padding that we cannot handle gracefully, this patch tweaks the assertion to check only if `mTotalOuterHypotheticalMainSize` and `mTotalItemMBP` have valid values. Differential Revision: https://phabricator.services.mozilla.com/D124409 --- layout/generic/crashtests/1728319.html | 11 +++++++++++ layout/generic/crashtests/crashtests.list | 1 + layout/generic/nsFlexContainerFrame.cpp | 23 +++++++++++++++++------ 3 files changed, 29 insertions(+), 6 deletions(-) create mode 100644 layout/generic/crashtests/1728319.html diff --git a/layout/generic/crashtests/1728319.html b/layout/generic/crashtests/1728319.html new file mode 100644 index 000000000000..1ef3bf1554a7 --- /dev/null +++ b/layout/generic/crashtests/1728319.html @@ -0,0 +1,11 @@ + + + + + + diff --git a/layout/generic/crashtests/crashtests.list b/layout/generic/crashtests/crashtests.list index b7a296a8d1be..9206bcd276bb 100644 --- a/layout/generic/crashtests/crashtests.list +++ b/layout/generic/crashtests/crashtests.list @@ -797,3 +797,4 @@ load 1697262-1.html pref(layout.css.aspect-ratio.enabled,true) load 1682032.html pref(layout.css.aspect-ratio.enabled,true) load 1699263.html pref(layout.css.aspect-ratio.enabled,true) load 1699468.html +load 1728319.html diff --git a/layout/generic/nsFlexContainerFrame.cpp b/layout/generic/nsFlexContainerFrame.cpp index f4813f0a7507..6c700800905a 100644 --- a/layout/generic/nsFlexContainerFrame.cpp +++ b/layout/generic/nsFlexContainerFrame.cpp @@ -3059,14 +3059,25 @@ void FlexLine::ResolveFlexibleLengths(nscoord aFlexContainerMainSize, availableFreeSpace -= item.MainSize(); } - FLEX_LOG(" available free space = %" PRId64, availableFreeSpace.value); + FLEX_LOG(" available free space: %" PRId64 "; flex items should \"%s\"", + availableFreeSpace.value, isUsingFlexGrow ? "grow" : "shrink"); // The sign of our free space should agree with the type of flexing - // (grow/shrink) that we're doing (except if we've had integer overflow, - // which is unlikely given we've used 64-bit arithmetic). Any disagreement - // should've made us use the other type of flexing, or should've been - // resolved in FreezeItemsEarly. - MOZ_ASSERT((isUsingFlexGrow && availableFreeSpace >= 0) || + // (grow/shrink) that we're doing. Any disagreement should've made us use + // the other type of flexing, or should've been resolved in + // FreezeItemsEarly. + // + // Note: it's possible that an individual flex item has huge + // margin/border/padding that makes either its + // MarginBorderPaddingSizeInMainAxis() or OuterMainSize() negative due to + // integer overflow. If that happens, the accumulated + // mTotalOuterHypotheticalMainSize or mTotalItemMBP could be negative due to + // that one item's negative (overflowed) size. In that case, we throw up our + // hands and don't require isUsingFlexGrow to agree with availableFreeSpace. + // Luckily, we won't get stuck in the algorithm below, and just distribute + // the wrong availableFreeSpace with the wrong grow/shrink factors. + MOZ_ASSERT(!(mTotalOuterHypotheticalMainSize >= 0 && mTotalItemMBP >= 0) || + (isUsingFlexGrow && availableFreeSpace >= 0) || (!isUsingFlexGrow && availableFreeSpace <= 0), "availableFreeSpace's sign should match isUsingFlexGrow");