"ASSERTION: Sample time should not precede current interval"

RESOLVED FIXED in Firefox 58

Status

()

--
critical
RESOLVED FIXED
6 years ago
11 months ago

People

(Reporter: jruderman, Assigned: birtles)

Tracking

(Blocks: 1 bug, {assertion, crash, testcase})

Trunk
mozilla58
x86_64
Mac OS X
assertion, crash, testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox56 wontfix, firefox57 wontfix, firefox58 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
Created attachment 723148 [details]
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
Still reproduces on trunk.
Keywords: crash
Has Regression Range: --- → irrelevant
status-firefox56: --- → wontfix
status-firefox57: --- → wontfix
status-firefox58: --- → fix-optional
status-firefox-esr52: --- → wontfix
Flags: in-testsuite?
(Assignee)

Comment 2

11 months ago
This seems worth investigating. Taking for now.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
(Assignee)

Comment 3

11 months ago
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 hidden (mozreview-request)

Comment 5

11 months ago
mozreview-review
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!
Attachment #8921325 - Flags: review?(dholbert) → review+

Comment 6

11 months ago
Pushed by bbirtles@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/680d798e440b
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox58: fix-optional → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.