849593 - "ASSERTION: Sample time should not precede current interval"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (asserts fatally when loaded)

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

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

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Still reproduces on trunk.

Comment 2

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

This seems worth investigating. Taking for now.

Comment 3

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

I haven't quite got to the bottom of this but I think what's happening is something like:

- The <set> element is attached to <svg> element 'a' which becomes its time
  container. Let's just call the <svg> element with ID 'a', "time container
  A".

- The <set> element registers its startup milestone with mTime = 0. All
  elements do this since that's where they resolve their initial interval.

- That milestone persists in time container A.

- Time container A is detached from the document tree.

- The <set> element is attached to the <svg> element 'b', aka time container
  B. Presumably at this point it also registers its startup milestone with
  time container B since it hasn't been sampled yet.

- The document is flushed which I guess means that time container B is sampled
  and the <set> element gets its initial interval resolved.

- Time container A is re-attached to the document tree at which point
  I presume it updates its parent offset (in SVGSVGElement::BindToTree which
  calls nsSMILTimeContainer::SetParent). Now, time container A's current time
  is 61ms (presumably that was the amount of time between firing onLoad and
  running the setTimeout callback in my debug build) so mParentOffset ends up
  being set to -61. Maybe it was that way before this step, I'm not sure.

- Then we attach time container A as a descendant of time container B.

- When we go to run the next sample we iterate through all time containers
  associated with the animation controller which includes both time containers
  A and B.

- Time container A renders up its "zero" milestone from the <set> element
  since it has yet to run it. It converts it to container time which is -61.

- That happens to be the earliest milestone so we go to sample it.

- We iterate through the elements, get to the <set>, then convert the time to
  <set>'s container time. That is, we take the -61 and convert it to a time in
  the time container B's time scale.

  Now, we set time container B's current time to 1000 earlier on so its
  mParentOffset is -1000. As a result, the time gets converted to 939.

- We pass 939 to <set> as the sample time (it's an "end" sample so we call
  SampleEndAt) and then it complains because it has a resolved interval
  starting at 1000 and it doesn't expect to get a sample time before its
  current interval begins.

There might be a few details amiss here but I think that's roughly what's happening. There are probably a few points a long the way where we could fix this but perhaps the simplest is just to make timed elements ignore these bogus samples.

Comment 4

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

Attachment: Bug 849593 - Skip samples of active SMIL timed elements when the sample time precedes the current interval;

In some circumstances it is possible to sample a timed element in the active
state with a time that precedes is current interval.

One possible sequence of steps leading to this situation is as follows:

1. A timed element (e.g. <set>, <animate>) with a non-zero begin time is the
   child of <svg> element A (its "time container") but has yet to be sampled.

2. In order to resolve its initial interval, the timed element registers a
   startup milestone with its time container at time 0.

3. However, before the sample is performed where the timed element's initial
   current interval is resolved, <svg> element A is detached from the document
   tree.

4. The timed element is then attached to a different <svg> element B that has
   a current time greater than the begin time of the timed element and less than
   that of <svg> element A.

5. Since the timed element is still in its startup state it registers its
   startup milestone again, this time with its new time container, i.e. <svg>
   element B.

6. A tick occurs or the document has its style flushed such that a sample is
   performed.

   This includes running the milestone sample which causes the timed element to
   resolve its initial current interval. Furthermore the subsequent regular
   sample of the timed element causes it to transition into its active state
   because the current time of <svg> element B is greater than the begin time of
   the timed element.

7. <svg> element A is re-attached to the document.

8. When we go to run the next sample, we iterate through all time containers
   associated with the document's animation controller which includes both <svg>
   element A, and <svg> element B.

9. <svg> element A renders up its 0 milestone from step (2) since it has yet to
   run it. It converts this to parent time, i.e. the time space of the animation
   controller, which will be zero or less depending on the current time of <svg>
   element A when it was re-attached.

10. Since the milestone from <svg> element A will be the earliest milestone
    time, it will be used as the next milestone sample time.

11. The timed element is then sampled using this time, but first it is converted
    to a time in the time space of the timed element's time container, which is
    now <svg> element B.

    As a result of this conversion, the sample time may end up being *before*
    the beginning of the timed element's current interval. Since timed elements
    never expect the time to go backwards an assertion fails when it detects
    that it is active, but is being sampled before its current interval.

For this particular case, ignoring the "early" sample seems to be the most
appropriate action.

More generally, however, we can anticipate other cases similar to this where
milestones are registered that cause the sample time to temporarily go
backwards. A quick audit of nsSMILTimedElement::DoSampleAt suggests that, with
the code changes from this patch, that is probably ok.

As an alternative we could, perhaps, try to drop and re-create all milestones
when time containers are re-attached to the document tree but that would add
more complexity and would not necessarily cover other similar cases of this
situation.

I have verified that the crashtest included in this changeset fails without the
code changes also in this changeset.

Review commit: https://reviewboard.mozilla.org/r/192336/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/192336/

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8921325 [details]
Bug 849593 - Skip samples of active SMIL timed elements when the sample time precedes the current interval;

https://reviewboard.mozilla.org/r/192336/#review197852

r=me -- this seems fine. Thanks for the detailed explanation!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/680d798e440b
Skip samples of active SMIL timed elements when the sample time precedes the current interval; r=dholbert

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/680d798e440b