Last Comment Bug 678822 - "ASSERTION: wrong type" with invalid SVG @accumulate attribute
: "ASSERTION: wrong type" with invalid SVG @accumulate attribute
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Daniel Holbert [:dholbert]
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 617817
  Show dependency treegraph
 
Reported: 2011-08-14 06:34 PDT by Jesse Ruderman
Modified: 2011-08-16 04:09 PDT (History)
4 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (triggers assertion after a second) (102 bytes, image/svg+xml)
2011-08-14 06:34 PDT, Jesse Ruderman
no flags Details
stack traces (3.71 KB, text/plain)
2011-08-14 06:35 PDT, Jesse Ruderman
no flags Details
fix: add check for mErrorFlags (1.83 KB, patch)
2011-08-14 22:36 PDT, Daniel Holbert [:dholbert]
bbirtles: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-08-14 06:34:49 PDT
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.
Comment 1 Jesse Ruderman 2011-08-14 06:35:12 PDT
Created attachment 552971 [details]
stack traces
Comment 2 Daniel Holbert [:dholbert] 2011-08-14 22:18:29 PDT
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] 2011-08-14 22:36:32 PDT
Created attachment 553110 [details] [diff] [review]
fix: add check for mErrorFlags
Comment 4 Daniel Holbert [:dholbert] 2011-08-14 22:50:36 PDT
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) 2011-08-15 17:12:01 PDT
Comment on attachment 553110 [details] [diff] [review]
fix: add check for mErrorFlags

Looks great Daniel! Thanks for taking this!
Comment 6 Daniel Holbert [:dholbert] 2011-08-15 18:15:25 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/d06c39b49453
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-16 04:09:37 PDT
http://hg.mozilla.org/mozilla-central/rev/d06c39b49453

Note You need to log in before you can comment on or make changes to this bug.