1533040 - Are the nsStyleCoord warnings about possible overflow valid?

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1. Load https://www.cryptool.org/en/cto-ciphers/scytale
2. Watch your terminal.

EXPECTED RESULTS: Not much warning spew.

ACTUAL RESULTS: Tons of warnings like so:

WARNING: nscoord overflow?: 'basis >= 0', file ../../../mozilla/layout/style/nsStyleCoord.h, line 136

What's happening here is that ComputeObjectAnchorCoord is called with aOriginBounds == 5220 and aImageSize == 5400. That means extraSpace ends up as -180 and then gets passed down into the "resolve a percentage" machinery.

Those values are coming from nsImageRenderer::ComputeObjectAnchorPoint where aOriginBounds.height == 5220 and aImageSize.height == 5400.

And those come from nsCSSRendering::PrepareImageLayer, where those correspond to bgPositionSize.height and imageSize.height.

I haven't dug deeply into the markup to see what the exact background-position and background-size styles look like, but I see nothing obvious that would force the background size to be smaller than the positioning area, which is what ends up tripping the warning here, right?

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I don't think they are, no. These callers used to do their own percentage resolution:

https://searchfox.org/mozilla-central/rev/aec730ed7d5dd8a4a5520190a26a631a0b4329e9/layout/painting/nsCSSRendering.cpp#1143

That assertion is ported from the equivalent Gecko code, which is:

https://searchfox.org/mozilla-central/rev/3e0f1d95fcf8832413457e3bec802113bdd1f8e8/layout/base/nsLayoutUtils.h#3017

I think we should just get rid of that.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1533040 - Remove invalid NS_WARNING_ASSERTION. r=dholbert,jwatt

We can totally get there with a negative percentage basis, see comment 0 for an
example.

We could keep the warning like:

NS_WARNING_ASSERTION(clamping_mode == StyleAllowedNumericType::All ||
basis >= 0, "nscoord overflow?");

Which will catch cases where the style system would refuse to parse a negative
<length-percentage>, but we got a negative percent basis, which would be weird.

But that's a bit misleading since right now at least we rely on the caller to do
the appropriate clamping. I also think that NS_WARNING_ASSERTION is not very
useful, since we're not very likely to catch stuff with it. But anyhow, your
call.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3a4d053ea82
Remove invalid NS_WARNING_ASSERTION. r=dholbert

Comment 4

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/d3a4d053ea82