1010681 - "ABORT: Rewind in the middle of a forwards seek?" with <svg:animate>

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ABORT: Rewind in the middle of a forwards seek?: 'mSeekState == SEEK_BACKWARD_FROM_INACTIVE || mSeekState == SEEK_BACKWARD_FROM_ACTIVE', file dom/smil/nsSMILTimedElement.cpp, line 798

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Daniel Holbert [:dholbert]]

I can repro on a 64-bit linux debug build. OS --> All.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

This appears to be a regression from bug 619469. From a little digging around I *think* what is happening is the following:

1. We create a new animation
(The animation defined with markup is not actually needed to reproduce this)
2. Calling animate.targetElement flushes animations and puts us in the active state
3. Calling animate.requiredFeatures.insertItemBefore(0, 0) sets a requiredFeatures to "0"
4. document.documentElement.setCurrentTime(4) initiates a forwards seek
5. As part of the forwards-seek we run milestone samples
6. The next milestone is at t=2s since that's the end of the animation's interval
7. We run that milestone 'end' sample at t=2s on the animation without testing requiredFeatures on the animation
8. That sample sets the animations mSeekState to SEEK_FORWARD_FROM_ACTIVE
9. We go to run regular samples at t=4s but at that point we test requiredFeatures (this was the part added in bug 619469) and decide to skip the animation since it doesn't support to "0" feature. Hence the animation is left hanging in the "SEEK_FORWARD_FROM_ACTIVE" state since we never end up calling DoPostSeek. Thus we end up in a state we should never arrive in.
10. document.documentElement.setCurrentTime(0) triggers a backwards seek at which point we assert that we are not in the middle of a forwards seek... except we are.

There are probably quite a few ways we could solve this.

For example, we *could* just run the passes-conditional-processing-tests step when doing milestone samples as well but I think that would actually be wrong. I *think* that would mean if we started failing a conditional-processing test mid-animation we'd basically end up freezing it at that point. I *think* the correct behaviour should actually be to remove that animation.

I think a better approach would be to check in nsSMILTimedElement::SampleAt and SampleEndAt. Something like:

  If it fails the test
    if (mElementState != STATE_STARTUP) {
      // Clear intervals state etc. in pretty much the same way as
      // nsSMILTimedElement::Rewind but without setting mSeekState
      // i.e. mElementState -> STATE_STARTUP
      //      mClient->Inactive(false); (if mClient is set)
      //      mCurrentInterval -> nullptr
      //      ClearIntervals()
      //      etc. etc.
    }
  Else
    DoSampleAt

Better still would be to actually check all this in the SVG side and set a 'disabled' flag on the nsSMILTimedElement.

Better still would be to make SVGTests cache the results of PassesConditionalProcessingTests or at least when the result won't change between samples (e.g. only testing on requiredFeatures etc.). I might be missing something but it seems like we're evaluating this on every sample and it's quite involved.

Daniel, Robert, any preferences for how we approach this?

Comment 4

[Daniel Holbert [:dholbert]]

Thanks for the detailed investigation & explanation, Brian!

So, I was initially going to say...
"Seems like a quick paperover hack might be to move the SampleTimedElement call outside of the PassesConditionalProcessingTests() check (in nsSMILAnimationController::SampleAnimation()), to keep the timing model up to date but still not visibly animate"
...but then I suppose we would fire animation events, which we shouldn't do.  I don't think we have tests for that (events not firing on disabled animations), but we should probably add some. If you don't end up adding such tests as part of this bug, could you perhaps file a followup on adding some?

> I *think* [...] if we started failing a conditional-processing test mid-animation
> [...] the correct behaviour should actually be to remove that animation.

Agreed.

> I think a better approach would be to check in nsSMILTimedElement::SampleAt and SampleEndAt.

Seems reasonable. So this prevents the state-change in your step 8 (and the associated stuff before it), right?

> Better still would be to actually check all this in the SVG side and set a 'disabled' flag

Agreed, though that would only be valid for the duration of a sample, I think (unless we could guarantee that the flag is kept up-to-date)

> Better still would be to make SVGTests cache the results of

Similarly, we'd have to be sure this cached result is invalidated when things change, and I'm not clear enough on how these tests work to know how tricky that would be.

So I'd probably lean towards your SampleAt() solution, personally, but the others sound good too if you can get them to work. :)

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Daniel Holbert [:dholbert] from comment #4)

Thanks for your comments here. I was just about to go ahead and implement the SampleAt() solution when I realised something.

> > Better still would be to actually check all this in the SVG side and set a 'disabled' flag
> 
> Agreed, though that would only be valid for the duration of a sample, I
> think (unless we could guarantee that the flag is kept up-to-date)

I realised that a conditional-processing test is only going to stop passing or stop failing if either of the following change:

a) The thing it is testing changes, or
b) The test itself changes

