Bug 97606 – flexbox assert fails with auto-sized item with padding

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dave Barton]

Created attachment 165674 [details]
web page that makes totalWeightedFlexShrink < 0

The attached flex-shrink-neg.html causes an assertion failure in chromium on snowleopard.

I'll also attach some debugging output, from these lines (sorry it's a bit of a mess - I could clarify):

RenderFlexibleBox.cpp:625:         fprintf(stderr, "%g + %g, %g\n", (float) preferredMainAxisExtent, (float) availableFreeSpace, (float) minMaxAppliedMainAxisExtent);//@@@
RenderFlexibleBox.cpp:630:             fprintf(stderr, " %g avail, %g shrink\n", (float) availableFreeSpace, totalWeightedFlexShrink);//@@@
RenderFlexibleBox.cpp:846:         fprintf(stderr, "   %d: %g after -= %g * %g\n", (int) i, totalWeightedFlexShrink, child->style()->flexShrink(), (float) preferredChildSize);//@@@
RenderFlexibleBox.cpp:870:             fprintf(stderr, "  preferredChildSize = %g, avail %g\n", (float) preferredChildSize, (float) availableFreeSpace);//@@@
RenderFlexibleBox.cpp:881:             fprintf(stderr, "  adjusted %g - childSize %g\n", (float) adjustedChildSize, (float) childSize);//@@@

[Dave Barton]

Created attachment 165675 [details]
crash log

[Ojan Vafai]

Thanks! I'm pretty sure the bug is in RenderFlexibleBox::preferredMainAxisContentExtentForChild. Fix should be super-easy.

LayoutUnit RenderFlexibleBox::preferredMainAxisContentExtentForChild(RenderBox* child)
{
    Length flexBasis = flexBasisForChild(child);
    if (flexBasis.isAuto()) {
        LayoutUnit mainAxisExtent = hasOrthogonalFlow(child) ? child->logicalHeight() : child->maxPreferredLogicalWidth();
        return mainAxisExtent - mainAxisBorderAndPaddingExtentForChild(child);
    }
    return std::max(LayoutUnit(0), computeMainAxisExtentForChild(child, MainOrPreferredSize, flexBasis));
}


In the first return statement we need to clamp to 0 the same way we do in the second one. If mainAxisExtent < mainAxisBorderAndPaddingExtentForChild, then we are incorrectly returning a negative value.

[Kenneth Rohde Christiansen]

I will have a look at this one tomorrow morning, thanks!

[Dave Barton]

Note the attached test only fails if subpixel layout is enabled, i.e. in chromium. It's fine in safari.

See bug 96960, comment 13: [this test] causes totalWeightedFlexShrink in RenderFlexibleBox::layoutFlexItems to go negative due to roundoff error, at least for chromium mac snowleopard [...]. I think the main issue is that MathML [or this test case] uses flexboxes but without any flex, so every child in RenderFlexibleBox::resolveFlexibleLengths becomes a violation [in a case where availableFreeSpace < 0]. Then RenderFlexibleBox::freezeViolations subtracts each preferredChildSize from totalWeightedFlexShrink [child->style()->flexShrink() is always 1], and the roundoff error brings totalWeightedFlexShrink slightly below 0.

The attached debugging output in the crash log shows all this.

Ojan's suggestion in comment 2 may be good, but it's a different problem I think. (Maybe you all know that now?) A LayoutUnit is stored exactly, as an int not float, so subtracting them can't cause roundoff error.

I think it is true that in RenderFlexibleBox::layoutFlexItems, preferredMainAxisExtent incorrectly doesn't include the padding, so availableFreeSpace is incorrectly made negative. But once this happens, RenderFlexibleBox::resolveFlexibleLengths marks every child as a violation, so RenderFlexibleBox::freezeViolations subtracts everything from totalWeightedFlexShrink, with roundoff error to make it slightly < 0 at the end.

I agree that you could probably cause the same problem with some floating-point value for flex-shrink, without needing subpixel layout.

Maybe you guys already knew all this. Anyway, I hope these comments help, or at least clarify my debugging output.

[Kenneth Rohde Christiansen]

