693790 - "ABORT: Expecting at least one non-identity operand"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: 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]

Attachment: stack trace

Comment 2

[Cameron McCormack (:heycam)]

Attachment: 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]

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]]

(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]]

(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]

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/[e-mail]-1d9e0a0ab986

Comment 7

[Cameron McCormack (:heycam)]

(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)]

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]]

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]]

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)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0071109aa94e

Thanks for the r+!  The delay was not appreciable. :)

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0071109aa94e