Closed Bug 693496 Opened 8 years ago Closed 6 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jruderman, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [fuzzblocker:CSS-calc])

Attachments

(3 files)

###!!! ABORT: only used on properties that always have percent in calc: 'v1.mHasPercent || v2.mHasPercent', file layout/style/nsStyleAnimation.cpp, line 783
Attached file stack trace
David, why does this assert make sense when one of the values is eCSSUnit_Calc as here and the other is a coord?
If it's a calc() value in this function, its mHasPercent should be true always, no?
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?
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.)
Whiteboard: [fuzzblocker:CSS-calc]
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).
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
The additional tests hit the ABORT without the patch and pass with the patch.
Attachment #763927 - Flags: review?(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
Attachment #763927 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/b65294b69a5b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.