overridingContainingBlockContentLogicalHeight() and overridingContainingBlockContentLogicalWidth() return type is std::optional<LayoutUnit>, but we always try to access value().
Created attachment 443774 [details] Patch
<rdar://problem/82857623>
Comment on attachment 443774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443774&action=review > Source/WebCore/ChangeLog:8 > + Test: fast/css-grid-layout/grid-crash-update-auto-margins-row-axis.html Nit: the test line should be placed after the description paragraph. > Source/WebCore/ChangeLog:16 > + Nit extra line here. > Source/WebCore/rendering/RenderGrid.cpp:1226 > + std::optional<LayoutUnit> availableWidth = child.overridingContainingBlockContentLogicalWidth(); Although this seems correct I don't get why we have nullopt in the overriding size here. These two methods are called after calling updateGridAreaBreathLogicalSize() which always sets the overrides. That's why we were calling value() directly here without checking the presence of the value. Do you have more info about the exact conditions that trigger the bad access? > LayoutTests/fast/css-grid-layout/grid-crash-update-auto-margins-row-axis.html:3 > +<head> Nit, you don't need neither <html> nor <head> nor <body>
Comment on attachment 443774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443774&action=review > Source/WebCore/rendering/RenderGrid.cpp:1260 > + std::optional<LayoutUnit> availableHeight = child.overridingContainingBlockContentLogicalHeight(); Maybe we could just use value_or(LayoutUnit()) as we already do in other functions of this class to handle the available space. In case of nullopt, we will use the early return already defined for non-positive values.
Comment on attachment 443774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443774&action=review Thank you for the feedback on the stylistic issues. >> Source/WebCore/rendering/RenderGrid.cpp:1226 >> + std::optional<LayoutUnit> availableWidth = child.overridingContainingBlockContentLogicalWidth(); > > Although this seems correct I don't get why we have nullopt in the overriding size here. These two methods are called after calling updateGridAreaBreathLogicalSize() which always sets the overrides. That's why we were calling value() directly here without checking the presence of the value. Do you have more info about the exact conditions that trigger the bad access? I note that these values are set in updateGridAreaBreathLogicalSize(), but later cleared in applyStretchAlignmentToChildIfNeeded: // We clear height and width override values because we will decide now whether it's allowed or // not, evaluating the conditions which might have changed since the old values were set. child.clearOverridingLogicalHeight(); child.clearOverridingLogicalWidth(); >> Source/WebCore/rendering/RenderGrid.cpp:1260 >> + std::optional<LayoutUnit> availableHeight = child.overridingContainingBlockContentLogicalHeight(); > > Maybe we could just use value_or(LayoutUnit()) as we already do in other functions of this class to handle the available space. In case of nullopt, we will use the early return already defined for non-positive values. Thank you, I will use the recommended approach.
Created attachment 443968 [details] Patch
Comment on attachment 443774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443774&action=review >>> Source/WebCore/rendering/RenderGrid.cpp:1226 >>> + std::optional<LayoutUnit> availableWidth = child.overridingContainingBlockContentLogicalWidth(); >> >> Although this seems correct I don't get why we have nullopt in the overriding size here. These two methods are called after calling updateGridAreaBreathLogicalSize() which always sets the overrides. That's why we were calling value() directly here without checking the presence of the value. Do you have more info about the exact conditions that trigger the bad access? > > I note that these values are set in updateGridAreaBreathLogicalSize(), but later cleared in applyStretchAlignmentToChildIfNeeded: > > // We clear height and width override values because we will decide now whether it's allowed or > // not, evaluating the conditions which might have changed since the old values were set. > child.clearOverridingLogicalHeight(); > child.clearOverridingLogicalWidth(); No, those are different "override" values. Notice the function names are different.
(In reply to Javier Fernandez from comment #7) > Comment on attachment 443774 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=443774&action=review > > >>> Source/WebCore/rendering/RenderGrid.cpp:1226 > >>> + std::optional<LayoutUnit> availableWidth = child.overridingContainingBlockContentLogicalWidth(); > >> > >> Although this seems correct I don't get why we have nullopt in the overriding size here. These two methods are called after calling updateGridAreaBreathLogicalSize() which always sets the overrides. That's why we were calling value() directly here without checking the presence of the value. Do you have more info about the exact conditions that trigger the bad access? > > > > I note that these values are set in updateGridAreaBreathLogicalSize(), but later cleared in applyStretchAlignmentToChildIfNeeded: > > > > // We clear height and width override values because we will decide now whether it's allowed or > > // not, evaluating the conditions which might have changed since the old values were set. > > child.clearOverridingLogicalHeight(); > > child.clearOverridingLogicalWidth(); > > No, those are different "override" values. Notice the function names are > different. I am taking a closer look why the values are cleared in applyStretchAlignmentToChildIfNeeded.
Comment on attachment 443968 [details] Patch Looks obviously right as long as none of the heights we are subtracting are negative.
(In reply to Darin Adler from comment #9) > Comment on attachment 443968 [details] > Patch > > Looks obviously right as long as none of the heights we are subtracting are > negative. Wouldn't like to dismiss your review, but I talked with Gabriel in private that this might fix the crash but does not address the root cause which is having an std::nullopt overriding size at that point.
(In reply to Sergio Villar Senin from comment #10) > Wouldn't like to dismiss your review, but I talked with Gabriel in private > that this might fix the crash but does not address the root cause which is > having an std::nullopt overriding size at that point. I don’t feel strongly either way. What matters is the bad behavior; if the behavior is wrong with this patch, incorrect layout or something, then we should keep working on it. If there is no observable bad behavior I’d be tempted to just land this as is.
(In reply to Javier Fernandez from comment #7) > Comment on attachment 443774 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=443774&action=review > > >>> Source/WebCore/rendering/RenderGrid.cpp:1226 > >>> + std::optional<LayoutUnit> availableWidth = child.overridingContainingBlockContentLogicalWidth(); > >> > >> Although this seems correct I don't get why we have nullopt in the overriding size here. These two methods are called after calling updateGridAreaBreathLogicalSize() which always sets the overrides. That's why we were calling value() directly here without checking the presence of the value. Do you have more info about the exact conditions that trigger the bad access? > > > > I note that these values are set in updateGridAreaBreathLogicalSize(), but later cleared in applyStretchAlignmentToChildIfNeeded: > > > > // We clear height and width override values because we will decide now whether it's allowed or > > // not, evaluating the conditions which might have changed since the old values were set. > > child.clearOverridingLogicalHeight(); > > child.clearOverridingLogicalWidth(); > > No, those are different "override" values. Notice the function names are > different. Taking a closer look, the backtrace below best shows the sequence of conditions for which overridingContainingBlockContentLogicalHeight is set to the non-existent values which leads to the crash in updateAutoMarginsInColumnAxisIfNeeded: 1 0x28a96517c WebCore::RenderBox::setOverridingContainingBlockContentLogicalHeight(std::__1::optional<WebCore::LayoutUnit>) 2 0x28ab74508 WebCore::RenderGrid::updateGridAreaLogicalSize(WebCore::RenderBox&, std::__1::optional<WebCore::LayoutUnit>, std::__1::optional<WebCore::LayoutUnit>) const 3 0x28ab6baa0 WebCore::RenderGrid::performGridItemsPreLayout(WebCore::GridTrackSizingAlgorithm const&) const 4 0x28ab6ec58 WebCore::RenderGrid::computeIntrinsicLogicalWidths(WebCore::LayoutUnit&, WebCore::LayoutUnit&) const 5 0x288475ccc WebCore::RenderBox::computeIntrinsicKeywordLogicalWidths(WebCore::LayoutUnit&, WebCore::LayoutUnit&) const 6 0x28a99afd0 WebCore::RenderBox::computeIntrinsicLogicalWidthUsing(WebCore::Length, WebCore::LayoutUnit, WebCore::LayoutUnit) const 7 0x28a957778 WebCore::RenderBox::computeLogicalWidthInFragmentUsing(WebCore::SizeType, WebCore::Length, WebCore::LayoutUnit, WebCore::RenderBlock const&, WebCore::RenderFragmentContainer*) const 8 0x28a956084 WebCore::RenderBox::constrainLogicalWidthInFragmentByMinMax(WebCore::LayoutUnit, WebCore::LayoutUnit, WebCore::RenderBlock&, WebCore::RenderFragmentContainer*, WebCore::RenderBox::AllowIntrinsic) const 9 0x28ab78994 WebCore::RenderGrid::applyStretchAlignmentToChildIfNeeded(WebCore::RenderBox&) At this point: (lldb) p logicalHeight (std::optional<WebCore::LayoutUnit>) $0 = Has Value=false {} I do note, the "entry point" in RenderGrid::applyStretchAlignmentToChildIfNeeded is a relatively new area of code (bug 228022).
Comment on attachment 443968 [details] Patch It looks like the unrelated layout test failure it is hitting was addressed in Bug 233043 - Regression(r285639) fast/dom/Geolocation/cached-position-iframe.html is frequently crashing on Mac-wk1 Bug 224742 - [css-contain] Support contain:paint (REVERTED) However, the bot is not yet running with that revision. Moving to "cq?".
(In reply to Gabriel Nava Marino from comment #12) > (In reply to Javier Fernandez from comment #7) > > Comment on attachment 443774 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=443774&action=review > > > > >>> Source/WebCore/rendering/RenderGrid.cpp:1226 > > >>> + std::optional<LayoutUnit> availableWidth = child.overridingContainingBlockContentLogicalWidth(); > > >> > > >> Although this seems correct I don't get why we have nullopt in the overriding size here. These two methods are called after calling updateGridAreaBreathLogicalSize() which always sets the overrides. That's why we were calling value() directly here without checking the presence of the value. Do you have more info about the exact conditions that trigger the bad access? > > > > > > I note that these values are set in updateGridAreaBreathLogicalSize(), but later cleared in applyStretchAlignmentToChildIfNeeded: > > > > > > // We clear height and width override values because we will decide now whether it's allowed or > > > // not, evaluating the conditions which might have changed since the old values were set. > > > child.clearOverridingLogicalHeight(); > > > child.clearOverridingLogicalWidth(); > > > > No, those are different "override" values. Notice the function names are > > different. > > Taking a closer look, the backtrace below best shows the sequence of > conditions for which overridingContainingBlockContentLogicalHeight is set to > the non-existent values which leads to the crash in > updateAutoMarginsInColumnAxisIfNeeded: > > 1 0x28a96517c > WebCore::RenderBox::setOverridingContainingBlockContentLogicalHeight(std:: > __1::optional<WebCore::LayoutUnit>) > 2 0x28ab74508 > WebCore::RenderGrid::updateGridAreaLogicalSize(WebCore::RenderBox&, > std::__1::optional<WebCore::LayoutUnit>, > std::__1::optional<WebCore::LayoutUnit>) const > 3 0x28ab6baa0 > WebCore::RenderGrid::performGridItemsPreLayout(WebCore:: > GridTrackSizingAlgorithm const&) const > 4 0x28ab6ec58 > WebCore::RenderGrid::computeIntrinsicLogicalWidths(WebCore::LayoutUnit&, > WebCore::LayoutUnit&) const > 5 0x288475ccc > WebCore::RenderBox::computeIntrinsicKeywordLogicalWidths(WebCore:: > LayoutUnit&, WebCore::LayoutUnit&) const > 6 0x28a99afd0 > WebCore::RenderBox::computeIntrinsicLogicalWidthUsing(WebCore::Length, > WebCore::LayoutUnit, WebCore::LayoutUnit) const > 7 0x28a957778 > WebCore::RenderBox::computeLogicalWidthInFragmentUsing(WebCore::SizeType, > WebCore::Length, WebCore::LayoutUnit, WebCore::RenderBlock const&, > WebCore::RenderFragmentContainer*) const > 8 0x28a956084 > WebCore::RenderBox::constrainLogicalWidthInFragmentByMinMax(WebCore:: > LayoutUnit, WebCore::LayoutUnit, WebCore::RenderBlock&, > WebCore::RenderFragmentContainer*, WebCore::RenderBox::AllowIntrinsic) const > 9 0x28ab78994 > WebCore::RenderGrid::applyStretchAlignmentToChildIfNeeded(WebCore:: > RenderBox&) > > At this point: > (lldb) p logicalHeight > (std::optional<WebCore::LayoutUnit>) $0 = Has Value=false {} > > I do note, the "entry point" in > RenderGrid::applyStretchAlignmentToChildIfNeeded is a relatively new area of > code (bug 228022). Thanks for the info Gabriel. So there are several issues in the current code 1. I don't think performGridItemsPrelayout() should be called at this point. We must cache and reuse the intrinsic sizes. 2. computeIntrinsicLogicalWidths() should not destroy the previous overriding sizes. We were perhaps assuming that it would only be called as part of the preferred width computation but this example clearly shows that the assumption was incorrect With all that in mind, if it's urgent to fix the crash then I'm tempted to accept a simpler patch like yours with a big FIXME, and address the underlying issues in follow ups
(In reply to Gabriel Nava Marino from comment #12) > (In reply to Javier Fernandez from comment #7) > > Comment on attachment 443774 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=443774&action=review > > > > >>> Source/WebCore/rendering/RenderGrid.cpp:1226 > > >>> + std::optional<LayoutUnit> availableWidth = child.overridingContainingBlockContentLogicalWidth(); > > >> > > >> Although this seems correct I don't get why we have nullopt in the overriding size here. These two methods are called after calling updateGridAreaBreathLogicalSize() which always sets the overrides. That's why we were calling value() directly here without checking the presence of the value. Do you have more info about the exact conditions that trigger the bad access? > > > > > > I note that these values are set in updateGridAreaBreathLogicalSize(), but later cleared in applyStretchAlignmentToChildIfNeeded: > > > > > > // We clear height and width override values because we will decide now whether it's allowed or > > > // not, evaluating the conditions which might have changed since the old values were set. > > > child.clearOverridingLogicalHeight(); > > > child.clearOverridingLogicalWidth(); > > > > No, those are different "override" values. Notice the function names are > > different. > > Taking a closer look, the backtrace below best shows the sequence of > conditions for which overridingContainingBlockContentLogicalHeight is set to > the non-existent values which leads to the crash in > updateAutoMarginsInColumnAxisIfNeeded: > > 1 0x28a96517c > WebCore::RenderBox::setOverridingContainingBlockContentLogicalHeight(std:: > __1::optional<WebCore::LayoutUnit>) > 2 0x28ab74508 > WebCore::RenderGrid::updateGridAreaLogicalSize(WebCore::RenderBox&, > std::__1::optional<WebCore::LayoutUnit>, > std::__1::optional<WebCore::LayoutUnit>) const > 3 0x28ab6baa0 > WebCore::RenderGrid::performGridItemsPreLayout(WebCore:: > GridTrackSizingAlgorithm const&) const > 4 0x28ab6ec58 > WebCore::RenderGrid::computeIntrinsicLogicalWidths(WebCore::LayoutUnit&, > WebCore::LayoutUnit&) const > 5 0x288475ccc > WebCore::RenderBox::computeIntrinsicKeywordLogicalWidths(WebCore:: > LayoutUnit&, WebCore::LayoutUnit&) const > 6 0x28a99afd0 > WebCore::RenderBox::computeIntrinsicLogicalWidthUsing(WebCore::Length, > WebCore::LayoutUnit, WebCore::LayoutUnit) const > 7 0x28a957778 > WebCore::RenderBox::computeLogicalWidthInFragmentUsing(WebCore::SizeType, > WebCore::Length, WebCore::LayoutUnit, WebCore::RenderBlock const&, > WebCore::RenderFragmentContainer*) const > 8 0x28a956084 > WebCore::RenderBox::constrainLogicalWidthInFragmentByMinMax(WebCore:: > LayoutUnit, WebCore::LayoutUnit, WebCore::RenderBlock&, > WebCore::RenderFragmentContainer*, WebCore::RenderBox::AllowIntrinsic) const > 9 0x28ab78994 > WebCore::RenderGrid::applyStretchAlignmentToChildIfNeeded(WebCore:: > RenderBox&) > > At this point: > (lldb) p logicalHeight > (std::optional<WebCore::LayoutUnit>) $0 = Has Value=false {} > > I do note, the "entry point" in > RenderGrid::applyStretchAlignmentToChildIfNeeded is a relatively new area of > code (bug 228022). Thank you so much for the analysis. This is clearly the culprit of this issue, which should be classifies as an actual REGRESSION. We usually don't need to use the overridingLogicalWidth, since the inline-size is 'stretched' by default. However, in the mentioned bug #228022 we introduced this new code to deal with some special cases related to replaced elements (eg Images). However, taking a deeper look at the new code, I couldn't understand why we are calling constrainLogicalWidthInFragmentByMinMax on the LayoutGrid instead on the child (despite I've been the one reviewing the original patch :( ). Calling constrainLogicalWidthInFragmentByMinMax on the LayoutGridItem it's indeed the root cause of this bug, since it requires to compute again the grid containers preferredLogicalWidth (as part of the call to constrainLogicalHeightByMinMax) and it probably reset to indefinite (nullopt) some of the grid item's grid areas (aka overridingContaningBlockContentXXX). Another issue (which I think we had to face already in other bugs) is why the grid container has its preferred width as dirty as this phase, since we have already computed its logical width (at the beginning of the layoutBlock function) and we are just laying out the grid items now. My theory on this is that in this test case, we are invalidating some of the grid item's preferred width, which causes the containing block chain to be invalidated as well. @zsun, could you take a look please ? Also, could you verify why the bug #228022 is still open ? Thanks.
Comment on attachment 443968 [details] Patch So Javi and myself have discussed this and we have came up to the conclusion that the issue is indeed located in the patch for bug 228022. In this patch the code is recomputing the intrinsic width of the grid when it should really recompute the intrinsic width of the grid item. We'll revert the patch from that bug making this fix not needed.
I've verified this has been resolved by bug #228022. *** This bug has been marked as a duplicate of bug 228022 ***
Wait, no this was a regression caused by bug 228022, so not a duplicate of it.
I guess this is resolved by the new corrected fix for bug 228022 and caused by the original incorrect fix for bug 228022, so I’ll leave it marked as a duplicate.