526536 - "ABORT: Sample time should not be negative" adding attribute to svg:animate

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (triggers fatal assertion in debug firefox)

###!!! ASSERTION: GetMillis() called for unresolved time.: 'mState == STATE_RESOLVED', file content/smil/nsSMILTimeValue.cpp, line 80

###!!! ABORT: Sample time should not be negative: 'mSampleTime >= 0.0f', file content/smil/nsSMILAnimationFunction.cpp, line 368

Comment 1

[Daniel Holbert [:dholbert]]

Confirmed on a mozilla-central debug build from yesterday.

Note that the assertion-failure is from a call one line above the failed NS_ABORT_IF_FALSE, in nsSMILAnimationFunction::InterpolateResult

365   const nsSMILTime& dur = mSimpleDuration.GetMillis();
366 
367   // Sanity Checks
368   NS_ABORT_IF_FALSE(mSampleTime >= 0.0f, "Sample time should not be negative");

So initially, we'll skip this animation, at the beginning of nsSMILAnimationFunction::ComposeResult, because "IsActiveOrFrozen()" returns false.  But when we set fill=freeze, that method starts returning true, and we abort on the next sample.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (triggers fatal assertions in debug build when clicked)

Here's a testcase that triggers the failure onclick instead of onload.

Comment 3

[Daniel Holbert [:dholbert]]

So the general problem here is this: we don't correctly handle setting fill="freeze" when our nsSMILTimedElement is in the "POSTACTIVE" state.

This includes the scenario in both testcases posted so far -- when the interval is entirely before document start-time (and is therefore ignored[1]) -- but it also includes simpler cases.

Normally, when we run an animation that's got fill="freeze", nsSMILTimedElement::SampleAt() calls SampleFillValue() on our first "frozen" sample.  This calls nsSMILAnimationFunction::SampleLastValue, which sets the |mLastValue| variable, which gets used for all subsequent samples.  This is what we want.

However, if we aren't in "freeze" mode at the end of the animation's interval (i.e. if we set fill="freeze" later on), then SampleFillValue & SampleLastValue never get called, and so |mLastValue| never gets set.  And so nsSMILAnimationFunction keeps trying to do normal interpolation, when we don't want it to.

I propose we fix this by moving the SampleFillValue call from STATE_ACTIVE to STATE_POSTACTIVE, and we add a flag to keep track of whether we need to call it.  Patch to do this coming up...

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix v1: add |nsSMILTimedElement.mNeedToSampleFillValue| flag

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 410713 [details] [diff] [review]
fix v1: add |nsSMILTimedElement.mNeedToSampleFillValue| flag

Brian, how does this look to you?

Comment 6

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

Looks good.

I'm just wondering how urgent this is though because it should already be fixed in the patch for syncbase timing (bug 474743) as I came across this bug in implementing that.

The patch for syncbase timing is a major overhaul of the timing model (and fixes several other bugs I've discovered) so I'm just wondering if it's best to land this or just wait? Of course the test cases should be added one way or another.

(I've significantly updated the handling of the previous interval since the latest wip patch attached to bug 474743, but that latest wip patch should still fix this issue.)

I'm expecting to put the syncbase timing patch up for review in about 3 weeks time. I can publish another wip patch in about 2 weeks time.

Comment 7

[Daniel Holbert [:dholbert]]

Ok -- since this only causes problems in debug builds, and since bug 474743 will be reworking this code anyway, I'm happy to just have this be fixed by in that patch when it's ready.
 --> Marking dependency.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: reftests & crashtest (as patch)

(In reply to comment #6)
> Of course the test cases should be added one way or
> another.

Here are the test cases from "fix v1", for landing once this has been fixed by bug 474743's patch.

Comment 9

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

Thanks Daniel. Sorry, that must be pretty frustrating after all your work on this. I would have let you know sooner but I wasn't CC'ed on the bug--not sure if there's some way I can automatically be CC'ed for new SMIL bugs?

Comment 10

[Daniel Holbert [:dholbert]]

No prob -- I probably should've CC'd you once I realized it was time-model stuff.  Anyway, it was good to refamiliarize myself with time model a bit. :)

> if there's some way I can automatically be CC'ed for new SMIL bugs?

You can receive email for all SVG bugs if you go to Bugzilla's "Preferences" | "Email Preferences" section and add the SVG QA Contact ("[e-mail]") to your watch-list.

Comment 11

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

Assigning this to me. Once bug 474743 I'll confirm that it fixes this and check in the test cases from this patch.

Comment 12

[Daniel Holbert [:dholbert]]

Landed this bug's testcases, now that bug 474743 has landed:
http://hg.mozilla.org/mozilla-central/rev/f4e56d114546

Comment 13

[Daniel Holbert [:dholbert]]

Resolving this as fixed by bug 474743.