596796 - "ABORT: In postactive state but the interval has been set" with <svg:animate>

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (causes fatal assertion when loaded)

###!!! ABORT: In postactive state but the interval has been set: '!mCurrentInterval', file content/smil/nsSMILTimedElement.cpp, line 1833

Comment 1

[Brian Birtles (:birtles)]

Attachment: Patch v1a

The problem here is that we have an invariant that so long as an animation's state is startup or postactive, the current interval should be null. However, sometimes we set the element state, then, before clearing the current interval we call unlink on it which triggers callbacks that (as in the test case) might result in us being called in this intermediate state.

This patch just moves the current interval in a temporary variable before calling Unlink.

I've also done an audit of everywhere else this situation could arise and given it the same treatment.

However, in doing so I've had to rework the way we handle BindToTree and this forced me to revisit some issues about what the behaviour should be there. Basically, what we were doing previously was wrong for a variety of reasons (e.g. the "local rewind" assumed that svg.setCurrentTime(svg.getCurrentTime()) had no side effects apart from putting the document in a seek state but that's incorrect when the document has yet to start. Furthermore there were other edge cases there such as events due between the current time and next sample getting suppressed.)

The correct behaviour isn't clear here so I've summarised my understanding and assumptions on www-smil here:

  http://lists.w3.org/Archives/Public/www-smil/2010OctDec/0004.html
  
This patch includes the simplified, corrected behaviour.

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 481727 [details] [diff] [review]
Patch v1a

Looks good! One nit - could you add a comment explicitly saying that the nsAutoPtr initialization will set |mCurrentInterval| to null? Maybe something like:
>+      // Transfer ownership to temp var. (This sets mCurrentInterval to null.)
>+      nsAutoPtr<nsSMILInterval> interval(mCurrentInterval);

Your header comment for this method basically already says that, but it wasn't clear to me (at first) *where* the null-setting actually took place (I was looking for and not finding a "mCurrentInterval = nsnull" line), until I remembered that feature of nsAutoPtr. :)

r=dholbert

Comment 3

[Brian Birtles (:birtles)]

Attachment: Patch v1b, r=dholbert

Add comment from review feedback. Thanks Daniel!

Requesting approval to land. This fixes previously incorrect and inconsistent behaviour in a number of places and fixes a fatal abort. Includes test.

Comment 4

[Brian Birtles (:birtles)]

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

Comment 5

[Brian Birtles (:birtles)]

For posterity, I reposted my query to www-svg as suggested by Jack Jansen on www-smil:

http://lists.w3.org/Archives/Public/www-svg/2010Oct/0048.html

The response I got confirms the behaviour as implemented in this patch:

http://lists.w3.org/Archives/Public/www-svg/2010Oct/0053.html

Comment 6

[Brian Birtles (:birtles)]

Seems there may be other views:

http://lists.w3.org/Archives/Public/www-svg/2010Oct/0082.html

Will file a separate bug if it turns out our behaviour needs to change here.