Bug 226790 – Nullopt in RenderFlexibleBox::computeInnerFlexBaseSizeForChild via RenderFlexibleBox::layoutFlexItems

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

[Ryosuke Niwa]

Created attachment 430911 [details]
Test

e.g.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
1   libsystem_pthread.dylib       	0x00007fff20686615 pthread_kill + 263
2   libsystem_c.dylib             	0x00007fff205db411 abort + 120
3   com.apple.WebCore             	0x0000000270044229 std::__1::__throw_bad_optional_access() + 9 (optional:193)
4   com.apple.WebCore             	0x0000000271a97cbd std::__1::optional<WebCore::LayoutUnit>::value() && + 5 (optional:965) [inlined]
5   com.apple.WebCore             	0x0000000271a97cbd WebCore::RenderFlexibleBox::computeInnerFlexBaseSizeForChild(WebCore::RenderBox&, WebCore::LayoutUnit) + 813 (RenderFlexibleBox.cpp:948)
6   com.apple.WebCore             	0x0000000271a983d6 WebCore::RenderFlexibleBox::constructFlexItem(WebCore::RenderBox&, bool) + 422 (RenderFlexibleBox.cpp:1335)
7   com.apple.WebCore             	0x0000000271a944c2 WebCore::RenderFlexibleBox::layoutFlexItems(bool) + 274 (RenderFlexibleBox.cpp:990)
8   com.apple.WebCore             	0x0000000271a94013 WebCore::RenderFlexibleBox::layoutBlock(bool, WebCore::LayoutUnit) + 851 (RenderFlexibleBox.cpp:307)
9   com.apple.WebCore             	0x0000000271a1e2da WebCore::RenderBlock::layout() + 42 (RenderBlock.cpp:596)
10  com.apple.WebCore             	0x0000000271a3f496 WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) + 982 (RenderBlockFlow.cpp:764)
11  com.apple.WebCore             	0x0000000271a3dc78 WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) + 632 (RenderBlockFlow.cpp:675)
12  com.apple.WebCore             	0x0000000271a3cc90 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) + 976 (RenderBlockFlow.cpp:527)
13  com.apple.WebCore             	0x0000000271a1e2da WebCore::RenderBlock::layout() + 42 (RenderBlock.cpp:596)
14  com.apple.WebCore             	0x0000000271a3f496 WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) + 982 (RenderBlockFlow.cpp:764)
15  com.apple.WebCore             	0x0000000271a3dc78 WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) + 632 (RenderBlockFlow.cpp:675)
16  com.apple.WebCore             	0x0000000271a3cc90 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) + 976 (RenderBlockFlow.cpp:527)
17  com.apple.WebCore             	0x0000000271a1e2da WebCore::RenderBlock::layout() + 42 (RenderBlock.cpp:596)
18  com.apple.WebCore             	0x0000000271b80e93 WebCore::RenderView::layout() + 611 (RenderView.cpp:185)
19  com.apple.WebCore             	0x00000002716c45f9 WebCore::FrameViewLayoutContext::layout() + 953 (FrameViewLayoutContext.cpp:233)
20  com.apple.WebCore             	0x00000002710caea1 WebCore::Document::implicitClose() + 849 (Document.cpp:3188)
21  com.apple.WebCore             	0x00000002715dc367 WebCore::FrameLoader::checkCompleted() + 391 (FrameLoader.cpp:881)
22  com.apple.WebCore             	0x00000002715dace3 WebCore::FrameLoader::finishedParsing() + 115 (FrameLoader.cpp:786)
23  com.apple.WebCore             	0x00000002710dacf0 WebCore::Document::finishedParsing() + 368 (Document.cpp:6061)
24  com.apple.WebCore             	0x000000027141ab66 WebCore::HTMLDocumentParser::end() + 12 (HTMLDocumentParser.cpp:449) [inlined]
25  com.apple.WebCore             	0x000000027141ab66 WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd() + 33 (HTMLDocumentParser.cpp:458) [inlined]
26  com.apple.WebCore             	0x000000027141ab66 WebCore::HTMLDocumentParser::prepareToStopParsing() + 246 (HTMLDocumentParser.cpp:152)
27  com.apple.WebCore             	0x000000027141c172 WebCore::HTMLDocumentParser::finish() + 114 (HTMLDocumentParser.cpp:498)
28  com.apple.WebCore             	0x00000002715b80dc WebCore::DocumentWriter::end() + 76 (DocumentWriter.cpp:294)
29  com.apple.WebCore             	0x00000002715b70c6 WebCore::DocumentLoader::finishedLoading() + 390 (DocumentLoader.cpp:488)

<rdar://78762842>

[Ryosuke Niwa]

Reproduced with non-ASAN release build of WebKitTestRunner at r278627.