For (a) the things tested are requiredFeatures, requiredExtensions, and systemLanguage. As far as I can tell, these will only change if the user goes to the prefs and changes their language settings or goes to about:config and enables/disables some feature. In either case I think it's pretty rare and probably acceptable if you had to reload content in order for the change to take effect.

For (b) we can tell when that happens because we'll get a call to setAttribute etc.

So I'm wondering if it would be possible to do the conditional processing test in setAttribute (actually SVGAnimation::ParseAttribute) and then call SetDisabled or something of the sort on the TimedElement (I'd actually like to null-out the mTimedElement pointer but that seems like more work since if the test starts passing again you'd have to go and rebuild the mTimedElement and wire it up correctly).

That would lead to some slightly odd situations. For example, you load the page with animations that are set to run only if English is one of your languages (maybe there are a series of flags representing languages spoken and you spin all the ones that match). While the animation is playing, the user updates their language prefs and removes English. The animation continues playing. However, if an identical animation is added to the document it will not play.

That seems suboptimal but tolerable and better than reevaluating the conditional processing tests on every sample. If we later decide we need to support dynamic changes to the language etc. we could probably hook up a kind of observer behaviour there to watch changes to language preferences.

I notice that for regular SVG content we don't dynamically update when the user changes their language preferences:

  http://jsfiddle.net/447Vt/1/

What do you think?

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Brian Birtles (:birtles) from comment #5)
> While the animation is playing, the
> user updates their language prefs and removes English. The animation
> continues playing. However, if an identical animation is added to the
> document it will not play.
> 
> That seems suboptimal but tolerable

Agreed.

> and better than reevaluating the
> conditional processing tests on every sample.

Agreed, perf/sanity-wise (albeit a bit confusing correctness-wise).

> I notice that for regular SVG content we don't dynamically update when the
> user changes their language preferences:

Ah, that's good to know.  We should be consistent with our behavior for conditional processing on other SVG content here.  If that means just catching this at parse-time (and not re-checking afterwards), that seems good to me.

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1 - Add nsSMILTimedElement::SetIsDisabled for turning off a timed element

This method is added so that animation elements that fail conditional processing
tests can disable the corresponding timed element. Currently, this handling is
performed by the animation controller which simply skips sampling animations
that fail tests. However, this doesn't provide the correct behavior since
"skipped" animations can still have intervals that trigger syncbase dependencies
etc. Also, we still do milestone samples for "skipped" animations which can
lead to cases where we are in an inconsistent state.

This patch provides a method that will be used in subsequent patches to more
correctly remove the effects of a timed element.

To do this, this patch also factors out some common methods for clearing timing
state that can be shared with the Rewind method.

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 2 - Make SVG animation elements call SetIsDisabled on the corresponding timed element

This patch makes use of the SetIsDisabled method added to nsSMILTimedElement in
the previous patch to "turn off" the associated timed element of an animation
element that has failing conditional processing tests.

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 3 - Add mochitest for dynamic changes to conditional processing tests on animation elements

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 4 - Add crashtest for changes to conditional processing tests on animation elements

Before the changes in this bug, this test used to produce an assertion:

  ABORT: Rewind in the middle of a forwards seek?

This was because for a timed element whose corresponding animation element fails
conditional processing tests, we were still performing milestone samples even
though we were skipping regular samples.

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 5 - Remove conditional processing inside animation controller

Remove the changes to nsSMILAnimationController introduced by bug 619469 now
that we have introduced a more thorough approach to disabling animations that
fail conditional processing tests.

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 6 - Remove PassesConditionProcessingTests from SVGAnimationElement

This method was originally exposed so that the animation compositor could test
if an animation should be skipped when sampling but now that we perform this
testing within the SVGAnimationElement itself it is no longer needed.

Comment 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Try run looks ok: https://tbpl.mozilla.org/?tree=Try&rev=f35888df92ec

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Hi Robert, are you interested in reviewing this? I've been overloading Daniel with review requests lately so if you are interested in taking this one that would be most appreciated!

Comment 15

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Thanks Robert!

Comment 16

[Brian Birtles (:birtles, Feb 26~ parental leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e17a019438d
https://hg.mozilla.org/integration/mozilla-inbound/rev/826bd3aacbf8
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbbdf5acea89
https://hg.mozilla.org/integration/mozilla-inbound/rev/3731487fd697
https://hg.mozilla.org/integration/mozilla-inbound/rev/7351770b2d46
https://hg.mozilla.org/integration/mozilla-inbound/rev/d54744a7661c

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6e17a019438d
https://hg.mozilla.org/mozilla-central/rev/826bd3aacbf8
https://hg.mozilla.org/mozilla-central/rev/bbbdf5acea89
https://hg.mozilla.org/mozilla-central/rev/3731487fd697
https://hg.mozilla.org/mozilla-central/rev/7351770b2d46
https://hg.mozilla.org/mozilla-central/rev/d54744a7661c

Comment 18

[Paul Silaghi, QA [:pauly]]

Verified fixed Nightly 32.0a1 2014-05-30-mozilla-central-debug