I cannot reproduce this issue with EFL, neither when enabling SUBPIXEL_LAYOUT and SATURATED_LAYOUT_ARITHMETIC.

The test breaks in older version of Chrome and the text overlaps horizontally, but it seems to work in Canary. The problem only happens when -webkit-line-box-contain: glyphs; is used.

Maybe this is another bug which was fixed?

[Kenneth Rohde Christiansen]

> Ojan's suggestion in comment 2 may be good, but it's a different problem I think. (Maybe you all know that now?) A LayoutUnit is stored exactly, as an int not float, so subtracting them can't cause roundoff error.

Not if you use SUBPIXEL_LAYOUT right.

> I think it is true that in RenderFlexibleBox::layoutFlexItems, preferredMainAxisExtent incorrectly doesn't include the padding, so availableFreeSpace is incorrectly made negative. 

With the test cases that I made it includes padding alright.

I had overlapping fonts as older version of Chrome apparently calculated glyphs as (0,0)px sized.

I guess in your test you wanted to add padding-bottom: 0.35em; to .mfrac > * and not .math > * ?

> I agree that you could probably cause the same problem with some floating-point value for flex-shrink, without needing subpixel layout.

How? it is just a multiplier for the main size property (when auto; or the flex-basic value) which is being used when distributing negative space. RenderFlexibleBox::adjustChildSizeForMinAndMax will make sure that items are never negative sized.

[Dave Barton]

Created attachment 166036 [details]
test case without -webkit-line-box-contain

Here's a version of my test case that produces basically the same debugging output as I had before, but without -webkit-line-box-contain. Can you reproduce the assertion failure with this test case, with a current debug build of chromium?

[Dave Barton]

The new test case works fine in Chrome Canary on my Mac. Apparently the only problem is the ASSERT(... && totalWeightedFlexShrink >= 0); on line 629 in RenderFlexibleBox::layoutFlexItems, in debug builds.

[Kenneth Rohde Christiansen]

