615872 - "ASSERTION: Sample time should not precede current interval" with removed documentElement

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Sample time should not precede current interval: 'aContainerTime >= beginTime', file content/smil/nsSMILTimedElement.cpp, line 609

###!!! ASSERTION: Expecting non-negative active time: 'aActiveTime >= 0', file content/smil/nsSMILTimedElement.cpp, line 1763

Comment 1

[Jesse Ruderman]

Attachment: stack traces

Comment 2

[Brian Birtles (:birtles)]

Attachment: Part 1: Refactoring, remove unused mTimedDocumentRoot member, v1a

Just a little refactoring to remove unneeded mTimedDocumentRoot that was confusing this part of the code.

Comment 3

[Brian Birtles (:birtles)]

Attachment: Part 2: Remove local samples from nsSMILTimedElement::Begin/EndElementAt

Main patch. This requires some explanation.

Firstly, we were previously doing these local samples inside nsSMILTimedElement to try and ensure we kept up-to-date. This is fine most of the time, but when a backwards seek is required we expect to get told to do a Rewind first and in this case we weren't since we were bypassing the time container which normally detects this sort of thing.

Now it's possible to detect that case in the timed element and handle it there but basically we'd end up duplicating the functionality of the time container. Better still is just to remove this kind of logic from the timed element altogether.

That's what this patch does. A note on how it works: Generally whenever we change via the DOM we don't resample immediately. Instead we just set the resample needed flag in the time container. When we go to do anything that requires our model to be up-to-date (such as fetching an animated value) we flush the resample requests at that time.

For Begin/EndElement we can do likewise PROVIDED we take care to flush resample requests before we perform subsequent Begin/EndElement requests (or calls to SetCurrentTime etc.)

For SetCurrentTime we need to force the resample there and then in order to mark off the end of the seek interval correctly.

Two changes in the patch that might raise eyebrows:

a) changing the number of expected assertions for anim-use-href-01.svg

This is because now we're doing an extra FlushAnimations() at the start of SetCurrentTime().

If we really want the old behaviour then we could remove this extra FlushAnimations() and make Begin/EndElement force a resample (by adding a FlushAnimations() at the end of each of those methods) but I think it makes more sense to have SetCurrentTime and the like be responsible for ensuring the model is up to date before they do anything.

Note that having Begin/EndElement NOT force a resample shouldn't cause problems for TimeEvents since these events are dispatched asynchronously anyway and will simply be dispatched on the next sample (or next FlushAnimations).

I suppose one noticeable difference would be if we had:

 BeginElement();
 SomeotherDOMCallThatPutsAnEventOnTheQueue();
 var x = circle.cx.animVal.value;

In this case we won't dispatch the event until the call to get the animated value (when we'll flush resample requests) meaning it might appear to be out of order. Is that ok?

b) changes to the test_smilKeySplines.xhtml test. Just fixing the test case. The previous test case was wrong. I'm not exactly sure why it worked before, it probably shouldn't have. I can only guess that now we're actually updating correctly.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 496056 [details] [diff] [review]
Part 2: Remove local samples from nsSMILTimedElement::Begin/EndElementAt

>diff --git a/content/smil/nsSMILTimedElement.cpp b/content/smil/nsSMILTimedElement.cpp
>--- a/content/smil/nsSMILTimedElement.cpp
>+++ b/content/smil/nsSMILTimedElement.cpp
>@@ -259,64 +259,28 @@ nsSMILTimedElement::BeginElementAt(doubl
> {
>-  if (mElementState != STATE_STARTUP) {
>-    DoSampleAt(currentTime, PR_FALSE); // Regular sample, not end sample
>-  }

Per irc discussion, we should flush animations here & in EndElementAt so that we get an up-to-date repaint once this completes.

>+      AnimationNeedsResample();
>+      // While seeking we don't fire events, but as soon as seeking finishes we
>+      // should start firing events. However, the seek state is reset within
>+      // a sample so we want to force a sample now to ensure that from here
>+      // onwards events will be fired.
>+      FlushAnimations();

This comment confused me a little bit -- maybe simplify (and expand the scope of why we need the synchronous sample) to say something like:
   // Trigger synchronous sample now, to:
   //  - Make sure we get an up-to-date paint after this method
   //  - re-enable event firing (it got disabled during seeking, and it doesn't get
   //    re-enabled until the first sample after the seek -- so let's make that happen now.)

r=dholbert with that. :)  Sorry for the wait on this one.

Comment 5

[Brian Birtles (:birtles)]

Attachment: Part 2: Remove local samples from nsSMILTimedElement::Begin/EndElementAt; r=dholbert

(In reply to comment #4)
> Per irc discussion, we should flush animations here & in EndElementAt so that
> we get an up-to-date repaint once this completes.
Fixed.

> >+      AnimationNeedsResample();
> >+      // While seeking we don't fire events, but as soon as seeking finishes we
...
> This comment confused me a little bit -- maybe simplify (and expand the scope
> of why we need the synchronous sample) to say something like:
>    // Trigger synchronous sample now, to:
>    //  - Make sure we get an up-to-date paint after this method
>    //  - re-enable event firing (it got disabled during seeking, and it doesn't
> get
>    //    re-enabled until the first sample after the seek -- so let's make that
> happen now.)
Fixed.

> r=dholbert with that. :)  Sorry for the wait on this one.
(Not at all, the [awaiting review] thing is just so I can remember where I'm up to--I wasn't intending to hurry you along :)

Comment 6

[Brian Birtles (:birtles)]

Attachment: Roll-up patch for landing approval

Requesting approval to land. Fixes assertion. Removes and simplifies code. Includes test.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 497721 [details] [diff] [review]
Roll-up patch for landing approval

Looks like you forgot to include the test in the patch?

Comment 8

[Brian Birtles (:birtles)]

(In reply to comment #7)
> Looks like you forgot to include the test in the patch?

Oh, cheers, I didn't notice that. hg had a fit when I tried to qfold a bitrotted patch and left the repo in a mess. I'll be sure to push attachment 497719 [details] [diff] [review] which includes the test.

Comment 9

[Brian Birtles (:birtles)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/819a2310b235
http://hg.mozilla.org/mozilla-central/rev/7f2832603edb