Closed Bug 1275067 Opened 3 years ago Closed 3 years ago

Make nsStyleCoord::ToLength default to safer behavior

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

Details

Attachments

(1 file)

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).
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
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).
Attachment #8755549 - Flags: review?(cam) → review+
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
https://hg.mozilla.org/mozilla-central/rev/484c1b1957f5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.