Bug 97606 - flexbox assert fails with auto-sized item with padding
Summary: flexbox assert fails with auto-sized item with padding
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on:
Blocks: 62048 96960
  Show dependency treegraph
 
Reported: 2012-09-25 14:24 PDT by Dave Barton
Modified: 2012-09-28 10:14 PDT (History)
6 users (show)

See Also:


Attachments
web page that makes totalWeightedFlexShrink < 0 (580 bytes, text/html)
2012-09-25 14:24 PDT, Dave Barton
no flags Details
crash log (13.12 KB, text/plain)
2012-09-25 14:27 PDT, Dave Barton
no flags Details
test case without -webkit-line-box-contain (679 bytes, text/html)
2012-09-27 11:31 PDT, Dave Barton
no flags Details
Patch (8.62 KB, patch)
2012-09-27 16:51 PDT, Tony Chang
ojan: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Barton 2012-09-25 14:24:50 PDT
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);//@@@
Comment 1 Dave Barton 2012-09-25 14:27:29 PDT
Created attachment 165675 [details]
crash log
Comment 2 Ojan Vafai 2012-09-25 14:47:18 PDT
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.
Comment 3 Kenneth Rohde Christiansen 2012-09-26 13:25:21 PDT
I will have a look at this one tomorrow morning, thanks!
Comment 4 Dave Barton 2012-09-26 14:15:02 PDT
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.
Comment 5 Kenneth Rohde Christiansen 2012-09-27 04:48:05 PDT
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?
Comment 6 Kenneth Rohde Christiansen 2012-09-27 06:11:16 PDT
> 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.
Comment 7 Dave Barton 2012-09-27 11:31:39 PDT
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?
Comment 8 Dave Barton 2012-09-27 11:49:00 PDT
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.
Comment 9 Kenneth Rohde Christiansen 2012-09-27 11:51:26 PDT
(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.
Comment 10 Dave Barton 2012-09-27 12:17:16 PDT
(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.
Comment 11 Dave Barton 2012-09-27 12:21:57 PDT
(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.
Comment 12 Tony Chang 2012-09-27 14:59:08 PDT
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 . . .
Comment 13 Dave Barton 2012-09-27 15:42:01 PDT
(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?
Comment 14 Dave Barton 2012-09-27 15:46:51 PDT
Wow. kFixedPointDenominator has been recently changed from 60 to 64. This explains a lot.
Comment 15 Tony Chang 2012-09-27 15:47:43 PDT
(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.
Comment 16 Tony Chang 2012-09-27 15:58:03 PDT
(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.
Comment 17 Ojan Vafai 2012-09-27 16:00:05 PDT
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.
Comment 18 Ojan Vafai 2012-09-27 16:01:24 PDT
Filed bug 97827 for the preferredMainAxisContentExtentForChild issue.
Comment 19 Tony Chang 2012-09-27 16:51:03 PDT
Created attachment 166096 [details]
Patch
Comment 20 Tony Chang 2012-09-27 16:51:56 PDT
Sorry, I'm stealing this bug.
Comment 21 Dave Barton 2012-09-27 17:05:37 PDT
(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.)
Comment 22 WebKit Review Bot 2012-09-27 17:32:23 PDT
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
Comment 23 Tony Chang 2012-09-28 10:14:21 PDT
Committed r129914: <http://trac.webkit.org/changeset/129914>