588287 - "ASSERTION: Sample time should not precede current interval" with animation

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 603

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

My fuzzer has been hitting this assertion for months, but this is the first time it has managed to create a reduced testcase for it.

Comment 1

[Brian Birtles (:birtles)]

I'll take this. My guess is that something's not getting cleaned up when we demote the newly created <svg> element from an outer <svg> to an inner <svg>.

It would be worth testing a variation on this test case too where the already active animation is re-parented to a more recently created outer svg container.

Comment 2

[Brian Birtles (:birtles)]

Attachment: Proposed patch v1a

Ok, here's a stab at a fix for this.

Note that in the test case, the created <g> element is not in the SVG namespace and so the nested <svg> element (which is in the SVG namespace) serves as an outermost <svg> element (since its parent is not an SVG element is doesn't look any further up).

The problem then is that the animation is re-parented to a younger time container and our assertion fails because time seems to go backwards without a backwards seek.

So this patch detects when we're re-parented to a younger time container (or at least a time container whose current time precedes the beginning of our current interval) and performs a local rewind on the element in that case.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 477016 [details] [diff] [review]
Proposed patch v1a

The crashtests need "reftest-wait" to make sure the setTimeout fires before we've dismissed the test.

Otherwise looks good!  Sorry for the delay on this.

Comment 4

[Brian Birtles (:birtles)]

Attachment: Patch v1b, r=dholbert

Add reftest-wait to test cases

Comment 5

[Brian Birtles (:birtles)]

Attachment: Patch v1c, r=dholbert

Oops, the removing of the reftest-wait was on the wrong <svg> element. Fixed, I hope.

Comment 6

[Brian Birtles (:birtles)]

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