709398 - "ABORT: negative scaling factors must be handled manually" with background, -moz-column

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase (asserts fatally when loaded)

###!!! ABORT: negative scaling factors must be handled manually: 'aScale >= 0.0f', file nsCoord.h, line 138

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Mats Palmgren (inactive)]

Attachment: fix+test

https://tbpl.mozilla.org/?tree=Try&rev=8da8879637d8

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 734275 [details] [diff] [review]
fix+test

Review of attachment 734275 [details] [diff] [review]:
-----------------------------------------------------------------

Wouldn't it make more sense to fix this in nsCSSRendering::ComputeBackgroundPositioningArea?

Comment 4

[Mats Palmgren (inactive)]

Attachment: fix+test, v2

OK.  The only other caller of ComputeBackgroundPositioningArea
is nsDisplayBackgroundImage::GetPositioningArea() and I figured the
mIsThemed path should be consistent with the non-mIsThemed path so
I clamped that too.

https://tbpl.mozilla.org/?tree=Try&rev=e813a1025a29

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 734940 [details] [diff] [review]
fix+test, v2

Review of attachment 734940 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSRendering.cpp
@@ +4601,5 @@
>  nsSize
>  nsImageRenderer::ComputeSize(const nsStyleBackground::Size& aLayerSize,
>                               const nsSize& aBgPositioningArea)
>  {
> +  MOZ_ASSERT(aBgPositioningArea.width >= 0 && aBgPositioningArea.height >= 0);

I think this should be non-fatal. It doesn't matter much if it's negative here I think.

::: layout/base/nsDisplayList.cpp
@@ +2023,5 @@
>  {
>    if (mIsThemed) {
> +    nsRect bgRect(ToReferenceFrame(), mFrame->GetSize());
> +    bgRect.width = std::max(0, bgRect.width);
> +    bgRect.height = std::max(0, bgRect.height);

Why is this necessary? Frame sizes should never be negative. Do we have known situations where frame sizes go negative?

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last months and this bug has severity 'critical'.
:dholbert, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: testcase with unprefixed `column-count`

Here's the testcase, with unprefixed column-count, to make it testable in current builds (which don't support -moz-column-count).

I can't reproduce any assertions in a debug build with this testcase, so this was presumably fixed at some point.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: Bug 709398: Add crashtest for this no-longer-reproducible bug. (no review, crashtest-only)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24c0088aa2c0
Add crashtest for this no-longer-reproducible bug. (no review, crashtest-only)

Comment 11

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/24c0088aa2c0

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)