531550 - "ASSERTION: Trying to do sandwich add of more than one value" with svg:animateTransform

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Trying to do sandwich add of more than one value.: 'srcTransforms.Length() == 1', file /Users/jruderman/central/content/svg/content/src/nsSVGTransformSMILType.cpp, line 169

###!!! ASSERTION: invalid array index: 'i < Length()', file ../../../../dist/include/nsTArray.h, line 326

The first assertion is misleading: srcTransforms.Length() is actually 0.

Comment 1

[Jesse Ruderman]

This prevents me from finding other "ASSERTION: invalid array index" bugs, which are often security holes :(

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Patch v1a

Proposed patch.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 491746 [details] [diff] [review]
Patch v1a

>+  // [...] but since the duration is indefinite we'll actually try
>+  // to add it.

I'm confused about what this means -- why does an indefinite duration mean we'll actually try to add, when we wouldn't otherwise?

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to comment #3)
> I'm confused about what this means -- why does an indefinite duration mean
> we'll actually try to add, when we wouldn't otherwise?

Yeah, good point this comment needs to be tweaked. For by-animation, normally the interpolation step would ensure that we don't end up with an empty transform animation value. However, when there's an indefinite duration we skip interpolation altogether and just set the first value (the empty 'from' value we'd normally interpolate from).

However, I think there are other cases where this can arise too such as when we have values="1". So I'll tweak the comment to make this clearer.

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Patch v1b

Fix up comment as per comment 3.

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 492618 [details] [diff] [review]
Patch v1b

Makes much more sense now -- thanks for clarifying that! r=dholbert

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Pushed: http://hg.mozilla.org/mozilla-central/rev/b578485c389e