898915 - "ABORT: Axis() isn't valid" with SVG animate

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ABORT: Axis() isn't valid: 'mElement', file content/svg/content/src/SVGLengthList.h, line 200

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Robert Longson [:longsonr]]

Attachment: patch

Comment 3

[Brian Birtles (:birtles)]

Thanks for taking care of this Robert. Why is the check for CanZeroPadList necessary?

Is it because <animate attributeName="y" from="" to=""/> should return an empty list with CanZeroPadList() == false and so shouldn't take this shortcut?

I'm not sure what "discrete by-animation" means here. Does it mean an empty list? Or are we actually falling back to calcMode=discrete here?

CC'ing dholbert since he fixed a similar bug in this area (bug 641393)

Comment 4

[Robert Longson [:longsonr]]

I'm using CanZeroPadList = true to determine whether the lengthList is a zero valued SMIL engine created object per: http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGLengthListSMILType.h#32

In the abort case both the startVal and endVal seem to be such objects.

The comment is copied from http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGLengthListSMILType.cpp#96, I assumed this was a similar issue where there's no base value and we have by-animation.

Bug 641393 seems to have introduced this issue by creating a SetInfo call.

Comment 5

[Brian Birtles (:birtles)]

Comment on attachment 785365 [details] [diff] [review]
patch

(In reply to Robert Longson from comment #4)
> I'm using CanZeroPadList = true to determine whether the lengthList is a
> zero valued SMIL engine created object per:
> http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/
> SVGLengthListSMILType.h#32
> 
> In the abort case both the startVal and endVal seem to be such objects.

Yes, I understand that, I'm just curious if removing those checks would actually be ok.

It might come down to whether the following should be allowed to zero-pad or not:

  <animate attributeName="y" from="" to=""/>
  <animate attributeName="y" additive="sum" to="5 10 15"/>

I think the condition is probably safe as it is, but I'd like to see, anyway, a test for <animate attributeName="y" from="" to=""/> (which will have CanZeroPadList = false I guess) just to make sure it doesn't generate a similar problem.

> The comment is copied from
> http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/
> SVGLengthListSMILType.cpp#96, I assumed this was a similar issue where
> there's no base value and we have by-animation.

I don't think the comment makes sense here though because I don't think this only occurs with discrete animation. I'm not sure if it makes sense in the original context either but I haven't looked into it.

r=me with a test for from="" to="" and the reference to discrete animation removed.

If you don't have time to look into the issues I raised I can dig into them tomorrow.

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 785365 [details] [diff] [review]
patch

I've got what might be better idea, I think. More details in a few minutes.

Comment 7

[Daniel Holbert [:dholbert]]

So what happens here is:
 (a) we parse the "by" attribute successfully (and save element/axis info on it)
 (b) We create an empty "from" value (no element/axis info)
 (c) We synthesize a "to" value by calling Add() on 'from' and 'by'
   - The addition succeeds, but it takes a success case that doesn't copy any element/axis info into the result.
 (d) We interpolate between our "from" and our "to" value, both of which lack element/axis info.

The attached patch fixes this by adding a special case in step (d), but I think we should fix it by propagating the element/axis info in step (c), from our "by" value to the addition-result (the synthesized "to" value).

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: patch v2

Rather than describing what I think should happen, I figured I'd just attach a patch, since I already verified locally that this fixes the abort.

This swaps two early-return success cases in Add(), so that when we get to the early-return that we hit for this testcase, we already know that valueToAdd.Element() is non-null and hence we can propagate element, axis, etc. from |valueToAdd| to the result.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 786373 [details] [diff] [review]
patch v2

Actually, if we delete the "Adding two identity values" clause that I'm modifying entirely, I think the right behavior might just fall out...

Comment 10

[Daniel Holbert [:dholbert]]

Yeah, that's easier to describe in a bug-comment. I've convinced myself that we'll do the right thing if we simply drop that whole "Adding two identity values" clause -- this whole block of code:
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGLengthListSMILType.cpp?rev=9e310bf47f50&mark=96-100#96

We should handle the "adding two identity values" situation correctly in the code below that (copying Element() etc. as-necessary).

r=me if you change your original patch to make that deletion, instead of its current code changes (i.e. don't modify ::Interpolate), assuming you agree.

Comment 11

[Robert Longson [:longsonr]]

That seems to work for the testcase. I'll send it to try and see how we get on.

Comment 12

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=6abe4babce64

Comment 13

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afb4118f4479

Comment 14

[Daniel Holbert [:dholbert]]

Thanks!

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/afb4118f4479