New issue
Advanced search Search tips

Issue 621517 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocking:
issue 589460



Sign in to add a comment

[css-grid] crash when using repeat(auto-fill) and min-width:-webkit-min-content

Project Member Reported by mpalmg...@mozilla.com, Jun 20 2016

Issue description

Version: 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?)

 
chrome-min-content-crash.html
387 bytes View Download

Comment 1 by svil...@igalia.com, Jun 20 2016

Status: Assigned (was: Untriaged)
Labels: Needs-Feedback
Iam unable to repro this issue on Win/ All chrome channels , could you please re-check and update here. Thanks 

Comment 3 by r...@igalia.com, Jul 7 2016

Blocking: 589460
Cc: svil...@igalia.com jfernan...@igalia.com r...@igalia.com
Labels: -OS-Linux -Needs-Feedback OS-All
Owner: svil...@igalia.com
@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.

Comment 4 by svil...@igalia.com, 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
Project Member

Comment 5 by bugdroid1@chromium.org, 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

Comment 6 by e...@chromium.org, Jul 15 2016

Status: Fixed (was: Assigned)
Project Member

Comment 7 by bugdroid1@chromium.org, 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

Project Member

Comment 8 by bugdroid1@chromium.org, 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

> 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?

Comment 10 by r...@igalia.com, 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.
Project Member

Comment 11 by bugdroid1@chromium.org, Nov 21 2016

Labels: merge-merged-2924
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