Closed
Bug 697640
Opened 13 years ago
Closed 13 years ago
"ABORT: Update current interval recursion depth exceeded threshold"
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: jruderman, Assigned: birtles)
References
Details
(Keywords: assertion, testcase)
Attachments
(3 files)
###!!! ABORT: Update current interval recursion depth exceeded threshold: 'false', file content/smil/nsSMILTimedElement.cpp, line 1953 Similar symptoms as bug 678847, which Brian Birtles fixed.
Reporter | ||
Comment 1•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → birtles
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•13 years ago
|
||
Proposed patch. When determining if an open-ended interval is ok, we used to ignore self-dependent end times by checking if all times were not definite but it turns out you can have potentially self-dependent end times that are definite. This patch updates the condition to properly ignore all potentially self-dependent end times. It also updates the comments to (hopefully) make this a little clear and align better with the SMIL pseudocode. I've also rearranged the tests to make things a fraction faster in the case where there is an end time.
Attachment #573755 -
Flags: review?(dholbert)
Comment 3•13 years ago
|
||
Try run for 0a39029ce1eb is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=0a39029ce1eb Results (out of 210 total builds): success: 190 warnings: 20 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bbirtles@mozilla.com-0a39029ce1eb
Comment 4•13 years ago
|
||
Comment on attachment 573755 [details] [diff] [review] Proposed patch v1a >@@ -1677,31 +1677,46 @@ nsSMILTimedElement::GetNextInterval(cons >+ openEndedIntervalOk = openEndedIntervalOk || >+ (aReplacedInterval && >+ EndTimesAreDependentOn(aReplacedInterval->End())); >+ >+ if (!openEndedIntervalOk) >+ return false; // Bad interval >+ } While you're touching this, could you add braces around "return false" here? One-liner "if" clauses that are followed by a differently-spaced "}" line are kind of confusing / hard to visually parse. >+bool >+nsSMILTimedElement::EndTimesAreDependentOn( >+ const nsSMILInstanceTime* aBase) const >+{ >+ for (PRUint32 i = 0; i < mEndInstances.Length(); ++i) { >+ if (mEndInstances[i]->GetBaseTime() != aBase) >+ return false; >+ } >+ return true; >+} Add braces around the "return false" there, too, for same reason as above. Also: Based on the function's name, I'd expect it to return false for an empty mEndInstances list -- but the way it's coded now, it'll return true for an empty list. In practice, it looks like this distinction doesn't actually end up affecting behavior, since you already check for IsEmpty() up one level before calling this function. But it's still probably worth fixing, to make the function more accurately do what its name suggests. So, I think we should add an initial "mEndInstances.IsEmpty()" check to this method, and return false if we're empty. (IsEmpty calls are cheap, thankfully.) r=dholbert with the above tweaks.
Attachment #573755 -
Flags: review?(dholbert) → review+
Assignee | ||
Comment 5•13 years ago
|
||
Thanks again Daniel! Pushed with review feedback addressed: https://hg.mozilla.org/integration/mozilla-inbound/rev/b1e2ee0bbdb5 I also took the liberty of changing EndTimesAreDependentOn to AreEndTimesDependentOn
OS: Mac OS X → All
Hardware: x86_64 → All
Whiteboard: [inbound]
Comment 6•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b1e2ee0bbdb5
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Assignee | ||
Updated•13 years ago
|
Flags: in-testsuite+
Whiteboard: [inbound]
You need to log in
before you can comment on or make changes to this bug.
Description
•