Bug 222854 – Nullopt crash in RenderFlexibleBox::computeInnerFlexBaseSizeForChild

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

[Ryosuke Niwa]

e.g.

1   0x1a74a4089 WTFCrash
2   0x18cc53389 WTF::Optional<WebCore::LayoutUnit>::value() &&
3   0x18cd8df11 WebCore::RenderFlexibleBox::computeInnerFlexBaseSizeForChild(WebCore::RenderBox&, WebCore::LayoutUnit)
4   0x18cd8e784 WebCore::RenderFlexibleBox::constructFlexItem(WebCore::RenderBox&, bool)
5   0x18cd8989a WebCore::RenderFlexibleBox::layoutFlexItems(bool)
6   0x18cd893a4 WebCore::RenderFlexibleBox::layoutBlock(bool, WebCore::LayoutUnit)
7   0x18ccae529 WebCore::RenderBlock::layout()
8   0x18cc064cc WebCore::RenderElement::layoutIfNeeded()
9   0x18cd8e711 WebCore::RenderFlexibleBox::constructFlexItem(WebCore::RenderBox&, bool)
10  0x18cd8989a WebCore::RenderFlexibleBox::layoutFlexItems(bool)
11  0x18cd893a4 WebCore::RenderFlexibleBox::layoutBlock(bool, WebCore::LayoutUnit)
12  0x18ccae529 WebCore::RenderBlock::layout()
13  0x18ccc94c2 WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
14  0x18ccc7e04 WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&)
15  0x18ccc6c83 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
16  0x18ccae529 WebCore::RenderBlock::layout()
17  0x18cf472f3 WebCore::RenderView::layout()
18  0x18c4393c1 WebCore::FrameViewLayoutContext::layout()
19  0x18b6834b6 WebCore::Document::implicitClose()
20  0x18c232a7b WebCore::FrameLoader::checkCallImplicitClose()
21  0x18c2324aa WebCore::FrameLoader::checkCompleted()
22  0x18c230597 WebCore::FrameLoader::finishedParsing()
23  0x18b698026 WebCore::Document::finishedParsing()
24  0x18bdc2228 WebCore::HTMLConstructionSite::finishedParsing()
25  0x18be07f67 WebCore::HTMLTreeBuilder::finished()
26  0x18bdc9858 WebCore::HTMLDocumentParser::end()
27  0x18bdc7528 WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd()
28  0x18bdc7267 WebCore::HTMLDocumentParser::prepareToStopParsing()
29  0x18bdc98c2 WebCore::HTMLDocumentParser::attemptToEnd()
30  0x18bdc9989 WebCore::HTMLDocumentParser::finish()
31  0x18c20aaa4 WebCore::DocumentWriter::end()

<rdar://problem/72991927>

[Ryosuke Niwa]

Created attachment 422471 [details]
Test

[Ryosuke Niwa]

Reproduced with DumpRenderTree and WebKitTestRunner at r274025.

[Rob Buis]

Created attachment 422477 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 422477 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:933
> +        return std::max(0_lu, computeMainAxisExtentForChild(child, MainOrPreferredSize, flexBasis).valueOr(0_lu));

We're using value() unconditionally because this is the childMainSizeIsDefinite() branch. Why are we getting a nullopt here?

[Rob Buis]

Comment on attachment 422477 [details]
Patch

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

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:933
>> +        return std::max(0_lu, computeMainAxisExtentForChild(child, MainOrPreferredSize, flexBasis).valueOr(0_lu));
> 
> We're using value() unconditionally because this is the childMainSizeIsDefinite() branch. Why are we getting a nullopt here?

Because we are having a !mainAxisIsChildInlineAxis case, we can return a null opt in computeMainAxisExtentForChild:
        if (!height)
            return height;

[Carlos Garcia Campos]

*** Bug 222716 has been marked as a duplicate of this bug. ***

[Rob Buis]

Created attachment 422810 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 422810 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:3044
> +    if (height.isIntrinsic() || height.isMinIntrinsic())

In this case you can use height.isLegacyIntrinsic()

[Rob Buis]

Created attachment 422815 [details]
Patch

[Rob Buis]

Created attachment 422816 [details]
Patch

[Rob Buis]

Created attachment 422818 [details]
Patch

[Rob Buis]

Comment on attachment 422810 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:3044
>> +    if (height.isIntrinsic() || height.isMinIntrinsic())
> 
> In this case you can use height.isLegacyIntrinsic()

Alas, private :)

[Rob Buis]

Created attachment 422819 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 422819 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:3025
> +    if (logicalHeightLength.isMinContent() || logicalHeightLength.isMaxContent() || logicalHeightLength.isFitContent() || logicalHeightLength.isMinIntrinsic()) {

Now that you change this maybe we could modify a bit the if block because it's really confusing. I'm talking about doing something like

if (intrinsicContentHeight && style().boxSizing() == BoxSizing::BorderBox)
  return intrinsicContentHeight.value() + borderAndPaddingLogicalHeight();
return intrinsicContentHeight;

> LayoutTests/fast/flexbox/indefinite-width-crash.html:2
> +<html><head>

Nit: both not needed as you're already using DOCTYPE

> LayoutTests/fast/flexbox/indefinite-width-crash.html:10
> +  };

Can we just use 0 as delay instead of 50?

[Rob Buis]

Created attachment 423156 [details]
Patch

[Rob Buis]

Comment on attachment 422819 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:3025
>> +    if (logicalHeightLength.isMinContent() || logicalHeightLength.isMaxContent() || logicalHeightLength.isFitContent() || logicalHeightLength.isMinIntrinsic()) {
> 
> Now that you change this maybe we could modify a bit the if block because it's really confusing. I'm talking about doing something like
> 
> if (intrinsicContentHeight && style().boxSizing() == BoxSizing::BorderBox)
>   return intrinsicContentHeight.value() + borderAndPaddingLogicalHeight();
> return intrinsicContentHeight;

Done.

>> LayoutTests/fast/flexbox/indefinite-width-crash.html:2
>> +<html><head>
> 
> Nit: both not needed as you're already using DOCTYPE

Removed.

>> LayoutTests/fast/flexbox/indefinite-width-crash.html:10
>> +  };
> 
> Can we just use 0 as delay instead of 50?

Yes, that works too, fixed.

[EWS]

Committed r274419: <https://commits.webkit.org/r274419>

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