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

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)

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 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 on attachment 785365 [details] [diff] [review] patch I've got what might be better idea, I think. More details in a few minutes.

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

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

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.

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

Thanks!