Summary: | [css-flexbox] Flex item construction may affect sibling flex item height computation | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | zalan <zalan> | ||||||||||||||||
Component: | Layout and Rendering | Assignee: | zalan <zalan> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | bfulgham, cgarcia, changseok, clopez, commit-queue, esprehn+autocc, ews-watchlist, glenn, gsnedders, kondapallykalyan, pdr, simon.fraser, svillar, youennf, zalan | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 225618 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
zalan
2021-05-06 15:37:48 PDT
Created attachment 427962 [details]
Patch
Comment on attachment 427962 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427962&action=review Good catch! I think we can fix it in a slightly better way though. The actual issue IMO is that we're updating m_hasDefiniteHeight when we should not. That is controlled by the infamous m_inLayout bool which has been quite confusing (I added some bugs due to it in the past) and with a superwide scope. My proposal to fix this issue would be: 1- Remove the m_inLayout handling from ::layoutBlock() 2- Rename m_inLayout to m_inFlexItemsConstruction for example (should be only used in canComputePercentageFlexBasis()) 3- Wrap the loop that creates the flex items with a pair of braces to define a new scope and something like: SetForScope<bool> inFlexItemConstruction(m_inFlexItemsConstruction, true); 4- Remove the m_hasDefiniteHeight = SizeDefiniteness::Unknown; from ::layoutFlexItems as it won't be needed (it's properly reset by the end of layoutBlock()) Does it make sense? > LayoutTests/ChangeLog:9 > + * fast/flexbox/flex-column-with-percent-height-descendants.html: Added. Nice test, WDYT about uploading it to the WPT directory instead? I think it'd serve well to all the other engines. (In reply to Sergio Villar Senin from comment #2) > Comment on attachment 427962 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427962&action=review > > Good catch! > > I think we can fix it in a slightly better way though. The actual issue IMO > is that we're updating m_hasDefiniteHeight when we should not. Right, that's why the reset hack is right here after the constructFlexItem call (and the comment trying to explain it). In this patch I was just fixing the hack. I was thinking of removing it but since this part of the code has been the source of regressions lately, I decided not to. But you are correct, we should do that and in case of a new regression we could always fall back to this version. The _actual_actual_ issue here is the way we handle the percent height resolution (gOverridingContainingBlockContentLogicalHeightMap) and that we have to scope certain calls to _not_ update certain states indirectly. This is due to the fact that in rendering we call the containing block chain for constraints (in LFC, the constrains are pushed down to the geometry functions, so whenever we try to resolve some property (width/height etc) the constraints are readily available). > controlled by the infamous m_inLayout bool which has been quite confusing (I > added some bugs due to it in the past) and with a superwide scope. My > proposal to fix this issue would be: > 1- Remove the m_inLayout handling from ::layoutBlock() > 2- Rename m_inLayout to m_inFlexItemsConstruction for example (should be > only used in canComputePercentageFlexBasis()) > 3- Wrap the loop that creates the flex items with a pair of braces to define > a new scope and something like: > SetForScope<bool> inFlexItemConstruction(m_inFlexItemsConstruction, true); > 4- Remove the m_hasDefiniteHeight = SizeDefiniteness::Unknown; from > ::layoutFlexItems as it won't be needed (it's properly reset by the end of > layoutBlock()) > > Does it make sense? Yeah, I am happy to remove m_inLayout. It does not do any good. Thanks for the review. Created attachment 428002 [details]
Patch
Comment on attachment 428002 [details]
Patch
Nice!. Still think the test makes sense in WPT.
(In reply to zalan from comment #3) > > I think we can fix it in a slightly better way though. The actual issue IMO > > is that we're updating m_hasDefiniteHeight when we should not. > Right, that's why the reset hack is right here after the constructFlexItem > call (and the comment trying to explain it). > In this patch I was just fixing the hack. I was thinking of removing it but > since this part of the code has been the source of regressions lately, I > decided not to. > But you are correct, we should do that and in case of a new regression we > could always fall back to this version. > The _actual_actual_ issue here is the way we handle the percent height > resolution (gOverridingContainingBlockContentLogicalHeightMap) and that we > have to scope certain calls to _not_ update certain states indirectly. This > is due to the fact that in rendering we call the containing block chain for > constraints (in LFC, the constrains are pushed down to the geometry > functions, so whenever we try to resolve some property (width/height etc) > the constraints are readily available). That's nice, I guess it'd make the computation of definite/indefinite sizes much easier. And if those constrains could be set at will that would be a massive win for layout systems like grid or flex, where the container sets specific constrains (the overriding sizes). (In reply to Sergio Villar Senin from comment #5) > Comment on attachment 428002 [details] > Patch > > Nice!. Still think the test makes sense in WPT. Oh right. I totally forgot about that. Thanks for reminding me (and thanks for the review) Created attachment 428055 [details]
Patch
Created attachment 428056 [details]
Patch
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess Sam, could you help me figure out the process here (re: WPT)? (In reply to EWS Watchlist from comment #10) > This patch modifies the imported WPT tests. Please ensure that any changes > on the tests (not coming from a WPT import) are exported to WPT. Please see > https://trac.webkit.org/wiki/WPTExportProcess oh there it is. (In reply to zalan from comment #12) > (In reply to EWS Watchlist from comment #10) > > This patch modifies the imported WPT tests. Please ensure that any changes > > on the tests (not coming from a WPT import) are exported to WPT. Please see > > https://trac.webkit.org/wiki/WPTExportProcess > oh there it is. > 6. Land the PR in WPT > 7. Finally land the patch in WebKit 6 needs to happen first? alright, I am going to land the original patch. will deal with WPT later. ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. (In reply to zalan from comment #14) > alright, I am going to land the original patch. will deal with WPT later. The export to WPT is unreliable in my experience. What I normally do is to create a PR in WPT and after it's merged then I import the tests in WebKit using import-w3c-tests. It's a bit clumsy but it works. Created attachment 428069 [details]
[fast-cq] Patch
(In reply to Sergio Villar Senin from comment #16) > (In reply to zalan from comment #14) > > alright, I am going to land the original patch. will deal with WPT later. > > The export to WPT is unreliable in my experience. What I normally do is to > create a PR in WPT and after it's merged then I import the tests in WebKit > using import-w3c-tests. It's a bit clumsy but it works. Thanks for the heads up. Will create a PR for this. and thanks for the review again. Committed r277222 (237493@main): <https://commits.webkit.org/237493@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 428069 [details]. I'm getting a crash after this change in matrix web application with epiphany. It's a release assert, see the bt: #0 0x00007f580845cc2e in WTFCrash () from /home/cgarcia/gnome/lib/libjavascriptcoregtk-4.1.so.0 #1 0x00007f580bcb7305 in WebCore::RenderFlexibleBox::computeInnerFlexBaseSizeForChild(WebCore::RenderBox&, WebCore::LayoutUnit) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #2 0x00007f580bcc6463 in WebCore::RenderFlexibleBox::constructFlexItems(bool) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #3 0x00007f580bcc7729 in WebCore::RenderFlexibleBox::layoutFlexItems(bool) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #4 0x00007f580bcc8067 in WebCore::RenderFlexibleBox::layoutBlock(bool, WebCore::LayoutUnit) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #5 0x00007f580bc201ea in WebCore::RenderBlock::layout() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #6 0x00007f580bcc372f in WebCore::RenderFlexibleBox::layoutAndPlaceChildren(WebCore::LayoutUnit&, WTF::Vector<WebCore::FlexItem, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::LayoutUnit, bool, WTF::Vector<WebCore::RenderFlexibleBox::LineContext, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::LayoutUnit) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #7 0x00007f580bcc79ba in WebCore::RenderFlexibleBox::layoutFlexItems(bool) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #8 0x00007f580bcc8067 in WebCore::RenderFlexibleBox::layoutBlock(bool, WebCore::LayoutUnit) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #9 0x00007f580bc201ea in WebCore::RenderBlock::layout() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #10 0x00007f580bcc372f in WebCore::RenderFlexibleBox::layoutAndPlaceChildren(WebCore::LayoutUnit&, WTF::Vector<WebCore::FlexItem, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::LayoutUnit, bool, WTF::Vector<WebCore::RenderFlexibleBox::LineContext, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::LayoutUnit) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #11 0x00007f580bcc79ba in WebCore::RenderFlexibleBox::layoutFlexItems(bool) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #12 0x00007f580bcc8067 in WebCore::RenderFlexibleBox::layoutBlock(bool, WebCore::LayoutUnit) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #13 0x00007f580bc201ea in WebCore::RenderBlock::layout() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #14 0x00007f580bcc372f in WebCore::RenderFlexibleBox::layoutAndPlaceChildren(WebCore::LayoutUnit&, WTF::Vector<WebCore::FlexItem, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::LayoutUnit, bool, WTF::Vector<WebCore::RenderFlexibleBox::LineContext, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::LayoutUnit) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #15 0x00007f580bcc79ba in WebCore::RenderFlexibleBox::layoutFlexItems(bool) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #16 0x00007f580bcc8067 in WebCore::RenderFlexibleBox::layoutBlock(bool, WebCore::LayoutUnit) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #17 0x00007f580bc201ea in WebCore::RenderBlock::layout() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #18 0x00007f580bcc372f in WebCore::RenderFlexibleBox::layoutAndPlaceChildren(WebCore::LayoutUnit&, WTF::Vector<WebCore::FlexItem, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::LayoutUnit, bool, WTF::Vector<WebCore::RenderFlexibleBox::LineContext, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::LayoutUnit) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #19 0x00007f580bcc79ba in WebCore::RenderFlexibleBox::layoutFlexItems(bool) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #20 0x00007f580bcc8067 in WebCore::RenderFlexibleBox::layoutBlock(bool, WebCore::LayoutUnit) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #21 0x00007f580bc201ea in WebCore::RenderBlock::layout() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #22 0x00007f580bc42b2f in WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #23 0x00007f580bc467dd in WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #24 0x00007f580bc4979d in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #25 0x00007f580bc201ea in WebCore::RenderBlock::layout() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #26 0x00007f580bc42b2f in WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #27 0x00007f580bc467dd in WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #28 0x00007f580bc4979d in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #29 0x00007f580bc201ea in WebCore::RenderBlock::layout() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #30 0x00007f580bc42b2f in WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #31 0x00007f580bc467dd in WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #32 0x00007f580bc4979d in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #33 0x00007f580bc201ea in WebCore::RenderBlock::layout() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #34 0x00007f580bc42b2f in WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 --Type <RET> for more, q to quit, c to continue without paging-- #35 0x00007f580bc467dd in WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #36 0x00007f580bc4979d in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #37 0x00007f580bc201ea in WebCore::RenderBlock::layout() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #38 0x00007f580bdff371 in WebCore::RenderView::layout() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #39 0x00007f580b87648f in WebCore::FrameViewLayoutContext::layout() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #40 0x00007f580b197082 in WebCore::Document::updateLayout() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #41 0x00007f580b197892 in WebCore::Document::updateLayoutIgnorePendingStylesheets(WebCore::Document::RunPostLayoutTasks) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #42 0x00007f580b1b38b8 in WebCore::Element::scrollTop() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #43 0x00007f580a368611 in WebCore::jsElement_scrollTop(JSC::JSGlobalObject*, long, JSC::PropertyName) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.1.so.0 #44 0x00007f580816d07c in JSC::PropertySlot::customGetter(JSC::JSGlobalObject*, JSC::PropertyName) const () from /home/cgarcia/gnome/lib/libjavascriptcoregtk-4.1.so.0 #45 0x00007f5807d96e3b in JSC::LLInt::performLLIntGetByID(JSC::Instruction const*, JSC::CodeBlock*, JSC::JSGlobalObject*, JSC::JSValue, JSC::Identifier const&, JSC::GetByIdModeMetadata&) () from /home/cgarcia/gnome/lib/libjavascriptcoregtk-4.1.so.0 #46 0x00007f5807d97c34 in llint_slow_path_get_by_id () from /home/cgarcia/gnome/lib/libjavascriptcoregtk-4.1.so.0 #47 0x00007f580726842a in llint_op_get_by_id () from /home/cgarcia/gnome/lib/libjavascriptcoregtk-4.1.so.0 #48 0x00007f579020cae0 in ?? () It sounds like some code was relying on incorrect state before. Any chance of having the test content/reduction? Thanks. (In reply to zalan from comment #21) > It sounds like some code was relying on incorrect state before. Any chance > of having the test content/reduction? Thanks. I'll try to get more data, as it crashes in an internal Igalia url (In reply to Sergio Villar Senin from comment #22) > (In reply to zalan from comment #21) > > It sounds like some code was relying on incorrect state before. Any chance > > of having the test content/reduction? Thanks. > > I'll try to get more data, as it crashes in an internal Igalia url Thanks. Appreciate it! (In reply to zalan from comment #23) > (In reply to Sergio Villar Senin from comment #22) > > (In reply to zalan from comment #21) > > > It sounds like some code was relying on incorrect state before. Any chance > > > of having the test content/reduction? Thanks. > > > > I'll try to get more data, as it crashes in an internal Igalia url > Thanks. Appreciate it! I cannot easily get a test case, but I think I know where the problem comes from. It's basically caused by the m_inLayout removal. canComputePercentageFlexBasis() assumes now (after the patch for this bug) that if we are not computing the flex base size then it's because we're laying out ourselves and thus we should update the m_hasDefiniteHeight value. That's clearly wrong because canComputePercentageFlexBasis() can be called from useChildOverridingMainSizeForPercentageResolution() which can be called at any time by availableLogicalHeightUsing(). Updating the m_hasDefiniteHeight while we are not laying out ourselves could easily lead to false positives. If that happens then the childMainSizeIsDefinite() would return true for flex containers where the main size is the child block axis so when executing if (childMainSizeIsDefinite(child, flexBasis)) return std::max(0_lu, computeMainAxisExtentForChild(child, MainOrPreferredSize, flexBasis).value()); then it'll crash because computeMainAxisExtentForChild() could return nullopt. That must not happen because we have checked that childMainSizeIsDefinite(), but as we have seen, a wrong m_hasDefiniteHeight could deceive us. Re-opened since this is blocked by bug 225618 Created attachment 428280 [details]
Test reduction for the crash after the fix
so, right, the issue with the patch is that the m_inFlexItemConstruction flag does not exactly cover the !m_inLayout cases and we end up setting the m_hasDefiniteHeight when !m_inFlexItemConstruction but also not in layout. I'll go with my original patch. Created attachment 428301 [details]
Patch
Could you check if you are still seeing the crash with this patch applied, please? (In reply to zalan from comment #29) > Could you check if you are still seeing the crash with this patch applied, > please? No crashes here. Thanks! Great! Thanks for confirming it! Comment on attachment 428301 [details]
Patch
Let's see if we can eventually manage to get rid of m_inLayout in the future. In the meantime let's make it work correctly at least.
(In reply to Sergio Villar Senin from comment #32) > Comment on attachment 428301 [details] > Patch > > Let's see if we can eventually manage to get rid of m_inLayout in the > future. In the meantime let's make it work correctly at least. Sounds like a plan. :) Committed r277435 (237683@main): <https://commits.webkit.org/237683@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 428301 [details]. |