Bug 1236745 - Fix infinite loop resulting from block formatting context entering resize oscillation due to considering floats over its whole height when sizing it. r?dholbert
What's happening here is that we enter an infinite loop by oscillating
between two states. The code assumes that (a) the available space will
never grow, only stay the same or shrink, and (b) that we should break
out of the loop if it stays the same. This also means we hit the
assertion about the available space growing every other time through the
loop.
This is in the inner loop in nsBlockFrame::ReflowBlockFrame that was
introduced in https://hg.mozilla.org/mozilla-central/rev/80ef9bb2c2e9 .
The problem is fundamentally a logic error in that code. The makes the
assumption that if you reduce the width available to a block formatting
context or replaced block-level element, its height does not shrink.
(The "replaced block" (really block formatting context) in this case, as
in the original testcase, is a scroll frame. I didn't debug the
original testcase enough to figure out what caused its sizing
characteristics, although a percentage-width image does seem like the
most likely candidate.)
Without the patch, the reftest test (but not reference) hangs, as does
the semi-simplified test in the bug (given a narrow window).
With the patch, neither the semi-simplified test in the bug nor the
reference hangs, and the reftest passes.
--- a/layout/generic/nsBlockFrame.cpp
+++ b/layout/generic/nsBlockFrame.cpp
@@ -902,31 +902,54 @@ nsBlockFrame::GetPrefWidthTightBounds(ns
}
}
}
data.ForceBreak();
return NS_OK;
}
+/**
+ * Return whether aNewAvailableSpace is smaller *on either side*
+ * (inline-start or inline-end) than aOldAvailableSpace, so that we know
+ * if we need to redo layout on an inline, replaced block, or block
+ * formatting context, because its height caused it to intersect
+ * additional floats.
+ */
static bool
AvailableSpaceShrunk(WritingMode aWM,
const LogicalRect& aOldAvailableSpace,
- const LogicalRect& aNewAvailableSpace)
+ const LogicalRect& aNewAvailableSpace,
+ bool aCanGrow /* debug-only */)
{
if (aNewAvailableSpace.ISize(aWM) == 0) {
// Positions are not significant if the inline size is zero.
return aOldAvailableSpace.ISize(aWM) != 0;
}
- NS_ASSERTION(aOldAvailableSpace.IStart(aWM) <=
- aNewAvailableSpace.IStart(aWM) &&
- aOldAvailableSpace.IEnd(aWM) >=
- aNewAvailableSpace.IEnd(aWM),
- "available space should never grow");
- return aOldAvailableSpace.ISize(aWM) != aNewAvailableSpace.ISize(aWM);
+ if (aCanGrow) {
+ NS_ASSERTION(aNewAvailableSpace.IStart(aWM) <=
+ aOldAvailableSpace.IStart(aWM) ||
+ aNewAvailableSpace.IEnd(aWM) <= aOldAvailableSpace.IEnd(aWM),
+ "available space should not shrink on the start side and "
+ "grow on the end side");
+ NS_ASSERTION(aNewAvailableSpace.IStart(aWM) >=
+ aOldAvailableSpace.IStart(aWM) ||
+ aNewAvailableSpace.IEnd(aWM) >= aOldAvailableSpace.IEnd(aWM),
+ "available space should not grow on the start side and "
+ "shrink on the end side");
+ } else {
+ NS_ASSERTION(aOldAvailableSpace.IStart(aWM) <=
+ aNewAvailableSpace.IStart(aWM) &&
+ aOldAvailableSpace.IEnd(aWM) >=
+ aNewAvailableSpace.IEnd(aWM),
+ "available space should never grow");
+ }
+ // Have we shrunk on either side?
+ return aNewAvailableSpace.IStart(aWM) > aOldAvailableSpace.IStart(aWM) ||
+ aNewAvailableSpace.IEnd(aWM) < aOldAvailableSpace.IEnd(aWM);
}
static LogicalSize
CalculateContainingBlockSizeForAbsolutes(WritingMode aWM,
const nsHTMLReflowState& aReflowState,
LogicalSize aFrameSize)
{
// The issue here is that for a 'height' of 'auto' the reflow state
@@ -3372,18 +3395,25 @@ nsBlockFrame::ReflowBlockFrame(nsBlockRe
brc.GetMetrics().Height(),
&floatManagerState);
NS_ASSERTION(floatAvailableSpace.mRect.BStart(wm) ==
oldFloatAvailableSpaceRect.BStart(wm),
"yikes");
// Restore the height to the position of the next band.
floatAvailableSpace.mRect.BSize(wm) =
oldFloatAvailableSpaceRect.BSize(wm);
+ // Determine whether the available space shrunk on either side,
+ // because (the first time round) we now know the block's height,
+ // and it intersects additional floats, or (on later iterations)
+ // because narrowing the width relative to the previous time has
+ // caused the block to become taller. Note that since we're
+ // reflowing the block, narrowing the width might also make it
+ // shorter, so we must pass aCanGrow as true.
if (!AvailableSpaceShrunk(wm, oldFloatAvailableSpaceRect,
- floatAvailableSpace.mRect)) {
+ floatAvailableSpace.mRect, true)) {
break;
}
bool advanced = false;
if (!aState.ReplacedBlockFitsInAvailSpace(replacedBlock,
floatAvailableSpace)) {
// Advance to the next band.
nscoord newBCoord = aState.mBCoord;
@@ -4411,18 +4441,21 @@ nsBlockFrame::PlaceLine(nsBlockReflowSta
aAvailableSpaceHeight,
aFloatStateBeforeLine).mRect;
NS_ASSERTION(aFloatAvailableSpace.BStart(wm) ==
oldFloatAvailableSpace.BStart(wm), "yikes");
// Restore the height to the position of the next band.
aFloatAvailableSpace.BSize(wm) = oldFloatAvailableSpace.BSize(wm);
// If the available space between the floats is smaller now that we
// know the height, return false (and cause another pass with
- // LINE_REFLOW_REDO_MORE_FLOATS).
- if (AvailableSpaceShrunk(wm, oldFloatAvailableSpace, aFloatAvailableSpace)) {
+ // LINE_REFLOW_REDO_MORE_FLOATS). We ensure aAvailableSpaceHeight
+ // never decreases, which means that we can't reduce the set of floats
+ // we intersect, which means that the available space cannot grow.
+ if (AvailableSpaceShrunk(wm, oldFloatAvailableSpace, aFloatAvailableSpace,
+ false)) {
return false;
}
#ifdef DEBUG
if (!GetParent()->IsCrazySizeAssertSuppressed()) {
static nscoord lastHeight = 0;
if (CRAZY_SIZE(aLine->BStart())) {
lastHeight = aLine->BStart();
new file mode 100644
--- /dev/null
+++ b/layout/reftests/floats/1236745-1-ref.html
@@ -0,0 +1,39 @@
+<!DOCTYPE HTML>
+<title>Reftest, bug 1236745</title>
+<style>
+
+div.contain {
+ border: medium solid blue;
+ width: 100px;
+ height: 200px;
+}
+
+.float1 {
+ float: left;
+ background: yellow;
+ width: 10px;
+ height: 60px;
+}
+
+.float2 {
+ float: left; clear: left;
+ background: aqua;
+ width: 50px;
+ height: 50px;
+}
+
+.bfc {
+ float: right;
+ background: #00137f;
+ width: 50px;
+ height: 50px;
+}
+
+</style>
+
+
+<div class="contain">
+ <div class="float1"></div>
+ <div class="bfc"></div>
+ <div class="float2"></div>
+</div>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/floats/1236745-1.html
@@ -0,0 +1,49 @@
+<!DOCTYPE HTML>
+<title>Reftest, bug 1236745</title>
+<style>
+
+div.contain {
+ border: medium solid blue;
+ width: 100px;
+ height: 200px;
+}
+
+.float1 {
+ float: left;
+ background: yellow;
+ width: 10px;
+ height: 60px;
+}
+
+.float2 {
+ float: left; clear: left;
+ background: aqua;
+ width: 50px;
+ height: 50px;
+}
+
+.bfc {
+ overflow: hidden;
+ background: fuchsia;
+ /*
+ * Will be 90px wide (and thus 90px high) if placed based on only its
+ * top or based on a 50px height, but 50px wide (and thus 50px high)
+ * if placed based on a 90px height.
+ */
+}
+
+img {
+ width: 100%;
+ display: block;
+}
+
+</style>
+
+
+<div class="contain">
+ <div class="float1"></div>
+ <div class="float2"></div>
+ <div class="bfc">
+ <img src="../bugs/solidblue.png"> <!-- 16x16 blue image -->
+ </div>
+</div>
--- a/layout/reftests/floats/reftest.list
+++ b/layout/reftests/floats/reftest.list
@@ -15,16 +15,17 @@ fails == 345369-2.html 345369-2-ref.html
== 345369-3.html 345369-3-ref.html
== 345369-4.html 345369-4-ref.html
== 345369-5.html 345369-5-ref.html
== 429974-1.html 429974-1-ref.html
== 478834-1.html 478834-1-ref.html
== 546048-1.html 546048-1-ref.html
== 775350-1.html 775350-1-ref.html
== 1114329.html 1114329-ref.html
+== 1236745-1.html 1236745-1-ref.html
== float-in-rtl-1a.html float-in-rtl-1-ref.html
== float-in-rtl-1b.html float-in-rtl-1-ref.html
== float-in-rtl-1c.html float-in-rtl-1-ref.html
== float-in-rtl-1d.html float-in-rtl-1-ref.html
== float-in-rtl-2a.html float-in-rtl-2-ref.html
== float-in-rtl-2b.html float-in-rtl-2-ref.html
== float-in-rtl-2c.html float-in-rtl-2-ref.html
== float-in-rtl-2d.html float-in-rtl-2-ref.html