693496 - "ABORT: only used on properties that always have percent in calc"

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

Attachment: testcase (asserts fatally when loaded)

###!!! ABORT: only used on properties that always have percent in calc: 'v1.mHasPercent || v2.mHasPercent', file layout/style/nsStyleAnimation.cpp, line 783

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Boris Zbarsky [:bzbarsky]]

David, why does this assert make sense when one of the values is eCSSUnit_Calc as here and the other is a coord?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

If it's a calc() value in this function, its mHasPercent should be true always, no?

Comment 4

[Boris Zbarsky [:bzbarsky]]

So what happens here is that we're in the value-pair case for eCSSProperty_border_top_left_radius.

pair1->mXValue is (eCSSUnit_Pixel, 0).
pair2->mXValue is (eCSSUnit_Calc, calc(5px)).

So we end up with "calc" as the common unit.

It looks like AddWeighted handles the eUnit_Calc case when neither value has percent, right?  So does it need to do likewise for value pairs?

Comment 5

[Daniel Holbert [:dholbert]]

FWIW: If you edit this mochitest...
https://mxr.mozilla.org/mozilla-central/source/layout/style/test/test_transitions_per_property.html?force=1
...to remove all the entries from "supported_properties" (so it's just an empty hash {}), then we fail this assertion during that mochitest, for the property "background-size".

(Ran up against that when locally tweaking the test to debug something unrelated.)

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I think my original intent here was that SetCalcValue actually do something different from its current behavior, i.e., that it produce a raw length rather than a calc expression when !aCalc->mHasPercent.  Though I originally wrote it the way it is now in https://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/a954b181e431 , but I think I may have meant otherwise.

I suppose it's probably safer to switch to defining canonical-form calc as either calc(length) or calc(length + percent).

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Fix ABORT by being consistent that canonical form calc() values need not have a percent part.

The additional tests hit the ABORT without the patch and pass with the patch.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 763927 [details] [diff] [review]
Fix ABORT by being consistent that canonical form calc() values need not have a percent part.

> static void
> AddCSSValueCanonicalCalc(double aCoeff1, const nsCSSValue &aValue1,
>                          double aCoeff2, const nsCSSValue &aValue2,
>                          nsCSSValue &aResult)
> {
[...]
>+  CalcValue result;
>+  result.mLength = aCoeff1 * v1.mLength + aCoeff2 * v2.mLength;
>+  result.mPercent = aCoeff1 * v1.mPercent + aCoeff2 * v2.mPercent;
>+  result.mHasPercent = v1.mHasPercent || v2.mHasPercent;

Observation: it looks like in all the places where we create a CalcValue, we explicitly set mPercent to 0 in the "mHasPercent is false" case.

Your patch maintains that invariant for 'result', albeit with some arithmetic involved (and assuming that the invariant holds true for v1 and v2).

To make it clearer that this invariant still holds (and to implicitly sanity-check our args & arithmetic), it'd be worth asserting that the invariant holds in the freshly-created CalcValue, with something like:

 MOZ_ASSERT(result.mHasPercent || result.mPercent == 0.0f,
            "Creating a CalcValue whose mPercent disagrees with mHasPercent "
            "(bad input values?)");

r=me with something like that

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b65294b69a5b

Comment 10

[Ed Morley [:emorley]]

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