Possible duplicate expression in SVGOrientSMILType::Interpolate()

RESOLVED FIXED in mozilla11

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Dolske, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
mozilla11
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [pvs-studio])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
From http://www.viva64.com/en/a/0078/,
8th section in http://www.viva64.com/external-pictures/txt/mozilla-test.txt

V501 There are identical sub-expressions to the left and to the right of the '||' operator.
svgorientsmiltype.cpp 161

nsresult
SVGOrientSMILType::Interpolate(...)
{
  ...
  if (aStartVal.mU.mOrient.mOrientType !=
      nsIDOMSVGMarkerElement::SVG_MARKER_ORIENT_ANGLE ||
      aStartVal.mU.mOrient.mOrientType !=
      nsIDOMSVGMarkerElement::SVG_MARKER_ORIENT_ANGLE) {
    // TODO: it would be nice to be able to handle auto angles too.
    return NS_ERROR_FAILURE;
  }
  ...
(Reporter)

Updated

5 years ago
Blocks: 710966
Whiteboard: [pvs-studio] → [pvs-studio][good first bug][lang=c++]

Comment 1

5 years ago
One of those aStartVal should be an aEndVal
(Assignee)

Updated

5 years ago
Flags: in-testsuite?
Version: unspecified → 1.0 Branch
(Assignee)

Updated

5 years ago
Version: 1.0 Branch → Trunk
(Assignee)

Comment 2

5 years ago
This code was added in bug 545042.
Blocks: 545042
(Assignee)

Comment 3

5 years ago
Right now if we try to interpolate for an animation with e.g.
  from="180deg" to="auto"
we end up treating "auto" as if it were "0deg", at least in my debug build, because that's the (bogus) angle-value associated with our "auto" value in the interpolation code. It may be that in optimized builds we end up with a nonzero value there & get random results.

(However, once the animation completes, if it's got fill='freeze', we'll skip the interpolation call and just directly set the correct "auto" value.)
(Assignee)

Comment 4

5 years ago
I'll just take this; it affects behavior, so best to fix it quickly, and I've already got a testcase in mind based on the previous comment.
Assignee: nobody → dholbert
Whiteboard: [pvs-studio][good first bug][lang=c++] → [pvs-studio]
(Assignee)

Comment 5

5 years ago
Created attachment 582073 [details] [diff] [review]
fix v1 (with reftest)

This includes a reftest for animating the "orient" attribute to the value "auto".

The reftest checks two things:
 (a) completed frozen animations to "auto" should end up at "auto"
 (b) in-progress animations to "auto" should fall back to discrete-mode (still be at the initial value for at least the first half of the animation), since interpolation fails.

We succeed at (a) already, but current trunk fails at (b) as described in comment 3.  I verified that the reftest as a whole fails pre-patch and passes post-patch.

(I used "hg cp" to create the reftest, so don't be tricked by bugzilla's diff viewer making it look like I'm modifying an existing file. :))
Attachment #582073 - Flags: review?(jwatt)
hg add anim-marker-orient-02.svg ?
(Assignee)

Comment 7

5 years ago
It's hg cp'd.  (see last sentence in my last comment :) )
Comment on attachment 582073 [details] [diff] [review]
fix v1 (with reftest)

Oops. :) r=jwatt
Attachment #582073 - Flags: review?(jwatt) → review+

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/4c22853f71ce
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11

Updated

5 years ago
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.