Bug 207603 – Crash in RenderFlexibleBox::computeInnerFlexBaseSizeForChild due to nullopt optional

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

[Ryosuke Niwa]

e.g.

WebKitBuild/Debug/usr/local/include/wtf/Optional.h(549) : T &&WTF::Optional<WebCore::LayoutUnit>::value() && [T = WebCore::LayoutUnit]
1   0x57cad4479 WTFCrash
2   0x5642bfb59 WTF::Optional<WebCore::LayoutUnit>::value() &&
3   0x564ea81a8 WebCore::RenderFlexibleBox::computeInnerFlexBaseSizeForChild(WebCore::RenderBox&, WebCore::LayoutUnit, bool)
4   0x564ea86ba WebCore::RenderFlexibleBox::constructFlexItem(WebCore::RenderBox&, bool)
5   0x564ea48ba WebCore::RenderFlexibleBox::layoutFlexItems(bool)
6   0x564ea43c4 WebCore::RenderFlexibleBox::layoutBlock(bool, WebCore::LayoutUnit)
7   0x564dda609 WebCore::RenderBlock::layout()
8   0x564d269bc WebCore::RenderElement::layoutIfNeeded()
9   0x564df9860 WebCore::RenderBlockFlow::insertFloatingObject(WebCore::RenderBox&)
10  0x565114b2e WebCore::LineBreaker::skipLeadingWhitespace(WebCore::BidiResolverWithIsolate<WebCore::InlineIterator, WebCore::BidiRun, WebCore::BidiIsolatedRun>&, WebCore::LineInfo&, WebCore::FloatingObject*, WebCore::LineWidth&)
11  0x565114f06 WebCore::LineBreaker::nextLineBreak(WebCore::BidiResolverWithIsolate<WebCore::InlineIterator, WebCore::BidiRun, WebCore::BidiIsolatedRun>&, WebCore::LineInfo&, WebCore::RenderTextInfo&, WebCore::FloatingObject*, unsigned int, WTF::Vector<WebCore::WordMeasurement, 64ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&)
12  0x564d21886 WebCore::ComplexLineLayout::layoutRunsAndFloatsInRange(WebCore::LineLayoutState&, WebCore::BidiResolverWithIsolate<WebCore::InlineIterator, WebCore::BidiRun, WebCore::BidiIsolatedRun>&, WebCore::InlineIterator const&, WebCore::BidiStatus const&, unsigned int)
13  0x564d20211 WebCore::ComplexLineLayout::layoutRunsAndFloats(WebCore::LineLayoutState&, bool)
14  0x564d261a9 WebCore::ComplexLineLayout::layoutLineBoxes(bool, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
15  0x564df81be WebCore::RenderBlockFlow::layoutInlineChildren(bool, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
16  0x564df71c9 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
17  0x564dda609 WebCore::RenderBlock::layout()
18  0x564df9dc2 WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
19  0x564df8434 WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&)
20  0x564df71e6 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
21  0x564dda609 WebCore::RenderBlock::layout()
22  0x564df9dc2 WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
23  0x564df8434 WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&)
24  0x564df71e6 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
25  0x564dda609 WebCore::RenderBlock::layout()
26  0x564df9dc2 WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
27  0x564df8434 WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&)
28  0x564df71e6 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
29  0x564dda609 WebCore::RenderBlock::layout()
30  0x5650a7e13 WebCore::RenderView::layout()


<rdar://problem/59135373>

[Ryosuke Niwa]

Created attachment 390470 [details]
Test case (unreduced)

[Eugene But]

Crashes inside RenderFlexibleBox::computeInnerFlexBaseSizeForChild method, because computeMainAxisExtentForChild returns Optional without value: 

if (mainAxisLengthIsDefinite(child, flexBasis))
  return std::max(0_lu, computeMainAxisExtentForChild(child, MainOrPreferredSize, flexBasis).value());

This is the only Optional::value call inside the file. All other calls are Optional::valueOr. Perhaps calling valueOr() would be the right fix, but I will also try to figure out why returned Optional does not have a value.

[Eugene But]

Created attachment 396033 [details]
Test case (reduced)

[Eugene But]

Here is what happens:

1. computeInnerFlexBaseSizeForChild is called for HTMLObjectElement child
2. mainAxisLengthIsDefinite returns true, because child's flex basis is percent and flexible box has definite height
3. computeMainAxisExtentForChild calls computeContentLogicalHeight which returns null, so computeMainAxisExtentForChild returns null

if #2 and #3 are correct, then calling valueOr(0) on computeMainAxisExtentForChild's result seems to be the right fix. 
I don't think I have enough knowledge to argue that #2 and #3 are correct though.

[Eugene But]

Chrome/Blink sizes this as fit-content instead of max-content:

https://chromium-review.googlesource.com/c/chromium/src/+/1769536/12/third_party/blink/renderer/core/layout/layout_flexible_box.cc

I will test if the same approach works in WebKit next week.

[zalan]

(In reply to Eugene But from comment #5)
> Chrome/Blink sizes this as fit-content instead of max-content:
> 
> https://chromium-review.googlesource.com/c/chromium/src/+/1769536/12/
> third_party/blink/renderer/core/layout/layout_flexible_box.cc
> 
> I will test if the same approach works in WebKit next week.
Thanks for the test reduction! I briefly looked into it and this might be one of the unspecified cases where there's a circular dependency between the child and the containing block sizing (e.g. shrink to fit vs. percentage width) In that case a .valueOr(0) would be sufficient. Will look into it a bit more tomorrow.

[zalan]

(In reply to zalan from comment #6)
> (In reply to Eugene But from comment #5)
> > Chrome/Blink sizes this as fit-content instead of max-content:
> > 
> > https://chromium-review.googlesource.com/c/chromium/src/+/1769536/12/
> > third_party/blink/renderer/core/layout/layout_flexible_box.cc
> > 
> > I will test if the same approach works in WebKit next week.
> Thanks for the test reduction! I briefly looked into it and this might be
> one of the unspecified cases where there's a circular dependency between the
> child and the containing block sizing (e.g. shrink to fit vs. percentage
> width) In that case a .valueOr(0) would be sufficient. Will look into it a
> bit more tomorrow.
<style>
.flexBox {
    display: -webkit-flex;
    width: min-content;
    writing-mode: vertical-lr;
    -webkit-flex-direction: column-reverse;    
}
.flexItem {
    flex: 0 0 50%;
    text-anchor: middle; 
}
</style>
<div class=flexBox><input class=flexItem><div class=flexItem></div></div>

We seems to set the m_hasDefiniteHeight to true incorrectly when laying out the <input> (since input reports horizontal writing mode here and RenderBox::computePercentageLogicalHeight returns with a non-null value (the percentage height is resolved against the containing block width here). Now when we get to the <div> we believe the axis length is set (height in this case) so we should be fine with resolving the percentage hight -but we are really not because the containing block's height is not computed yet (again, the width is).

[zalan]

Created attachment 396340 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 396340 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396340&action=review

> Source/WebCore/ChangeLog:10
> +        because perpendicular boxes's height are resolved against the containing block's width.

a perpendicular box's height is resolved

> Source/WebCore/rendering/RenderFlexibleBox.cpp:771
> +        // Perpendicular children's height are resolved against the containing block's width which is not the main axis.

The height of a perpendicular child is resolved

[zalan]

Created attachment 396341 [details]
Patch

[EWS]

Committed r260055: <https://trac.webkit.org/changeset/260055>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396341 [details].