574750 - Code that iterates through nsCSSValue::Arrays should use size_t for index now

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Now that Bug 574059 landed and nsCSSValue::Array::Count() returns a size_t (instead of PRUint16), we need to do a s/PRUint16/size_t/ on code that iterates through these arrays, too.

e.g.
278     nsCSSValue::Array *array = aValue.GetArrayValue();
[...]
330     for (PRUint16 index = 1; index < array->Count(); ++index) {
331       AppendCSSValueToString(aProperty, array->Item(index), aResult);
332 
333       /* If we're not at the final element, append a comma. */
334       if (index + 1 != array->Count())
335         aResult.AppendLiteral(", ");
336     }
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSDeclaration.cpp

Comment 1

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: fix

I think this catches all the instances.  There are a few that don't matter as much (e.g. in nsStyleAnimation, where we use a PRUint32 as the loop counter, and we're only iterating up to 4), but I figured we might as well fix those too for consistency.

Comment 2

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(meant to say: haven't compiled with the attached patch yet -- doing that now)

Comment 3

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Comment on attachment 454105 [details] [diff] [review]
fix

This builds fine on my machine.  bz, would you mind reviewing this?

Comment 4

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/619563f026f5

Requesting blocking1.9.1 & 1.9.2, to avoid complications on those branches from comparing PRUint16 list-counter vs. a size_t Count() method as a result of bug 574059.

Comment 5

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Marking as security-sensitive at reed's suggestion, to be on the safe side.

Comment 6

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: fix for 1.9.2 & 1.9.1

Here's the fix backported to 1.9.2 -- basically just removes the chunks for CSS calc() and nsStyleAnimation (which are new on m-c since 1.9.2 branched), and fixes contextual code.

This patch applies fine to 1.9.1, too, with fuzz=3 (for minor changes in contextual code).

Comment 7

[LegNeato]

Comment on attachment 454109 [details] [diff] [review]
fix for 1.9.2 & 1.9.1

a=LegNeato for 1.9.2.6 and 1.9.2.11.

Please submit to mozilla-1.9.2 default and mozilla-1.9.1 default asap, as code freeze is tonight!

Comment 8

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Thanks for the quick turnaround on approval!

I'm going to wait for this to cycle on mozilla-central first, before landing the 1.9.1 / 1.9.2 version.

Comment 9

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f2a4c8e82d16
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/025191a13eb7

Comment 10

[Al Billings [:abillings - ex-MoCo]]

Is there a means for QA to verify this fix for 1.9.2.7 and 1.9.1.11?

Comment 11

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Only via code inspection (making sure the patch landed).

I noticed the problem via code inspection in the first place -- we never had a testcase (though it's probably possible to make one).