Summary: | [css-grid] ASSERTION FAILED: !m_gridIsDirty in WebCore::RenderGrid::gridRowCount | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Renata Hodovan <hodovan> | ||||||||||||||
Component: | Layout and Rendering | Assignee: | Manuel Rego Casasnovas <rego> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | commit-queue, darin, esprehn+autocc, glenn, hodovan, hyatt, jfernandez, kondapallykalyan, rego, simon.fraser, svillar, zalan | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | WebKit Local Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 60731, 116980 | ||||||||||||||||
Attachments: |
|
Description
Renata Hodovan
2016-10-14 09:56:05 PDT
Created attachment 291804 [details]
Test case to reproduce the issue
I can verify the crash too. I'm attached a little bit reduced test case.
The issue is that for some reason when we have the "autofocus" property
a simplifiedLayout() is performed.
So in RenderGrid::layoutBlock() we early return and the grid is not populated,
so the m_gridIsDirty flag is not cleared.
As the grid was not populated we cannot ask for the size of the grid
when trying to layout the positioned object.
Created attachment 291937 [details]
Patch
Comment on attachment 291937 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291937&action=review > Source/WebCore/rendering/RenderGrid.cpp:453 > + if (!relayoutChildren && !posChildNeedsLayout() && simplifiedLayout()) I understand that we need to ensure the grid is laid out before performing a simplifiedLayout on positioned items, but I'm not sure whether we are addressing the root cause of the issue. Why simplifiedLayout doesn't return false because of the Gird needsLayout flag ? Comment on attachment 291937 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291937&action=review >> Source/WebCore/rendering/RenderGrid.cpp:453 >> + if (!relayoutChildren && !posChildNeedsLayout() && simplifiedLayout()) > > I understand that we need to ensure the grid is laid out before performing a simplifiedLayout on positioned items, but I'm not sure whether we are addressing the root cause of the issue. Why simplifiedLayout doesn't return false because of the Gird needsLayout flag ? So basically in this example RenderGrid::layoutBlock() is called 3 times. The 1st one the flags that are TRUE are selfNeedsLayout(), normalChildNeedsLayout() and posChildNeedsLayout(). The 2nd time only normalChildNeedsLayout() is TRUE. The 3rd time, which only happens if you use "autofocus", only posChildNeedsLayout() is TRUE. The problem is that at the end of RenderGrid::layoutBlock() we call clearGrid(), so after each layout we clear the grid and set the dirty flag to TRUE. So I think that in this situation we should force a layout (with posChildNeedsLayout() TRUE) to be sure that we can check the number of columns/rows and the size of them. Comment on attachment 291937 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291937&action=review >>> Source/WebCore/rendering/RenderGrid.cpp:453 >>> + if (!relayoutChildren && !posChildNeedsLayout() && simplifiedLayout()) >> >> I understand that we need to ensure the grid is laid out before performing a simplifiedLayout on positioned items, but I'm not sure whether we are addressing the root cause of the issue. Why simplifiedLayout doesn't return false because of the Gird needsLayout flag ? > > So basically in this example RenderGrid::layoutBlock() is called 3 times. > The 1st one the flags that are TRUE are selfNeedsLayout(), normalChildNeedsLayout() and posChildNeedsLayout(). > The 2nd time only normalChildNeedsLayout() is TRUE. > The 3rd time, which only happens if you use "autofocus", only posChildNeedsLayout() is TRUE. > > The problem is that at the end of RenderGrid::layoutBlock() we call clearGrid(), > so after each layout we clear the grid and set the dirty flag to TRUE. > So I think that in this situation we should force a layout (with posChildNeedsLayout() TRUE) > to be sure that we can check the number of columns/rows and the size of them. I think someone with more render tree knowledge than me should review. Hyatt? Zalan? (In reply to comment #5) > Comment on attachment 291937 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=291937&action=review > > >>> Source/WebCore/rendering/RenderGrid.cpp:453 > >>> + if (!relayoutChildren && !posChildNeedsLayout() && simplifiedLayout()) > >> > >> I understand that we need to ensure the grid is laid out before performing a simplifiedLayout on positioned items, but I'm not sure whether we are addressing the root cause of the issue. Why simplifiedLayout doesn't return false because of the Gird needsLayout flag ? > > > > So basically in this example RenderGrid::layoutBlock() is called 3 times. > > The 1st one the flags that are TRUE are selfNeedsLayout(), normalChildNeedsLayout() and posChildNeedsLayout(). > > The 2nd time only normalChildNeedsLayout() is TRUE. > > The 3rd time, which only happens if you use "autofocus", only posChildNeedsLayout() is TRUE. > > > > The problem is that at the end of RenderGrid::layoutBlock() we call clearGrid(), > > so after each layout we clear the grid and set the dirty flag to TRUE. > > So I think that in this situation we should force a layout (with posChildNeedsLayout() TRUE) > > to be sure that we can check the number of columns/rows and the size of them. > > I think someone with more render tree knowledge than me should review. > Hyatt? Zalan? Looking (In reply to comment #5) > Comment on attachment 291937 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=291937&action=review > > >>> Source/WebCore/rendering/RenderGrid.cpp:453 > >>> + if (!relayoutChildren && !posChildNeedsLayout() && simplifiedLayout()) > >> > >> I understand that we need to ensure the grid is laid out before performing a simplifiedLayout on positioned items, but I'm not sure whether we are addressing the root cause of the issue. Why simplifiedLayout doesn't return false because of the Gird needsLayout flag ? > > > > So basically in this example RenderGrid::layoutBlock() is called 3 times. > > The 1st one the flags that are TRUE are selfNeedsLayout(), normalChildNeedsLayout() and posChildNeedsLayout(). > > The 2nd time only normalChildNeedsLayout() is TRUE. > > The 3rd time, which only happens if you use "autofocus", only posChildNeedsLayout() is TRUE. > > > > The problem is that at the end of RenderGrid::layoutBlock() we call clearGrid(), > > so after each layout we clear the grid and set the dirty flag to TRUE. > > So I think that in this situation we should force a layout (with posChildNeedsLayout() TRUE) > > to be sure that we can check the number of columns/rows and the size of them. > > I think someone with more render tree knowledge than me should review. > Hyatt? Zalan? Looking at it now. Comment on attachment 291937 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291937&action=review >>>>>> Source/WebCore/rendering/RenderGrid.cpp:453 >>>>>> + if (!relayoutChildren && !posChildNeedsLayout() && simplifiedLayout()) >>>>> >>>>> I understand that we need to ensure the grid is laid out before performing a simplifiedLayout on positioned items, but I'm not sure whether we are addressing the root cause of the issue. Why simplifiedLayout doesn't return false because of the Gird needsLayout flag ? >>>> >>>> So basically in this example RenderGrid::layoutBlock() is called 3 times. >>>> The 1st one the flags that are TRUE are selfNeedsLayout(), normalChildNeedsLayout() and posChildNeedsLayout(). >>>> The 2nd time only normalChildNeedsLayout() is TRUE. >>>> The 3rd time, which only happens if you use "autofocus", only posChildNeedsLayout() is TRUE. >>>> >>>> The problem is that at the end of RenderGrid::layoutBlock() we call clearGrid(), >>>> so after each layout we clear the grid and set the dirty flag to TRUE. >>>> So I think that in this situation we should force a layout (with posChildNeedsLayout() TRUE) >>>> to be sure that we can check the number of columns/rows and the size of them. >>> >>> I think someone with more render tree knowledge than me should review. Hyatt? Zalan? >> >> Looking >> (In reply to comment #5) > > Looking at it now. I am not too familiar with the grid dependencies but it seems a bit odd that the grid gets dirty soon after the layout is done, even before any changes happen to the tree. However since this is how it currently works, any kind of layout computation that has any dependency on the grid can't go through the simplified layout path. Simplified layout supports 2 type of changes (atm) - positioned descendant move - overflow re-computation It seems obvious that while computing offsets (offsetAndBreadthForPositionedChild) for the positioned grid descendants, we rely on some of the grid values -although m_rowPositions looks just fine to me (but again I don't know whether those values are stale or not) The question is whether re-computing the overflow has some dependencies on this grid too. If it does, simplified layout is clearly not working for RenderGrid and should be removed it completely. if not, then my proposal is do something like this: Right now RenderBlock::simplifiedLayout() checks first whether it can actually do a simplified layout and if so, we preform both the positioned placement and the overflow computation. It might be better to decouple them and have a virtual function to check if we can perform simplified layout at all. This function then could be overwritten by RenderGrid and return false when the type of the layout requires a clean grid bool RenderGrid::foobarCanDoSimplifiedLayout() (<-terrible name) { if (posChildNeedsLayout() && m_gridIsDirty) (in the future m_gridIsDirty might not be dirty after every layout) return false; return RenderBlock::foobarCanDoSimplifiedLayout(); } Created attachment 293879 [details]
Patch
Thanks for the detailed review Zalan! (In reply to comment #7) > I am not too familiar with the grid dependencies but it seems a bit odd that > the grid gets dirty soon after the layout is done, even before any changes > happen to the tree. However since this is how it currently works, any kind > of layout computation that has any dependency on the grid can't go through > the simplified layout path. Yeah eventually we want to avoid marking the grid as dirty after every layout. Anyway I believe we should protect the simplified layout code path, in case it's run on a dirty grid with positioned items. > Right now RenderBlock::simplifiedLayout() checks first whether it can > actually do a simplified layout and if so, we preform both the positioned > placement and the overflow computation. It might be better to decouple them > and have a virtual function to check if we can perform simplified layout at > all. This function then could be overwritten by RenderGrid and return false > when the type of the layout requires a clean grid > bool RenderGrid::foobarCanDoSimplifiedLayout() (<-terrible name) > { > if (posChildNeedsLayout() && m_gridIsDirty) (in the future m_gridIsDirty > might not be dirty after every layout) > return false; > return RenderBlock::foobarCanDoSimplifiedLayout(); > } I've implemented this proposal. Actually on top of the crash we were having a wrong behavior when you move a positioned grid item. This will fix it too. Comment on attachment 293879 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293879&action=review > Source/WebCore/rendering/RenderBlock.h:400 > private: > + > static std::unique_ptr<RenderBlock> createAnonymousBlockWithStyleAndDisplay(Document&, const RenderStyle&, EDisplay); Please don’t add this blank line. > Source/WebCore/rendering/RenderGrid.h:112 > + bool canPerformSimplifiedLayout() const override; I suggest we use final here instead of override, since there is no need to override further at this time. Created attachment 294050 [details]
Patch for landing
Thanks for the review. (In reply to comment #10) > Comment on attachment 293879 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293879&action=review > > > Source/WebCore/rendering/RenderBlock.h:400 > > private: > > + > > static std::unique_ptr<RenderBlock> createAnonymousBlockWithStyleAndDisplay(Document&, const RenderStyle&, EDisplay); > > Please don’t add this blank line. Fixed. > > Source/WebCore/rendering/RenderGrid.h:112 > > + bool canPerformSimplifiedLayout() const override; > > I suggest we use final here instead of override, since there is no need to > override further at this time. Done. Created attachment 294362 [details]
Patch for landing rebased
Created attachment 294484 [details]
Patch for landing rebased again
Comment on attachment 294484 [details] Patch for landing rebased again Clearing flags on attachment: 294484 Committed r208586: <http://trac.webkit.org/changeset/208586> All reviewed patches have been landed. Closing bug. |