[css-grid] crash when using repeat(auto-fill) and min-width:-webkit-min-content |
|||||
Issue descriptionVersion: 53.0.2767.4 (Official Build) dev (64-bit) OS: Linux What steps will reproduce the problem? (1) load the attached testcase (2) (3) What is the expected output? A blank page. What do you see instead? "Aw, Snap! ..." (a crash?)
,
Jul 6 2016
Iam unable to repro this issue on Win/ All chrome channels , could you please re-check and update here. Thanks
,
Jul 7 2016
@mmohammad probably you're not enabling the experimental flag (--enable-experimental-web-platform-features), otherwise you're not testing the Grid Layout code. BTW, I can reproduce the issue too.
,
Jul 8 2016
So the problem is that the code has a recursive never ending loop: 0x00007fffef839165 in blink::LayoutGrid::computeIntrinsicLogicalWidths(blink::LayoutUnit&, blink::LayoutUnit&) const () from /home/sergio/checkout/chromium/src/out/Release/lib/libwebcore_shared.so 0x00007fffef7dab3b in blink::LayoutBox::computeIntrinsicLogicalWidthUsing(blink::Length const&, blink::LayoutUnit, blink::LayoutUnit) const () from /home/sergio/checkout/chromium/src/out/Release/lib/libwebcore_shared.so 0x00007fffef7de48e in blink::LayoutBox::computeLogicalWidthUsing(blink::SizeType, blink::Length const&, blink::LayoutUnit, blink::LayoutBlock const*) const () from /home/sergio/checkout/chromium/src/out/Release/lib/libwebcore_shared.so 0x00007fffef7de73b in blink::LayoutBox::constrainLogicalWidthByMinMax(blink::LayoutUnit, blink::LayoutUnit, blink::LayoutBlock*) const () from /home/sergio/checkout/chromium/src/out/Release/lib/libwebcore_shared.so 0x00007fffef8264e4 in blink::LayoutGrid::computeAutoRepeatTracksCount(blink::GridTrackSizingDirection) const () from /home/sergio/checkout/chromium/src/out/Release/lib/libwebcore_shared.so 0x00007fffef8384d5 in blink::LayoutGrid::placeItemsOnGrid() [clone .part.505] [clone .constprop.511] () from /home/sergio/checkout/chromium/src/out/Release/lib/libwebcore_shared.so 0x00007fffef839165 in blink::LayoutGrid::computeIntrinsicLogicalWidths(blink::LayoutUnit&, blink::LayoutUnit&) const () from /home/sergio/checkout/chromium/src/out/Release/lib/libwebcore_shared.so
,
Jul 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1c073ae5204b186aa0695ecedc80abc5225d85a4 commit 1c073ae5204b186aa0695ecedc80abc5225d85a4 Author: svillar <svillar@igalia.com> Date: Thu Jul 14 19:06:11 2016 [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 Review-Url: https://codereview.chromium.org/2135703002 Cr-Commit-Position: refs/heads/master@{#405529} [add] https://crrev.com/1c073ae5204b186aa0695ecedc80abc5225d85a4/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash-expected.txt [add] https://crrev.com/1c073ae5204b186aa0695ecedc80abc5225d85a4/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash.html [modify] https://crrev.com/1c073ae5204b186aa0695ecedc80abc5225d85a4/third_party/WebKit/Source/core/layout/LayoutGrid.cpp [modify] https://crrev.com/1c073ae5204b186aa0695ecedc80abc5225d85a4/third_party/WebKit/Source/core/layout/LayoutGrid.h
,
Jul 15 2016
,
Oct 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/90ea1ed689536a8500f9d92652b2a4a0baa24db6 commit 90ea1ed689536a8500f9d92652b2a4a0baa24db6 Author: svillar <svillar@igalia.com> Date: Tue Oct 18 14:44:12 2016 [css-grid] Remove a duplicated auto repeat computation for intrinsic sizes After crrev.com/414446 we can safely remove the temporary fix (crrev.com/405529) we had to avoid crashes and recursive calls when computing the auto repeat tracks count for grids with intrinsic sizes. This change unveiled a couple of bugs in the computation of auto repeat tracks for intrinsic sizes. The first one is that we were not considering the existence of definite minimum sizes. In those cases, instead of just returning 1 repetition, we should return the minimum number of repetitions that fulfill the minimum size requirement. The second bug was that grids were not properly recomputing the number of auto repeat tracks when the min|max-content contributions of grid items changed (whenever the grid container had an intrinsic width). One Mozilla test had to be updated too because it was wrong. BUG= 621517 , 633474 Review-Url: https://codereview.chromium.org/2287533002 Cr-Commit-Position: refs/heads/master@{#425958} [add] https://crrev.com/90ea1ed689536a8500f9d92652b2a4a0baa24db6/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-repeat-intrinsic.html [add] https://crrev.com/90ea1ed689536a8500f9d92652b2a4a0baa24db6/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-change-intrinsic-size-with-auto-repeat-tracks-expected.txt [add] https://crrev.com/90ea1ed689536a8500f9d92652b2a4a0baa24db6/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-change-intrinsic-size-with-auto-repeat-tracks.html [modify] https://crrev.com/90ea1ed689536a8500f9d92652b2a4a0baa24db6/third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-001-expected.html [modify] https://crrev.com/90ea1ed689536a8500f9d92652b2a4a0baa24db6/third_party/WebKit/Source/core/layout/LayoutGrid.cpp [modify] https://crrev.com/90ea1ed689536a8500f9d92652b2a4a0baa24db6/third_party/WebKit/Source/core/layout/LayoutGrid.h
,
Nov 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64 commit b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64 Author: rego <rego@igalia.com> Date: Fri Nov 18 12:31:52 2016 Revert of [css-grid] Remove a duplicated auto repeat computation for intrinsic sizes (patchset #4 id:60001 of https://codereview.chromium.org/2287533002/ ) Reason for revert: Caused crash on Mac. See http://crbug.com/662016 We're reverting only the code change as we want to keep the tests, that will be fixed after the refactoring happening in bug #627812 is done. So, we need to mark 2 tests as failure in TestExpectations file. BUG=662016, 666688 , 627812 Original issue's description: > [css-grid] Remove a duplicated auto repeat computation for intrinsic sizes > > After crrev.com/414446 we can safely remove the temporary fix > (crrev.com/405529) we had to avoid crashes and recursive calls when > computing the auto repeat tracks count for grids with intrinsic sizes. > > This change unveiled a couple of bugs in the computation of auto repeat > tracks for intrinsic sizes. The first one is that we were not considering the > existence of definite minimum sizes. In those cases, instead of just returning 1 > repetition, we should return the minimum number of repetitions that fulfill the > minimum size requirement. > > The second bug was that grids were not properly recomputing the number of > auto repeat tracks when the min|max-content contributions of grid items > changed (whenever the grid container had an intrinsic width). > > One Mozilla test had to be updated too because it was wrong. > BUG= 621517 , 633474 > > Review-Url: https://codereview.chromium.org/2287533002 > Cr-Commit-Position: refs/heads/master@{#425958} Review-Url: https://codereview.chromium.org/2510393002 Cr-Commit-Position: refs/heads/master@{#433183} [modify] https://crrev.com/b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64/third_party/WebKit/Source/core/layout/LayoutGrid.cpp [modify] https://crrev.com/b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64/third_party/WebKit/Source/core/layout/LayoutGrid.h
,
Nov 19 2016
> One Mozilla test had to be updated too because it was wrong. I'm curious which test you think is wrong. Can you be more specific please?
,
Nov 21 2016
@mpalmgren the Mozilla tests that was modified was basically this comment by Sergio: https://bugzilla.mozilla.org/show_bug.cgi?id=1280798#c3 So I believe this is already fixed in Firefox too.
,
Nov 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ab661a19616ff4cd7ff522cef71354971c4d5694 commit ab661a19616ff4cd7ff522cef71354971c4d5694 Author: Manuel Rego Casasnovas <rego@igalia.com> Date: Mon Nov 21 11:38:41 2016 Revert of [css-grid] Remove a duplicated auto repeat computation for intrinsic sizes (patchset #4 id:60001 of https://codereview.chromium.org/2287533002/ ) Reason for revert: Caused crash on Mac. See http://crbug.com/662016 We're reverting only the code change as we want to keep the tests, that will be fixed after the refactoring happening in bug #627812 is done. So, we need to mark 2 tests as failure in TestExpectations file. BUG=662016, 666688 , 627812 Original issue's description: > [css-grid] Remove a duplicated auto repeat computation for intrinsic sizes > > After crrev.com/414446 we can safely remove the temporary fix > (crrev.com/405529) we had to avoid crashes and recursive calls when > computing the auto repeat tracks count for grids with intrinsic sizes. > > This change unveiled a couple of bugs in the computation of auto repeat > tracks for intrinsic sizes. The first one is that we were not considering the > existence of definite minimum sizes. In those cases, instead of just returning 1 > repetition, we should return the minimum number of repetitions that fulfill the > minimum size requirement. > > The second bug was that grids were not properly recomputing the number of > auto repeat tracks when the min|max-content contributions of grid items > changed (whenever the grid container had an intrinsic width). > > One Mozilla test had to be updated too because it was wrong. > BUG= 621517 , 633474 > > Review-Url: https://codereview.chromium.org/2287533002 > Cr-Commit-Position: refs/heads/master@{#425958} Review-Url: https://codereview.chromium.org/2510393002 Cr-Commit-Position: refs/heads/master@{#433183} (cherry picked from commit b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64) Review URL: https://codereview.chromium.org/2521573002 . Cr-Commit-Position: refs/branch-heads/2924@{#24} Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059} [modify] https://crrev.com/ab661a19616ff4cd7ff522cef71354971c4d5694/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/ab661a19616ff4cd7ff522cef71354971c4d5694/third_party/WebKit/Source/core/layout/LayoutGrid.cpp [modify] https://crrev.com/ab661a19616ff4cd7ff522cef71354971c4d5694/third_party/WebKit/Source/core/layout/LayoutGrid.h |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by svil...@igalia.com
, Jun 20 2016