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

RESOLVED FIXED in mozilla8

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: dholbert)

Tracking

({assertion, testcase})

Trunk
mozilla8
assertion, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
Created attachment 552970 [details]
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.
(Reporter)

Comment 1

6 years ago
Created attachment 552971 [details]
stack traces
(Assignee)

Comment 2

6 years ago
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
(Assignee)

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 3

6 years ago
Created attachment 553110 [details] [diff] [review]
fix: add check for mErrorFlags
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #553110 - Flags: review?(birtles)
(Assignee)

Comment 4

6 years ago
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+
(Assignee)

Comment 6

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.