Closed Bug 678822 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: jruderman, Assigned: dholbert)

References

Details

(Keywords: assertion, testcase)

Attachments

(3 files)

###!!! 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.
Attached file stack traces
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.
Blocks: 617817
OS: Mac OS X → All
Hardware: x86 → All
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #553110 - Flags: review?(birtles)
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 on attachment 553110 [details] [diff] [review]
fix: add check for mErrorFlags

Looks great Daniel! Thanks for taking this!
Attachment #553110 - Flags: review?(birtles) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/d06c39b49453
Flags: in-testsuite+
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
http://hg.mozilla.org/mozilla-central/rev/d06c39b49453
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.