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

RESOLVED FIXED

Status

()

defect
--
critical
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: jruderman, Assigned: birtles)

Tracking

(Blocks 1 bug, {assertion, testcase})

Trunk
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

10 years ago
###!!! 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
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.
OS: Mac OS X → All
Here's a testcase that triggers the failure onclick instead of onload.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
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 on attachment 410713 [details] [diff] [review]
fix v1: add |nsSMILTimedElement.mNeedToSampleFillValue| flag

Brian, how does this look to you?
Attachment #410713 - Flags: review?(birtles)
Assignee

Comment 6

10 years ago
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.
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.
Assignee: dholbert → nobody
Status: ASSIGNED → NEW
Depends on: 474743
Attachment #410713 - Flags: review?(birtles)
(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.
Attachment #410713 - Attachment is obsolete: true
Attachment #410738 - Attachment description: tests → reftests & crashtest (as patch)
Assignee

Comment 9

10 years ago
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?
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 ("general@svg.bugs") to your watch-list.
Assignee

Comment 11

10 years ago
Assigning this to me. Once bug 474743 I'll confirm that it fixes this and check in the test cases from this patch.
Assignee: nobody → birtles
Status: NEW → ASSIGNED
Landed this bug's testcases, now that bug 474743 has landed:
http://hg.mozilla.org/mozilla-central/rev/f4e56d114546
Reporter

Updated

10 years ago
Flags: in-testsuite+
Resolving this as fixed by bug 474743.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.