611927 - "ASSERTION: Trying to add incompatible types" with svg:animate

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

ASSERTION: Trying to add incompatible types: 'Error', file content/smil/nsSMILValue.cpp

Comment 1

[Daniel Holbert [:dholbert]]

Perhaps a regression from bug 603917?  (if we end up trying to add a "real" nsSMILValue to a null-typed "base value" nsSMILValue)

Comment 2

[Brian Birtles (:birtles)]

(In reply to comment #1)
> Perhaps a regression from bug 603917?  (if we end up trying to add a "real"
> nsSMILValue to a null-typed "base value" nsSMILValue)

Yep, definitely the case. I have a patch I'll post in a moment.

Comment 3

[Brian Birtles (:birtles)]

Attachment: Patch v1a

As mentioned by Daniel in comment 1, this is a regression from bug 603917. We skip fetching the base value when it *looks* like we won't need it -- i.e. there is at least one animation function in sandwich that overwrites the base value.

This is an important optimisation since fetching the base value can be expensive for certain properties (e.g. 'display' where we have to turn the property on and off to get it which can trigger recreating frames etc.) It also gets us bug 604557 for free.

The cost is that if we skip getting the base value and the function that was going to overwrite the base value unexpectedly fails then we have no base value to build on.

SVG basically says the whole animation should fail in this case but this is really strict and most other browsers aren't implementing it this strictly. It also seems to be reversed in in SVGT1.2. e.g. see http://lists.w3.org/Archives/Public/www-svg/2010Sep/0114.html

I think we can either:
a) Do something complicated and fetch the base value if it turns out we need it after all.
b) Fail outright -- e.g. if we go to sandwich add and the underlying value is null, just do nothing
c) Just set the value to add as the new value.

I've implemented (b) for now since that seems to make more sense to me than (c) where we use an additive value in a non-additive way.

Comment 4

[Daniel Holbert [:dholbert]]

Agreed on the ranking of those options -- I'd prefer to avoid (c), since it's kind of lying.  And (a) does seem like a lot of complicated work for not a lot of gain.

So given that we're doing the optimization from bug 603917, (b) seems reasonable here (particularly given that _an_ SVG spec says this is what we should do).

However, I think the current patch might perform this check later than we'd like -- as it stands, we end up doing all the work of interpolating the value-to-be-added, only to find out that we're not going to be able to do the addition after all.

Perhaps it would make more sense to insert this check at the beginning of nsSMILAnimationFunction::ComposeResult, with the other early-return clauses there?

Comment 5

[Brian Birtles (:birtles)]

Attachment: Patch v1b

(In reply to comment #4)
> However, I think the current patch might perform this check later than we'd
> like -- as it stands, we end up doing all the work of interpolating the
> value-to-be-added, only to find out that we're not going to be able to do the
> addition after all.
> 
> Perhaps it would make more sense to insert this check at the beginning of
> nsSMILAnimationFunction::ComposeResult, with the other early-return clauses
> there?

Good idea. Fixed in this version of the patch.

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 491733 [details] [diff] [review]
Patch v1b

Looks great! One nit:

>+  if (isAdditive && aResult.IsNull())
>+    return; // aResult will be null-typed

We already have "aResult.IsNull()" on the first line there, so "aResult will be null-typed" on the second line doesn't really add much :)

I think what you're meaning to say is something more like
   // Null-typed aResult will tell caller that we failed.

Anyway, r=dholbert with that comment tweaked to be more helpful. :) (or just removed if there's nothing to say)

Comment 7

[Brian Birtles (:birtles)]

Pushed: http://hg.mozilla.org/mozilla-central/rev/bc688a29feed