556841 - Assertions: "Wrong number of elements in from value", "invalid array index", etc. with animateTransform by-animation w/ calcMode="paced"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

See attached testcase, which produces these assertion failures & warnings in every SMIL sample in its duration:
{
> ###!!! ASSERTION: Wrong number of elements in from value: 'fromTransforms->Length() == 1', file ../../../../../mozilla/content/svg/content/src/nsSVGTransformSMILType.cpp, line 217
> ###!!! ASSERTION: invalid array index: 'i < Length()', file ../../../../dist/include/nsTArray.h, line 333
> ###!!! ASSERTION: Incompatible transform types to calculate distance between: 'fromTransform.mTransformType == toTransform.mTransformType', file ../../../../../mozilla/content/svg/content/src/nsSVGTransformSMILType.cpp, line 224
> ###!!! ASSERTION: Got bad transform types for calculating distances: 'Error', file ../../../../../mozilla/content/svg/content/src/nsSVGTransformSMILType.cpp, line 253
> ###!!! ASSERTION: Trying to do sandwich add of more than one value: 'srcTransforms.Length() == 1', file ../../../../../mozilla/content/svg/content/src/nsSVGTransformSMILType.cpp, line 187
> ###!!! ASSERTION: invalid array index: 'i < Length()', file ../../../../dist/include/nsTArray.h, line 333
> WARNING: Unexpected transform type: file ../../../../../mozilla/content/svg/content/src/nsSVGTransformSMILAttr.cpp, line 439
> WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file ../../../../../mozilla/content/svg/content/src/nsSVGTransformSMILAttr.cpp, line 382
> WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file ../../../../../mozilla/content/svg/content/src/nsSVGTransformSMILAttr.cpp, line 134
> WARNING: nsISMILAttr::SetAnimValue failed: file ../../../mozilla/content/smil/nsSMILCompositor.cpp, line 131
}

These failures are because the assumption in the comment at line nsSVGTransformSMILType.cpp:211 (below) isn't correct -- it assumes we can only have paced-mode when there's a 'values' array, when really it can happen with pure "by" animation (with just one "by" value that we're interpolating across) as well:

197 nsresult
198 nsSVGTransformSMILType::ComputeDistance(const nsSMILValue& aFrom,
199                                         const nsSMILValue& aTo,
200                                         double& aDistance) const
201 {
[...]
211   // ComputeDistance is only used for calculating distances between single
212   // values in a values array which necessarily have the same type
213   //
214   // So we should only have one transform in each array and they should be of
215   // the same type
216   NS_ASSERTION(fromTransforms->Length() == 1,
217     "Wrong number of elements in from value");
218   NS_ASSERTION(toTransforms->Length() == 1,
219     "Wrong number of elements in to value");

The correct fix might be to *make* that assumption correct, by treating paced-calcMode animation just like linear-calcMode (and skipping any distance-computation) whenever we've got <=2 values.  Note that paced-calcMode and linear-calcMode should behave the same in that circumstance.

Comment 1

[Daniel Holbert [:dholbert]]

Note: Despite the scary-sounding "ASSERTION: invalid array index" message, I don't think this is a security issue.  This is a fairly constrained reading-uninitialized-memory problem, with all of the reading localized in the switch statement of nsSVGTransformSMILType::ComputeDistance:
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGTransformSMILType.cpp#197

Most of the time, we'll fall into the "default" case and do nothing, since fromTransform.mTransformType is uninitialized and out-of-bounds for a valid transform-type.  And when that doesn't happen, we'll read a few uninitialized floats and use them for computing a euclidean distance.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> These failures are because the assumption in the comment at line
> nsSVGTransformSMILType.cpp:211 (below) isn't correct

Actually, the comment *is* technically correct, regarding our underlying "values" array  (generated by nsSMILAnimationFunction::GetValues).  (I initially read it to mean "the 'values' attribute", and it wouldn't be correct in that sense.)

But the problem we run into is that, in the case of pure by-animation, the first entry in our |values| array is a just-barely-initialized "dummy" value.   This is shown here:

739 nsSMILAnimationFunction::GetValues(const nsISMILAttr& aSMILAttr,
[...]
746   nsSMILValueArray result;
[...]
785     } else if (!by.IsNull()) {
786       nsSMILValue effectiveFrom(by.mType);
787       if (!from.IsNull())
788         effectiveFrom = from;
789       // Set values to 'from; from + by'
790       result.AppendElement(effectiveFrom);
791       nsSMILValue effectiveTo(effectiveFrom);
792       if (!effectiveTo.IsNull() && NS_SUCCEEDED(effectiveTo.Add(by))) {
793         result.AppendElement(effectiveTo);

If we lack a "from" value, the first element appended to our |values| array ("result" in the code above) is a just-barely-initialized nsSMILValue (with mType == nsSVGTransformSMILType::sSingleton in this case).  And since it's just-barely-initialized, it has an empty transform-array, and that's what ends up confusing us in nsSVGTransformSMILType::ComputeDistance.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Simple fix -- just adds a special case in ComputePacedPosition for the trivial 2-values-case, with no distance-computation.

NOTE: I also add an ABORT_IF_FALSE to assert we have *at least* 2 values in that function, to make that more clear.  (That's ensured by an early check in nsSMILAnimationFunction::InterpolateResult[1], which is the only caller of ComputePacedPosition.)

[1] http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILAnimationFunction.cpp#400

Comment 4

[Daniel Holbert [:dholbert]]

Landed: http://hg.mozilla.org/mozilla-central/rev/fabb17b2c94e