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

RESOLVED FIXED in mozilla26

Status

()

defect
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jruderman, Assigned: longsonr)

Tracking

(Blocks 1 bug, {assertion, testcase})

Trunk
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

6 years ago
Posted image testcase
###!!! ABORT: Axis() isn't valid: 'mElement', file content/svg/content/src/SVGLengthList.h, line 200
Reporter

Comment 1

6 years ago
Posted file stack
Assignee

Comment 2

6 years ago
Posted patch patchSplinter Review
Assignee: nobody → longsonr
Attachment #785365 - Flags: review?(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)
Flags: needinfo?(longsonr)
Assignee

Comment 4

6 years ago
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.
Blocks: 641393
Flags: needinfo?(longsonr)
Assignee

Updated

6 years ago
Attachment #785365 - Flags: review?(dholbert)
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.
Attachment #785365 - Flags: review?(birtles) → review+
Comment on attachment 785365 [details] [diff] [review]
patch

I've got what might be better idea, I think. More details in a few minutes.
Attachment #785365 - Flags: review?(dholbert) → review-
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).
OS: Mac OS X → All
Hardware: x86_64 → All
Posted patch patch v2 (obsolete) — Splinter Review
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.
Attachment #786373 - Flags: review?
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...
Attachment #786373 - Attachment description: alternate patch → patch v2
Attachment #786373 - Flags: review?
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.
Attachment #786373 - Attachment is obsolete: true
Assignee

Comment 11

6 years ago
That seems to work for the testcase. I'll send it to try and see how we get on.
Thanks!
https://hg.mozilla.org/mozilla-central/rev/afb4118f4479
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.