The default bug view has changed. See this FAQ.

bug 614879 forgot to update the signature on subclass SVGMotionSMILAnimationFunction

RESOLVED FIXED in mozilla9

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

({assertion})

Trunk
mozilla9
assertion
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

6 years ago
Taking; hoping to fix tomorrow morning.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 2

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

Comment 3

6 years ago
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.
Attachment #557255 - Flags: review?(birtles)
(Assignee)

Comment 4

6 years ago
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.
Keywords: assertion
(Assignee)

Comment 5

6 years ago
Created attachment 557274 [details] [diff] [review]
fix with reftest

(here's the fix with a reftest included)
Attachment #557255 - Attachment is obsolete: true
Attachment #557255 - Flags: review?(birtles)
Attachment #557274 - Flags: review?(birtles)
(Assignee)

Updated

6 years ago
Flags: in-testsuite?
(Assignee)

Comment 6

6 years ago
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.)
Attachment #557310 - Flags: review?(birtles)
(Assignee)

Comment 7

6 years ago
(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 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)
Attachment #557274 - Flags: review?(birtles) → review+
Attachment #557310 - Flags: review?(birtles) → review+
(Assignee)

Comment 9

6 years ago
oops, thanks! :)  I'll fix that before landing.
(Assignee)

Comment 10

6 years ago
Created attachment 557725 [details] [diff] [review]
fix with reftest (whitespace fixed, noted r=birtles)
Attachment #557274 - Attachment is obsolete: true
Attachment #557725 - Flags: review+
(Assignee)

Comment 11

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

Comment 12

6 years ago
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).
Attachment #557725 - Attachment is obsolete: true
(Assignee)

Comment 13

6 years ago
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.)
Attachment #558592 - Flags: review?(birtles)
(Assignee)

Comment 14

6 years ago
fix v2 passed a reftest/crashtest/mochitest Try run on mac/linux, btw:
  https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d33d489b39ec
Comment on attachment 558592 [details] [diff] [review]
fix v2: directly check for |path| attr & <mpath> child

Looks great. Thanks Daniel!
Attachment #558592 - Flags: review?(birtles) → review+
(Assignee)

Comment 16

6 years ago
Thanks!
 http://hg.mozilla.org/integration/mozilla-inbound/rev/12507ee12f19
 http://hg.mozilla.org/integration/mozilla-inbound/rev/3ae9b2a742df
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
http://hg.mozilla.org/mozilla-central/rev/12507ee12f19
http://hg.mozilla.org/mozilla-central/rev/3ae9b2a742df
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.