Last Comment Bug 693790 - "ABORT: Expecting at least one non-identity operand"
: "ABORT: Expecting at least one non-identity operand"
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla13
Assigned To: Cameron McCormack (:heycam)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 369230
  Show dependency treegraph
 
Reported: 2011-10-11 13:42 PDT by Jesse Ruderman
Modified: 2012-02-09 10:26 PST (History)
4 users (show)
cam: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (asserts fatally when loaded) (144 bytes, image/svg+xml)
2011-10-11 13:42 PDT, Jesse Ruderman
no flags Details
stack trace (1.96 KB, text/plain)
2011-10-11 13:43 PDT, Jesse Ruderman
no flags Details
Don't abort on discrete by-animation of SVG length lists when there is no underlying value. (5.69 KB, patch)
2012-02-05 22:01 PST, Cameron McCormack (:heycam)
dholbert: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-10-11 13:42:50 PDT
Created attachment 566326 [details]
testcase (asserts fatally when loaded)

###!!! ABORT: Expecting at least one non-identity operand: '!dest.IsEmpty() || !valueToAdd.IsEmpty()', file content/svg/content/src/SVGLengthListSMILType.cpp, line 127
Comment 1 Jesse Ruderman 2011-10-11 13:43:08 PDT
Created attachment 566327 [details]
stack trace
Comment 2 Cameron McCormack (:heycam) 2012-02-05 22:01:05 PST
Created attachment 594635 [details] [diff] [review]
Don't abort on discrete by-animation of SVG length lists when there is no underlying value.

It looks like doing an addition with an empty list as both the underlying value
and the to value is a valid case that can come up; specifically, when doing a
discrete by-animation where there is no explicit underlying value.

The patch just removes the assertion and makes that case a no-op.
Comment 3 Mozilla RelEng Bot 2012-02-05 22:04:58 PST
Autoland Patchset:
	Patches: 594635
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/rev/1d9e0a0ab986
Try run started, revision 1d9e0a0ab986. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=1d9e0a0ab986
Comment 4 Daniel Holbert [:dholbert] 2012-02-05 23:56:22 PST
(In reply to Cameron McCormack (:heycam) from comment #2)
> It looks like doing an addition with an empty list as both the underlying
> value
> and the to value is a valid case that can come up; specifically, when doing a
> discrete by-animation where there is no explicit underlying value.

I'm not immediately clear on why that happens here.  I'd thought we did an addition like so:
 [ baseVal + byVal * progress]
and while baseVal is empty there for this testcase, the byVal is not.

Why is the added value (the by value, presumably) not-explicitly-initialized here?
Comment 5 Daniel Holbert [:dholbert] 2012-02-06 00:09:10 PST
(The attached patch is likely correct -- I just want to make sure we know what both non-initialized values represent, to be sure the bug isn't elsewhere.)
Comment 6 Mozilla RelEng Bot 2012-02-06 02:00:29 PST
Try run for 1d9e0a0ab986 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1d9e0a0ab986
Results (out of 206 total builds):
    success: 186
    warnings: 20
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-1d9e0a0ab986
Comment 7 Cameron McCormack (:heycam) 2012-02-06 04:06:38 PST
(In reply to Daniel Holbert [:dholbert] from comment #4)
> I'm not immediately clear on why that happens here.  I'd thought we did an
> addition like so:
>  [ baseVal + byVal * progress]
> and while baseVal is empty there for this testcase, the byVal is not.

Only for the non-discrete calcModes.  For discrete by-animations, it is treated as a two-valued animation where the first value is the "identity" value and the second value is the by="" value.

> Why is the added value (the by value, presumably) not-explicitly-initialized
> here?

I think the equivalent additive from/to animation values are computed here:

  http://hg.mozilla.org/mozilla-central/file/tip/content/smil/nsSMILAnimationFunction.cpp#l837

resulting in the first value in the length 2 result array being the empty list.

It's then selected as part of the discrete animation here:

  http://hg.mozilla.org/mozilla-central/file/tip/content/smil/nsSMILAnimationFunction.cpp#l496
Comment 8 Cameron McCormack (:heycam) 2012-02-06 04:07:54 PST
Sorry, not equivalent additive from-to animation; it's non-additive.  nsSMILAnimationFunction::GetValues turns the by-animation into a non-additive from-to animation by looking up the base value (the empty list) to use as the from value, and adding that to the by="" value to use as the to value.
Comment 9 Daniel Holbert [:dholbert] 2012-02-08 11:14:14 PST
Gotcha -- so in particular:
* nsSMILAnimationFunction::ComposeResult does its discrete "interpolation" between identity & the "by" value  (producing the identity value, for the first half of the animation)
* In the final chunk of ComposeResult, we try to SandwichAdd that result onto the underlying base value, which is also identity.

So we get identity+identity.  And of course the right result for identity += identity is just to leave the value as-is & succeed, which this patch does.  Cool.
Comment 10 Daniel Holbert [:dholbert] 2012-02-08 11:19:41 PST
Comment on attachment 594635 [details] [diff] [review]
Don't abort on discrete by-animation of SVG length lists when there is no underlying value.

r=me! Sorry for the delay on this.
Comment 11 Cameron McCormack (:heycam) 2012-02-08 14:23:17 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/0071109aa94e

Thanks for the r+!  The delay was not appreciable. :)
Comment 12 Ed Morley [:emorley] 2012-02-09 10:26:18 PST
https://hg.mozilla.org/mozilla-central/rev/0071109aa94e

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