| Summary: | Nullopt in RenderFlexibleBox::computeInnerFlexBaseSizeForChild via RenderFlexibleBox::layoutFlexItems | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||
| Component: | Layout and Rendering | Assignee: | Rob Buis <rbuis> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | bfulgham, cgarcia, ews-feeder, fred.wang, gpoo, koivisto, product-security, rbuis, simon.fraser, svillar, webkit-bug-importer, zalan | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=226367 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Ryosuke Niwa
2021-06-08 16:53:46 PDT
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]. |