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

RESOLVED FIXED in mozilla13

Status

()

Core
SVG
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: heycam)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla13
assertion, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

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

Comment 1

6 years ago
Created attachment 566327 [details]
stack trace
(Assignee)

Updated

6 years ago
Assignee: nobody → cam
Status: NEW → ASSIGNED
(Assignee)

Comment 2

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

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86_64 → All
(Assignee)

Updated

6 years ago
Whiteboard: [autoland-try]

Updated

6 years ago
Whiteboard: [autoland-try] → [autoland-in-queue]

Comment 3

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

6 years ago
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

Updated

6 years ago
Whiteboard: [autoland-in-queue]
(Assignee)

Comment 7

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

Comment 8

6 years ago
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.
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 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.
Attachment #594635 - Flags: review?(dholbert) → review+
(Assignee)

Comment 11

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

Thanks for the r+!  The delay was not appreciable. :)
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/0071109aa94e
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.