Last Comment Bug 683040 - bug 614879 forgot to update the signature on subclass SVGMotionSMILAnimationFunction
: bug 614879 forgot to update the signature on subclass SVGMotionSMILAnimationF...
Status: RESOLVED FIXED
: assertion
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
Depends on:
Blocks: 614879 681634
  Show dependency treegraph
 
Reported: 2011-08-29 17:00 PDT by Daniel Holbert [:dholbert]
Modified: 2011-09-07 07:57 PDT (History)
2 users (show)
dholbert: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.79 KB, image/svg+xml)
2011-08-31 11:03 PDT, Daniel Holbert [:dholbert]
no flags Details
fix (2.33 KB, patch)
2011-08-31 11:17 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
fix with reftest (5.77 KB, patch)
2011-08-31 12:27 PDT, Daniel Holbert [:dholbert]
bbirtles: review+
Details | Diff | Splinter Review
animateMotion-tweaked copies of bug 614879's reftests (2.93 KB, patch)
2011-08-31 13:54 PDT, Daniel Holbert [:dholbert]
bbirtles: review+
Details | Diff | Splinter Review
fix with reftest (whitespace fixed, noted r=birtles) (5.77 KB, patch)
2011-09-01 17:59 PDT, Daniel Holbert [:dholbert]
dholbert: review+
Details | Diff | Splinter Review
fix v2: directly check for |path| attr & <mpath> child (6.61 KB, patch)
2011-09-06 13:47 PDT, Daniel Holbert [:dholbert]
bbirtles: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2011-08-29 17:00:51 PDT
jcranmer told me on IRC that 
>88   NS_OVERRIDE virtual PRBool TreatSingleValueAsStatic() const;
https://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGMotionSMILAnimationFunction.h#88

is causing problems in static-analysis builds since the overridden method has been removed. 

This function needs to be removed/replaced.
Comment 1 Daniel Holbert [:dholbert] 2011-08-30 22:15:40 PDT
Taking; hoping to fix tomorrow morning.
Comment 2 Daniel Holbert [:dholbert] 2011-08-31 11:03:54 PDT
Created attachment 557248 [details]
testcase

Here's a testcase demonstrating what's broken here.

Basically, we have special-case handling for animation elements with the "to" attribute & no from/values attributes.

However, for <animateMotion>, the |path| attribute and <mpath> sub-element are both supposed to override "to".  Currently, if there's "to" and a path/mpath-specified animateMotion (the orange boxes in the attached testcase), we behave oddly -- snapping between endpoints rather than animating smoothly.

We need to override "IsToAnimation" (added in bug 614879) to fix this.
Comment 3 Daniel Holbert [:dholbert] 2011-08-31 11:17:59 PDT
Created attachment 557255 [details] [diff] [review]
fix

This fixes the attached testcase.

Still need to write a reftest or two - I'll post those in a followup patch.
Comment 4 Daniel Holbert [:dholbert] 2011-08-31 12:26:33 PDT
FWIW, the attached testcase & upcoming reftest also trigger this assertion in unpatched builds:
> ###!!! ASSERTION: Unexpected number of values: 'Error', file ../../../mozilla/content/smil/nsSMILAnimationFunction.cpp, line 396

I've verified that the attached patch fixes this assertion-failure.
Comment 5 Daniel Holbert [:dholbert] 2011-08-31 12:27:30 PDT
Created attachment 557274 [details] [diff] [review]
fix with reftest

(here's the fix with a reftest included)
Comment 6 Daniel Holbert [:dholbert] 2011-08-31 13:54:38 PDT
Created attachment 557310 [details] [diff] [review]
animateMotion-tweaked copies of bug 614879's reftests

Just for thoroughness, here are 2 more reftests -- copies of bug 614879's first 2 reftests, but tweaked to be for <animateMotion> instead of <animate>. (and tweaked to match lime.svg, since that's the standard reference case in the smil/motion reftests dir)

(These bonus reftests pass already, regardless of the fix on this bug here, but they fail in builds before bug 614879 landed.)
Comment 7 Daniel Holbert [:dholbert] 2011-08-31 13:56:41 PDT
(note that the bonus reftests patch I just attached uses "copy from... copy to..." w/ diffs on the copies, which bugzilla's diff viewer interprets as modifying the original files.  Don't let that alarm you. :))
Comment 8 Brian Birtles (:birtles) 2011-09-01 17:53:56 PDT
Comment on attachment 557274 [details] [diff] [review]
fix with reftest

Looks good. (Also, since I know you care about this sort of thing, jst's review tool says there is some trailing whitespace: http://beaufour.dk/jst-review/?patch=557274)
Comment 9 Daniel Holbert [:dholbert] 2011-09-01 17:57:30 PDT
oops, thanks! :)  I'll fix that before landing.
Comment 10 Daniel Holbert [:dholbert] 2011-09-01 17:59:39 PDT
Created attachment 557725 [details] [diff] [review]
fix with reftest (whitespace fixed, noted r=birtles)
Comment 11 Daniel Holbert [:dholbert] 2011-09-01 18:01:25 PDT
trailing-whitespace-free now. :)  http://beaufour.dk/jst-review/?patch=557725

(thanks for that link, BTW - I didn't know it was so easy to generate auto-jst-review URLs)
Comment 12 Daniel Holbert [:dholbert] 2011-09-06 13:47:23 PDT
Created attachment 558592 [details] [diff] [review]
fix v2: directly check for |path| attr & <mpath> child

As discussed last week on IRC, the previous fix failed the mochitest "test_smilAnimateMotionInvalidValues.xhtml" -- it wasn't changing the CTM immediately after a "pure to" animateMotion animation was applied.

This was actually due to a bug in the previous patch. IsToAnimation was relying on mPathSourceType, which doesn't get set to a useful value until we reach SVGMotionSMILAnimationFunction::GetValues(), and we depend on IsToAnimation() working before then.

(In the specific case of the mochitest failure, WillReplace was getting the wrong answer from IsToAnimation(), and so WillReplace returned the wrong answer ("true"), making us think we'd replace the underlying value.  This made us skip the base-value lookup -- but then when we try to use the base value later, we bail out and don't animate.

This was only a problem in the very first sample (since after that mPathSourceType is set) -- but luckily this mochitest depends on the very first sample being correct, so it caught the bug. :)

Anyway, the fix is to directly check for the |path| attribute and the <mpath> sub-element (those being the additional features that animateMotion brings along which can override the "to" attribute).
Comment 13 Daniel Holbert [:dholbert] 2011-09-06 13:48:42 PDT
Comment on attachment 558592 [details] [diff] [review]
fix v2: directly check for |path| attr & <mpath> child

(Note: The only differences in this new patch vs. the old one are in SVGMotionSMILAnimationFunction.h/cpp.  The tests are unchanged.)
Comment 14 Daniel Holbert [:dholbert] 2011-09-06 16:36:08 PDT
fix v2 passed a reftest/crashtest/mochitest Try run on mac/linux, btw:
  https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d33d489b39ec
Comment 15 Brian Birtles (:birtles) 2011-09-06 16:42:41 PDT
Comment on attachment 558592 [details] [diff] [review]
fix v2: directly check for |path| attr & <mpath> child

Looks great. Thanks Daniel!

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