Closed
Bug 596796
Opened 14 years ago
Closed 14 years ago
"ABORT: In postactive state but the interval has been set" with <svg:animate>
Categories
(Core :: SVG, defect)
Tracking
()
RESOLVED
FIXED
mozilla2.0b7
People
(Reporter: jruderman, Assigned: birtles)
References
Details
(Keywords: assertion, testcase)
Attachments
(2 files, 1 obsolete file)
239 bytes,
image/svg+xml
|
Details | |
8.49 KB,
patch
|
birtles
:
review+
roc
:
approval2.0+
|
Details | Diff | Splinter Review |
###!!! ABORT: In postactive state but the interval has been set: '!mCurrentInterval', file content/smil/nsSMILTimedElement.cpp, line 1833
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → birtles
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•14 years ago
|
||
The problem here is that we have an invariant that so long as an animation's state is startup or postactive, the current interval should be null. However, sometimes we set the element state, then, before clearing the current interval we call unlink on it which triggers callbacks that (as in the test case) might result in us being called in this intermediate state. This patch just moves the current interval in a temporary variable before calling Unlink. I've also done an audit of everywhere else this situation could arise and given it the same treatment. However, in doing so I've had to rework the way we handle BindToTree and this forced me to revisit some issues about what the behaviour should be there. Basically, what we were doing previously was wrong for a variety of reasons (e.g. the "local rewind" assumed that svg.setCurrentTime(svg.getCurrentTime()) had no side effects apart from putting the document in a seek state but that's incorrect when the document has yet to start. Furthermore there were other edge cases there such as events due between the current time and next sample getting suppressed.) The correct behaviour isn't clear here so I've summarised my understanding and assumptions on www-smil here: http://lists.w3.org/Archives/Public/www-smil/2010OctDec/0004.html This patch includes the simplified, corrected behaviour.
Attachment #481727 -
Flags: review?(dholbert)
Comment 2•14 years ago
|
||
Comment on attachment 481727 [details] [diff] [review] Patch v1a Looks good! One nit - could you add a comment explicitly saying that the nsAutoPtr initialization will set |mCurrentInterval| to null? Maybe something like: >+ // Transfer ownership to temp var. (This sets mCurrentInterval to null.) >+ nsAutoPtr<nsSMILInterval> interval(mCurrentInterval); Your header comment for this method basically already says that, but it wasn't clear to me (at first) *where* the null-setting actually took place (I was looking for and not finding a "mCurrentInterval = nsnull" line), until I remembered that feature of nsAutoPtr. :) r=dholbert
Attachment #481727 -
Flags: review?(dholbert) → review+
Assignee | ||
Comment 3•14 years ago
|
||
Add comment from review feedback. Thanks Daniel! Requesting approval to land. This fixes previously incorrect and inconsistent behaviour in a number of places and fixes a fatal abort. Includes test.
Attachment #481727 -
Attachment is obsolete: true
Attachment #482185 -
Flags: review+
Attachment #482185 -
Flags: approval2.0?
Attachment #482185 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Comment 4•14 years ago
|
||
Pushed: http://hg.mozilla.org/mozilla-central/rev/fa486a281aa8
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Assignee | ||
Comment 5•14 years ago
|
||
For posterity, I reposted my query to www-svg as suggested by Jack Jansen on www-smil: http://lists.w3.org/Archives/Public/www-svg/2010Oct/0048.html The response I got confirms the behaviour as implemented in this patch: http://lists.w3.org/Archives/Public/www-svg/2010Oct/0053.html
Assignee | ||
Comment 6•14 years ago
|
||
Seems there may be other views: http://lists.w3.org/Archives/Public/www-svg/2010Oct/0082.html Will file a separate bug if it turns out our behaviour needs to change here.
Updated•14 years ago
|
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in
before you can comment on or make changes to this bug.
Description
•