[css-grid] Protect against negative values in applyStretchAlignmentToChildIfNeeded()

[css-grid] Protect against negative values in applyStretchAlignmentToChildIfNeeded()

In LayoutGrid::applyStretchAlignmentToChildIfNeeded()
we're subtracting the top and bottom border and padding
to the desired height.
If the height is 0px due to the margins on the item
and not enough space on the cell, and the border or padding
are not 0px, then we end up with negative values.

In this patch we protect the call to setOverrideLogicalContentHeight()
to avoid passing a negative value.

BUG=697317
TEST=fast/css-grid-layout/grid-crash-huge-margins-and-min-height-max-content.html

Review-Url: https://codereview.chromium.org/2744593002
Cr-Commit-Position: refs/heads/master@{#455728}
Committed: https://chromium.googlesource.com/chromium/src/+/7c3cb4cb41c85b2de48a5ef82f40010b4b4f5040

[Manuel Rego, 2017-03-09 10:14:10]

rego@igalia.com changed reviewers:
+ cbiesinger@chromium.org, eae@chromium.org, jfernandez@igalia.com,
svillar@igalia.com

[svillar, 2017-03-09 10:29:15]

lgtm

We need a better explanation about the potential issues that could make those
values negative in the bug description.

https://codereview.chromium.org/2744593002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2744593002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1477: LayoutUnit()));
clampNegativeToZero()

[Manuel Rego, 2017-03-09 10:43:39]

The CQ bit was checked by rego@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 10:43:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Manuel Rego, 2017-03-09 10:52:47]

The CQ bit was checked by rego@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 10:53:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Manuel Rego, 2017-03-09 10:53:13]

Description was changed from

==========
[css-grid] Protect against negative values in
applyStretchAlignmentToChildIfNeeded()

We can end up with negative values in some corner cases,
so we protect the call to setOverrideLogicalContentHeight()
to avoid passing a negative value.

BUG=697317
TEST=fast/css-grid-layout/grid-crash-huge-margins-and-min-height-max-content.html
==========

to

==========
[css-grid] Protect against negative values in
applyStretchAlignmentToChildIfNeeded()

In LayoutGrid::applyStretchAlignmentToChildIfNeeded()
we're subtracting the top and bottom border and padding
to the desired height.
If the height is 0px due to the margins on the item
and not enough space on the cell, and the border or padding
are not 0px, then we end up with negative values.

In this patch we protect the call to setOverrideLogicalContentHeight()
to avoid passing a negative value.

BUG=697317
TEST=fast/css-grid-layout/grid-crash-huge-margins-and-min-height-max-content.html
==========

[Manuel Rego, 2017-03-09 10:59:13]

On 2017/03/09 10:29:15, svillar wrote:
> lgtm

Thanks for the review.

> We need a better explanation about the potential issues that could make those
> values negative in the bug description.

Updated the description and the test too, so it's clearer
where is the issue.

https://codereview.chromium.org/2744593002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2744593002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1477: LayoutUnit()));
On 2017/03/09 10:29:15, svillar wrote:
> clampNegativeToZero()

Done.

[commit-bot: I haz the power, 2017-03-09 12:35:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-09 12:35:40]

Dry run: This issue passed the CQ dry run.

[Manuel Rego, 2017-03-09 13:06:20]

The CQ bit was checked by rego@igalia.com

[Manuel Rego, 2017-03-09 13:06:20]

The patchset sent to the CQ was uploaded after l-g-t-m from svillar@igalia.com
Link to the patchset: https://codereview.chromium.org/2744593002/#ps40001
(title: "Simplify test to make issue even clearer")

[commit-bot: I haz the power, 2017-03-09 13:06:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 13:20:16]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489064780373170,
"parent_rev": "d5a7eabf41ff530632d5cd18b7136b52bbdc5f20", "commit_rev":
"7c3cb4cb41c85b2de48a5ef82f40010b4b4f5040"}

[commit-bot: I haz the power, 2017-03-09 13:21:06]

Description was changed from

==========
[css-grid] Protect against negative values in
applyStretchAlignmentToChildIfNeeded()

In LayoutGrid::applyStretchAlignmentToChildIfNeeded()
we're subtracting the top and bottom border and padding
to the desired height.
If the height is 0px due to the margins on the item
and not enough space on the cell, and the border or padding
are not 0px, then we end up with negative values.

In this patch we protect the call to setOverrideLogicalContentHeight()
to avoid passing a negative value.

BUG=697317
TEST=fast/css-grid-layout/grid-crash-huge-margins-and-min-height-max-content.html
==========

to

==========
[css-grid] Protect against negative values in
applyStretchAlignmentToChildIfNeeded()

In LayoutGrid::applyStretchAlignmentToChildIfNeeded()
we're subtracting the top and bottom border and padding
to the desired height.
If the height is 0px due to the margins on the item
and not enough space on the cell, and the border or padding
are not 0px, then we end up with negative values.

In this patch we protect the call to setOverrideLogicalContentHeight()
to avoid passing a negative value.

BUG=697317
TEST=fast/css-grid-layout/grid-crash-huge-margins-and-min-height-max-content.html

Review-Url: https://codereview.chromium.org/2744593002
Cr-Commit-Position: refs/heads/master@{#455728}
Committed:
https://chromium.googlesource.com/chromium/src/+/7c3cb4cb41c85b2de48a5ef82f40...
==========

[commit-bot: I haz the power, 2017-03-09 13:21:07]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/7c3cb4cb41c85b2de48a5ef82f40...

[Michael Achenbach, 2017-03-09 13:30:20]

machenbach@chromium.org changed reviewers:
+ machenbach@chromium.org

[Michael Achenbach, 2017-03-09 13:30:21]

Bizarre: Why did bugdroid mark this bug
https://bugs.chromium.org/p/chromium/issues/detail?id=682617#c19 with this CL I
wonder?

[Manuel Rego, 2017-03-09 13:51:52]

On 2017/03/09 13:30:21, Michael Achenbach wrote:
> Bizarre: Why did bugdroid mark this bug
> https://bugs.chromium.org/p/chromium/issues/detail?id=682617#c19 with this CL
I
> wonder?

I don't know, I don't have access to that bug.

[Michael Achenbach, 2017-03-09 14:06:24]

On 2017/03/09 13:51:52, Manuel Rego wrote:
> On 2017/03/09 13:30:21, Michael Achenbach wrote:
> > Bizarre: Why did bugdroid mark this bug
> > https://bugs.chromium.org/p/chromium/issues/detail?id=682617#c19 with this
CL
> I
> > wonder?
> 
> I don't know, I don't have access to that bug.

Bugdroid seems to have a bad time. File http://crbug.com/699987 for that (also
internal).