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>
Reproduced with non-ASAN release build of WebKitTestRunner at r278627.
Created attachment 430943 [details] Patch
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?
Created attachment 430953 [details] Patch
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.
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.
(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.
Created attachment 431067 [details] Patch
Comment on attachment 431067 [details] Patch Looks good provided the EWS are happy
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].