678822 - "ASSERTION: wrong type" with invalid SVG @accumulate attribute

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (triggers assertion after a second)

###!!! ASSERTION: wrong type: 'Type() == eEnum', file content/base/src/nsAttrValue.h, line 415

###!!! ASSERTION: wrong type: 'BaseType() == eOtherBase', file content/base/src/nsAttrValue.h, line 507

Found by a new dom fuzzer component that modifies existing attributes.

Comment 1

[Jesse Ruderman]

Attachment: stack traces

Comment 2

[Daniel Holbert [:dholbert]]

OK - so in most cases, we don't call the attr-getters like "GetAccumulate" until after we've checked mErrorFlags to be sure that all attrs have parsed correctly.

However, Jesse's found one case where we do call GetAccumulate() without having checked those flags -- in SampleAt().  This call was added in bug 617817, so I'm assuming this assertion-failure only dates back that far.

We should be able to fix this by just adding a "mErrorFlags" check around that call.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix: add check for mErrorFlags

Comment 4

[Daniel Holbert [:dholbert]]

I assert that this patch won't change our rendering in any circumstances. (which is good).  Explanation below.

The line that I'm putting inside a conditional is just for turning on mHasChanged under certain circumstances.

Now, suppose mErrorFlags is set and we now skip that line.  That means our animation isn't going to participate in the animation sandwich.  So the "mHasChanged" assignment doesn't make any difference in that case, anyway, since we're not going to participate in the sandwich.

The one caveat (but not really) to the above is: we do need to make sure to toggle mHasChanged *when the invalid value is set*, to be sure we get a sample ASAP at that point, so that the animation's effects will be removed.  That'll be handled elsewhere, though -- specifically, the attribute-setting function (e.g. SetAccumulate in this case) will set mHasChanged, so we'll be covered.

Comment 5

[Brian Birtles (:birtles)]

Comment on attachment 553110 [details] [diff] [review]
fix: add check for mErrorFlags

Looks great Daniel! Thanks for taking this!

Comment 6

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d06c39b49453

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/d06c39b49453