(In reply to comment #8)
> The new test case works fine in Chrome Canary on my Mac. Apparently the only problem is the ASSERT(... && totalWeightedFlexShrink >= 0); on line 629 in RenderFlexibleBox::layoutFlexItems, in debug builds.

I am not running Chrome, but I am using SUBPIXEL layout etc. I had modified the test myself but I still couldn't reproduct.

[Dave Barton]

(In reply to comment #6)
> > I think it is true that in RenderFlexibleBox::layoutFlexItems, preferredMainAxisExtent incorrectly doesn't include the padding, so availableFreeSpace is incorrectly made negative. 
> 
> With the test cases that I made it includes padding alright.

In the attached test cases, this debugging line produces this output:

RenderFlexibleBox.cpp:625:         fprintf(stderr, "%g + %g, %g\n", (float) preferredMainAxisExtent, (float) availableFreeSpace, (float) minMaxAppliedMainAxisExtent);//@@@

STDERR: 76.6 + -5.6, 76.6

> I guess in your test you wanted to add padding-bottom: 0.35em; to .mfrac > * and not .math > * ?

No, I wanted the test case as is.

> > I agree that you could probably cause the same problem with some floating-point value for flex-shrink, without needing subpixel layout.
> 
> How? it is just a multiplier for the main size property (when auto; or the flex-basic value) which is being used when distributing negative space. RenderFlexibleBox::adjustChildSizeForMinAndMax will make sure that items are never negative sized.

The items aren't negatively sized, the final value of totalWeightedFlexShrink is.

[Dave Barton]

(In reply to comment #9)
> (In reply to comment #8)
> > The new test case works fine in Chrome Canary on my Mac. Apparently the only problem is the ASSERT(... && totalWeightedFlexShrink >= 0); on line 629 in RenderFlexibleBox::layoutFlexItems, in debug builds.
> 
> I am not running Chrome, but I am using SUBPIXEL layout etc. I had modified the test myself but I still couldn't reproduct.

Can anyone (maybe Ojan or Tony ?) reproduce this assertion failure, using the second attached test case & SUBPIXEL layout on (e.g. in chromium), in a debug build? If not, can someone try that on a Mac?

Sorry, my explanations and debugging output don't seem to be communicating what I'm seeing.

[Tony Chang]

I tried both test cases and I'm not seeing the ASSERT triggered.  I am using a debug build of Chromium Linux's DumpRenderTree.

The printf produces the following output for me:
70.9062 + -5.59375, 76.5

I'm still digging . . .

[Dave Barton]

(In reply to comment #12)
> 70.9062 + -5.59375, 76.5

Thanks!! I am really confused. In FractionalLayoutUnit.h we have:

#if ENABLE(SUBPIXEL_LAYOUT)
static const int kFixedPointDenominator = 60;
...

but the numbers you're showing for preferredMainAxisExtent and availableFreeSpace aren't even integer multiples of 1/60. (They are multiples of 1/160.) Shouldn't they be multiples of 1/60?

[Dave Barton]

Wow. kFixedPointDenominator has been recently changed from 60 to 64. This explains a lot.

[Tony Chang]

(In reply to comment #14)
> Wow. kFixedPointDenominator has been recently changed from 60 to 64. This explains a lot.

Yeah, 30 hours ago: http://trac.webkit.org/changeset/129656

Let me try the tests with it changed back to 60.

[Tony Chang]

(In reply to comment #15)
> (In reply to comment #14)
> > Wow. kFixedPointDenominator has been recently changed from 60 to 64. This explains a lot.
> 
> Yeah, 30 hours ago: http://trac.webkit.org/changeset/129656
> 
> Let me try the tests with it changed back to 60.

I'm able to repro now.  I have a couple ideas on how to fix.

1) We could switch totalFlexGrow and totalWeightedFlexShrink to doubles.  Should be safe.  It'll use a tiny bit more memory and be a tiny bit slower.

2) Rework the code to compute the flex-grow and weight flex-shrink value during each iteration of resolveFlexibleLengths.  I think the rounding problem is because we assume that adding and subtracting the same value will produce the same value.

3) Just check in the test case and hope that the denominator stays at 64 which causes no loss of precision.

[Ojan Vafai]

I'm ok with 1 or 3. If we do 3, we should checkin the testcase with a clear description of what it's testing so that it'll hit an assert if we change back to 60.

I'm not a huge fan of 2.

[Ojan Vafai]

Filed bug 97827 for the preferredMainAxisContentExtentForChild issue.

[Tony Chang]

Created attachment 166096 [details]
Patch

[Tony Chang]

Sorry, I'm stealing this bug.

[Dave Barton]

(In reply to comment #16)
> 1) We could switch totalFlexGrow and totalWeightedFlexShrink to doubles.  Should be safe.  It'll use a tiny bit more memory and be a tiny bit slower.
> 
> 2) Rework the code to compute the flex-grow and weight flex-shrink value during each iteration of resolveFlexibleLengths.  I think the rounding problem is because we assume that adding and subtracting the same value will produce the same value.
> 
> 3) Just check in the test case and hope that the denominator stays at 64 which causes no loss of precision.

I don't think (3) is enough. If the user has decimal flex-grow or flex-shrink values, then adding & subtracting such floats in different orders may not cancel exactly to 0. It does seem like (1) would cure this problem, at least if the LayoutUnit denominator stays at 64. Maybe it would be safer to also change the ASSERT to totalWeightedFlexShrink >= -1e-8 and later totalWeightedFlexShrink > 0 comparison to totalWeightedFlexShrink > 1e-8, or something like that? It seems like you can allow for negligibly small totalWeightedFlexShrink values, and just treat them as 0. (I haven't thought this through completely, and don't really know your flexbox code.)

[WebKit Review Bot]

Comment on attachment 166096 [details]
Patch

Rejecting attachment 166096 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 9
Updating OpenSource
From http://git.chromium.org/external/Webkit
   82bc0a1..8e72d08  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 129817 = 875c689a27a358ee4b1b720bf25d701e2eb82525
last_rev is higher!: 129817 >= 129802 at /usr/lib/git-core/git-svn line 1523

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/14062050

[Tony Chang]

Committed r129914: <http://trac.webkit.org/changeset/129914>