[Rob Buis]

Created attachment 430943 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 430943 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        childMainSizeIsDefinite.

One line is OK.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:867
> +    if (flexBasis.type() == LengthType::Intrinsic)

I think we should change this to check isLegacyIntrinsic() and then add the proper test case for min-intrinsic, because I'b bet it'll crash the same way.

> LayoutTests/ChangeLog:12
> +        * fast/css/flex-box-intrinsic-width-crash.html:

There is fast/flexbox, could you move the test there?

> LayoutTests/fast/css/flex-box-intrinsic-width-column-flow-crash.html:1
> +<script>

Can we add a <!DOCTYPE html> ?

> LayoutTests/fast/css/flex-box-intrinsic-width-crash.html:14
> +<body>

Do we need <body> here?

[Rob Buis]

Created attachment 430953 [details]
Patch

[Rob Buis]

Comment on attachment 430943 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        childMainSizeIsDefinite.
> 
> One line is OK.

Done.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:867
>> +    if (flexBasis.type() == LengthType::Intrinsic)
> 
> I think we should change this to check isLegacyIntrinsic() and then add the proper test case for min-intrinsic, because I'b bet it'll crash the same way.

Actually for min-intrinsic there is no crash.

>> LayoutTests/ChangeLog:12
>> +        * fast/css/flex-box-intrinsic-width-crash.html:
> 
> There is fast/flexbox, could you move the test there?

Done.

>> LayoutTests/fast/css/flex-box-intrinsic-width-column-flow-crash.html:1
>> +<script>
> 
> Can we add a <!DOCTYPE html> ?

Done.

>> LayoutTests/fast/css/flex-box-intrinsic-width-crash.html:14
>> +<body>
> 
> Do we need <body> here?

It is needed to cause the crash, yes.

[Rob Buis]

Comment on attachment 430943 [details]
Patch

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

>>> Source/WebCore/rendering/RenderFlexibleBox.cpp:867
>>> +    if (flexBasis.type() == LengthType::Intrinsic)
>> 
>> I think we should change this to check isLegacyIntrinsic() and then add the proper test case for min-intrinsic, because I'b bet it'll crash the same way.
> 
> Actually for min-intrinsic there is no crash.

I looked a bit more into it. For both intrinsic and min-intrinsic computeInnerFlexBaseSizeForChild calls computeMainAxisExtentForChild. In this test case this ends up calling child.computeContentLogicalHeight (because of !mainAxisIsChildInlineAxis) check. Then computeContentLogicalHeight, or rather computeContentAndScrollbarLogicalHeightUsing, acts differently for these two. For min-intrinsic computeIntrinsicLogicalContentHeightUsing is called but for intrinsic std::nullopt is returned, causing the crash later when we call value() on the returned value. Because we use computeIntrinsicLogicalContentHeightUsing, we pass an intrinsic height, so the min-intrinsic will resolve against that and return some non-null value, avoiding the crash.

[Sergio Villar Senin]

(In reply to Rob Buis from comment #6)
> Comment on attachment 430943 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430943&action=review
> 
> >>> Source/WebCore/rendering/RenderFlexibleBox.cpp:867
> >>> +    if (flexBasis.type() == LengthType::Intrinsic)
> >> 
> >> I think we should change this to check isLegacyIntrinsic() and then add the proper test case for min-intrinsic, because I'b bet it'll crash the same way.
> > 
> > Actually for min-intrinsic there is no crash.
> 
> I looked a bit more into it. For both intrinsic and min-intrinsic
> computeInnerFlexBaseSizeForChild calls computeMainAxisExtentForChild. In
> this test case this ends up calling child.computeContentLogicalHeight
> (because of !mainAxisIsChildInlineAxis) check. Then
> computeContentLogicalHeight, or rather
> computeContentAndScrollbarLogicalHeightUsing, acts differently for these
> two. For min-intrinsic computeIntrinsicLogicalContentHeightUsing is called
> but for intrinsic std::nullopt is returned, causing the crash later when we
> call value() on the returned value. Because we use
> computeIntrinsicLogicalContentHeightUsing, we pass an intrinsic height, so
> the min-intrinsic will resolve against that and return some non-null value,
> avoiding the crash.

Right, IIRC you added a patch recently that was making computeIntrinsicLogicalContentHeightUsing() treat minintrinsic as the other content sized cases. I wonder why we don't do the same here. I guess you could replace that check by isLegacyIntrinsic() in RenderBox and then you could use it as well in RenderFlexibleBox.

[Rob Buis]

Created attachment 431067 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 431067 [details]
Patch

Looks good provided the EWS are happy

[EWS]

Committed r278705 (238676@main): <https://commits.webkit.org/238676@main>

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