Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2135703002:
[css-grid] Fix crash when using auto repeat for indefinite widths (Closed)
|
Created:
4 years, 5 months ago by svillar Modified:
4 years, 5 months ago CC:
chromium-reviews, jfernandez, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, leviw+renderwatch, jchaffraix+rendering, blink-reviews, eae+blinkwatch Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[css-grid] Fix crash when using auto repeat for indefinite widths
The computeAutoRepeatTracksCount() call needs to know the available size in
order to compute a number of repetitions. When computing the preferred
logical widths that size is indefinite so it should just return 1. The
problem is that instead it was directly calling another methods that were
reentering the preferred size computation. That lead to a situation of never
ending recursive calls to the preferred widths code.
As we know whether we're computing the intrinsic sizes or not, we can
leverage that knowledge and do not call the method in that case (the number
of repetitions would be 0 or 1 depending on whether auto-fit|fill was
specified or not).
Made some of the attributes mutable as they are modified by the intrinsic size
computation. Moved the clean up of those attributes to a new method. We'll
eventually need to change the current code so that we could call the track
sizing algorithm without affecting the internal state of the layout object.
BUG=621517
Committed: https://crrev.com/1c073ae5204b186aa0695ecedc80abc5225d85a4
Cr-Commit-Position: refs/heads/master@{#405529}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Test improvements #
Total comments: 11
Patch Set 3 : Simplest approach #
Messages
Total messages: 23 (6 generated)
svillar@igalia.com changed reviewers: + cbiesinger@chromium.org, eae@chromium.org, jfernandez@igalia.com, rego@igalia.com
Sending out for review
I'm not sure about some of the changes, check the comments inline. https://codereview.chromium.org/2135703002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash.html (right): https://codereview.chromium.org/2135703002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash.html:16: <div class="grid autoFit"><span></span></div> Nit: Are those <span> elements needed? https://codereview.chromium.org/2135703002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash.html:30: <p>This test has PASSED if it didn't CRASH.</p> Could you be little bit more descriptive about what the test is checking? https://codereview.chromium.org/2135703002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2135703002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:532: clearGridDataAndMarkAsDirty(); But we're already setting m_autoRepeatColumns|Rows on placeItemsOnGrid(). Do we really need this clear()? Also, could you explain in the description all the changes related to "mutable" attributes and so on?
Thanks for the review! https://codereview.chromium.org/2135703002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash.html (right): https://codereview.chromium.org/2135703002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash.html:16: <div class="grid autoFit"><span></span></div> On 2016/07/11 09:47:41, Manuel Rego wrote: > Nit: Are those <span> elements needed? Not sure why I added them. Perhaps some initial testing, I need to double-check though https://codereview.chromium.org/2135703002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash.html:30: <p>This test has PASSED if it didn't CRASH.</p> On 2016/07/11 09:47:41, Manuel Rego wrote: > Could you be little bit more descriptive about what the test is checking? Acknowledged. https://codereview.chromium.org/2135703002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2135703002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:532: clearGridDataAndMarkAsDirty(); On 2016/07/11 09:47:42, Manuel Rego wrote: > But we're already setting m_autoRepeatColumns|Rows on > placeItemsOnGrid(). > Do we really need this clear()? If you don't mark the grid as empty here, placeItemsOnGrid() won't do anything (as it has an early return). And you must recompute the tracks because the number of auto repeat columns will be likely different. > Also, could you explain in the description all the changes related to "mutable" > attributes and so on? OK I could mention that I clear all the mutable attributes in a specific method if you want.
Description was changed from ========== [css-grid] Fix crash when using auto repeat for indefinite widths The computeAutoRepeatTracksCount() call needs to know the available size in order to compute a number of repetitions. When computing the preferred logical widths that size is indefinite so it should just return 1. The problem is that instead it was directly calling another methods that were reentering the preferred size computation. That lead to a situation of never ending recursive calls to the preferred widths code. As we know whether we're computing the intrinsic sizes or not, we can leverage that knowledge and do not call the method in that case (the number of repetitions would be 0 or 1 depending on whether auto-fit|fill was specified or not). BUG=621517 ========== to ========== [css-grid] Fix crash when using auto repeat for indefinite widths The computeAutoRepeatTracksCount() call needs to know the available size in order to compute a number of repetitions. When computing the preferred logical widths that size is indefinite so it should just return 1. The problem is that instead it was directly calling another methods that were reentering the preferred size computation. That lead to a situation of never ending recursive calls to the preferred widths code. As we know whether we're computing the intrinsic sizes or not, we can leverage that knowledge and do not call the method in that case (the number of repetitions would be 0 or 1 depending on whether auto-fit|fill was specified or not). Made some of the attributes mutable as they are modified by the intrinsic size computation. Moved the clean up of those attributes to a new method. We'll eventually need to change the current code so that we could call the track sizing algorithm without affecting the internal state of the layout object. BUG=621517 ==========
Description was changed from ========== [css-grid] Fix crash when using auto repeat for indefinite widths The computeAutoRepeatTracksCount() call needs to know the available size in order to compute a number of repetitions. When computing the preferred logical widths that size is indefinite so it should just return 1. The problem is that instead it was directly calling another methods that were reentering the preferred size computation. That lead to a situation of never ending recursive calls to the preferred widths code. As we know whether we're computing the intrinsic sizes or not, we can leverage that knowledge and do not call the method in that case (the number of repetitions would be 0 or 1 depending on whether auto-fit|fill was specified or not). Made some of the attributes mutable as they are modified by the intrinsic size computation. Moved the clean up of those attributes to a new method. We'll eventually need to change the current code so that we could call the track sizing algorithm without affecting the internal state of the layout object. BUG=621517 ========== to ========== [css-grid] Fix crash when using auto repeat for indefinite widths The computeAutoRepeatTracksCount() call needs to know the available size in order to compute a number of repetitions. When computing the preferred logical widths that size is indefinite so it should just return 1. The problem is that instead it was directly calling another methods that were reentering the preferred size computation. That lead to a situation of never ending recursive calls to the preferred widths code. As we know whether we're computing the intrinsic sizes or not, we can leverage that knowledge and do not call the method in that case (the number of repetitions would be 0 or 1 depending on whether auto-fit|fill was specified or not). Made some of the attributes mutable as they are modified by the intrinsic size computation. Moved the clean up of those attributes to a new method. We'll eventually need to change the current code so that we could call the track sizing algorithm without affecting the internal state of the layout object. BUG=621517 ==========
Ok thanks for the explanation. Non-owner LGTM. https://codereview.chromium.org/2135703002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2135703002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:532: clearGridDataAndMarkAsDirty(); On 2016/07/11 10:34:48, svillar1 wrote: > On 2016/07/11 09:47:42, Manuel Rego wrote: > > But we're already setting m_autoRepeatColumns|Rows on > > placeItemsOnGrid(). > > Do we really need this clear()? > > If you don't mark the grid as empty here, placeItemsOnGrid() won't do anything > (as it has an early return). And you must recompute the tracks because the > number of auto repeat columns will be likely different. True, I missed that. https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1627: { Maybe add a similar if to the one in dirtyGrid(): if (m_gridIsDirty) return; https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1641: void LayoutGrid::dirtyGrid() Shouldn't we rename this method? We've now: * dirtyGrid() vs * clearGridDataAndMarkAsDirty() Hard to know which one we should call.
https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1627: { On 2016/07/11 11:10:57, Manuel Rego wrote: > Maybe add a similar if to the one in dirtyGrid(): > if (m_gridIsDirty) > return; I think I'm adding an ASSERT there. https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1641: void LayoutGrid::dirtyGrid() On 2016/07/11 11:10:57, Manuel Rego wrote: > Shouldn't we rename this method? > > We've now: > * dirtyGrid() > vs > * clearGridDataAndMarkAsDirty() > > Hard to know which one we should call. The reason why I added a new method is because dirtyGrid() does one extra thing which is to flip the needsLayout flag. I cannot do that during the intrinsic size computation as is forbidden (there are asserts to test that).
https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1392: void LayoutGrid::placeItemsOnGrid(SizingOperation inlineAxisSizingOperation) Why we need to know whether is inline or block axis ? And if we do need, I don't think just a variable name is enough. As far as I understand the code below, we are processing logic for both axis, so I don't think there is any relation between axis and the SizingOperation value. https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1400: m_autoRepeatColumns = styleRef().gridAutoRepeatColumns().isEmpty() ? 0 : 1; Why 1 ? Hadn't we avoid that kind of harcoded 1 values in other cases ? https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.h:212: void clearGridDataAndMarkAsDirty() const; Why const ?
https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1392: void LayoutGrid::placeItemsOnGrid(SizingOperation inlineAxisSizingOperation) On 2016/07/11 12:52:22, jfernandez wrote: > Why we need to know whether is inline or block axis ? And if we do need, I don't > think just a variable name is enough. As far as I understand the code below, we > are processing logic for both axis, so I don't think there is any relation > between axis and the SizingOperation value. Well I tried to make a bit explicit that the sizing operation would be just used for inline axis, but you're right that the sizing operation has nothing to do with axis. I'll rename it. https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1400: m_autoRepeatColumns = styleRef().gridAutoRepeatColumns().isEmpty() ? 0 : 1; On 2016/07/11 12:52:22, jfernandez wrote: > Why 1 ? Hadn't we avoid that kind of harcoded 1 values in other cases ? Because that's what the spec says (https://drafts.csswg.org/css-grid/#auto-repeat). Basically if you don't have definite sizes then you must default to 1 repetition. https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.h:212: void clearGridDataAndMarkAsDirty() const; On 2016/07/11 12:52:22, jfernandez wrote: > Why const ? Because it's called inside computeIntrinsicLogicalWidths() which is a const function. We're only modifying mutable attributes so it's technically correct, although I agree that a const method called clear looks weird. The actual problem is that we're using the track sizing algorithm (which modifies some attributes of the layout object) to compute the intrinsic width, something that should not do this kind of state modifications. That's the reason why we have a couple of const_cast that should be indeed removed, but that is another topic and should be addressed in some other bug IMHO.
https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.h:215: mutable GridRepresentation m_grid; A const clear method and mutable non-cache data members is a pretty big warning flag that something is wrong with the API. We really need to find a better way to do this.
On 2016/07/12 17:38:48, eae wrote: > https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/LayoutGrid.h (right): > > https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutGrid.h:215: mutable > GridRepresentation m_grid; > A const clear method and mutable non-cache data members is a pretty big warning > flag that something is wrong with the API. We really need to find a better way > to do this. Yes and it's well known. The track sizing algorithm modifies several attributes of LayoutGrid as part of its computations. Thing is that we're using it also for intrinsic widths computations, which as you know, is a const operation. That's why we already have the ugly const_cast<> in that method. Our plan is to pass the algorithm some object that will be modified by the algorithm so that we wouldn't have to modify the internal state of the object (except some cached data that will be useful for the layout).
On 2016/07/13 09:13:21, svillar wrote: > On 2016/07/12 17:38:48, eae wrote: > > > https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/layout/LayoutGrid.h (right): > > > > > https://codereview.chromium.org/2135703002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/LayoutGrid.h:215: mutable > > GridRepresentation m_grid; > > A const clear method and mutable non-cache data members is a pretty big > warning > > flag that something is wrong with the API. We really need to find a better way > > to do this. > > Yes and it's well known. The track sizing algorithm modifies several attributes > of LayoutGrid as part of its computations. Thing is that we're using it also for > intrinsic widths computations, which as you know, is a const operation. That's > why we already have the ugly const_cast<> in that method. Our plan is to pass > the algorithm some object that will be modified by the algorithm so that we > wouldn't have to modify the internal state of the object (except some cached > data that will be useful for the layout). Sounds like a good plan. Thanks!
New simplified version looks much better to me. I keep my non-owner LGTM. :-)
It's really a better patch, LGTM
Much better, thank you! LGTM
The CQ bit was checked by eae@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Fix crash when using auto repeat for indefinite widths The computeAutoRepeatTracksCount() call needs to know the available size in order to compute a number of repetitions. When computing the preferred logical widths that size is indefinite so it should just return 1. The problem is that instead it was directly calling another methods that were reentering the preferred size computation. That lead to a situation of never ending recursive calls to the preferred widths code. As we know whether we're computing the intrinsic sizes or not, we can leverage that knowledge and do not call the method in that case (the number of repetitions would be 0 or 1 depending on whether auto-fit|fill was specified or not). Made some of the attributes mutable as they are modified by the intrinsic size computation. Moved the clean up of those attributes to a new method. We'll eventually need to change the current code so that we could call the track sizing algorithm without affecting the internal state of the layout object. BUG=621517 ========== to ========== [css-grid] Fix crash when using auto repeat for indefinite widths The computeAutoRepeatTracksCount() call needs to know the available size in order to compute a number of repetitions. When computing the preferred logical widths that size is indefinite so it should just return 1. The problem is that instead it was directly calling another methods that were reentering the preferred size computation. That lead to a situation of never ending recursive calls to the preferred widths code. As we know whether we're computing the intrinsic sizes or not, we can leverage that knowledge and do not call the method in that case (the number of repetitions would be 0 or 1 depending on whether auto-fit|fill was specified or not). Made some of the attributes mutable as they are modified by the intrinsic size computation. Moved the clean up of those attributes to a new method. We'll eventually need to change the current code so that we could call the track sizing algorithm without affecting the internal state of the layout object. BUG=621517 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Fix crash when using auto repeat for indefinite widths The computeAutoRepeatTracksCount() call needs to know the available size in order to compute a number of repetitions. When computing the preferred logical widths that size is indefinite so it should just return 1. The problem is that instead it was directly calling another methods that were reentering the preferred size computation. That lead to a situation of never ending recursive calls to the preferred widths code. As we know whether we're computing the intrinsic sizes or not, we can leverage that knowledge and do not call the method in that case (the number of repetitions would be 0 or 1 depending on whether auto-fit|fill was specified or not). Made some of the attributes mutable as they are modified by the intrinsic size computation. Moved the clean up of those attributes to a new method. We'll eventually need to change the current code so that we could call the track sizing algorithm without affecting the internal state of the layout object. BUG=621517 ========== to ========== [css-grid] Fix crash when using auto repeat for indefinite widths The computeAutoRepeatTracksCount() call needs to know the available size in order to compute a number of repetitions. When computing the preferred logical widths that size is indefinite so it should just return 1. The problem is that instead it was directly calling another methods that were reentering the preferred size computation. That lead to a situation of never ending recursive calls to the preferred widths code. As we know whether we're computing the intrinsic sizes or not, we can leverage that knowledge and do not call the method in that case (the number of repetitions would be 0 or 1 depending on whether auto-fit|fill was specified or not). Made some of the attributes mutable as they are modified by the intrinsic size computation. Moved the clean up of those attributes to a new method. We'll eventually need to change the current code so that we could call the track sizing algorithm without affecting the internal state of the layout object. BUG=621517 Committed: https://crrev.com/1c073ae5204b186aa0695ecedc80abc5225d85a4 Cr-Commit-Position: refs/heads/master@{#405529} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1c073ae5204b186aa0695ecedc80abc5225d85a4 Cr-Commit-Position: refs/heads/master@{#405529} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
