1275067 - Make nsStyleCoord::ToLength default to safer behavior

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

Right now, nsStyleCoord's "ToLength()" static method is implemented with a special case for exact coord units, and then it falls back to assuming Calc for non-coord values. Code quote:

> 187   static nscoord ToLength(nsStyleUnit aUnit, nsStyleUnion aValue) {
> 188     MOZ_ASSERT(ConvertsToLength(aUnit, aValue));
> 189     if (aUnit == eStyleUnit_Coord) {
> 190       return aValue.mInt;
> 191     }
> 192     MOZ_ASSERT(IsCalcUnit(aUnit) && !AsCalcValue(aValue)->mHasPercent);
> 193     return AsCalcValue(aValue)->ToLength();
> 194   }
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleCoord.h?rev=b17cd6fd7cd0#187

I'd like to flip the logic -- treat Calc as a special case, and then fall back to coord units if we're non-Calc.

When our assertions hold up, the two formulations should be equivalent.

But importantly, when our assertions do *NOT* hold up (e.g. if a non-calc & non-coord value gets passed in to ToLength), then the new behavior (fallback to coord) will be strictly safer than the current behavior (fallback to Calc, i.e. read a member variable off of an arbitrary blob of memory pointed to by our bogus not-actually-calc-typed tagged union).

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: MozReview Request: Bug 1275067: Flip logic in nsStyleCoord::ToLength, for safer general-case behavior. r?heycam

Review commit: https://reviewboard.mozilla.org/r/54668/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54668/

Comment 3

[Daniel Holbert [:dholbert]]

Note that the "if (IsCalcUnit(aUnit))" check in the patch shouldn't be any more expensive than the check that it's replacing.

IsCalcUnit() is implemented as a one-liner in the same header file, and its impl  ("return aUnit == eStyleUnit_Calc") looks pretty much identical to the removed check here (except for the unit being different).

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8755549 [details]
MozReview Request: Bug 1275067: Flip logic in nsStyleCoord::ToLength, for safer general-case behavior. r?heycam

https://reviewboard.mozilla.org/r/54668/#review51642

Comment 5

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d26bc1ac4bc

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/484c1b1957f5

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/484c1b1957f5