Last Comment Bug 697640 - "ABORT: Update current interval recursion depth exceeded threshold"
: "ABORT: Update current interval recursion depth exceeded threshold"
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla11
Assigned To: Brian Birtles (:birtles)
:
:
Mentors:
: 704847 (view as bug list)
Depends on:
Blocks: 369230
  Show dependency treegraph
 
Reported: 2011-10-26 19:01 PDT by Jesse Ruderman
Modified: 2012-02-01 13:58 PST (History)
3 users (show)
bbirtles: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (asserts fatally when loaded) (86 bytes, image/svg+xml)
2011-10-26 19:01 PDT, Jesse Ruderman
no flags Details
stack trace (8.54 KB, text/plain)
2011-10-26 19:01 PDT, Jesse Ruderman
no flags Details
Proposed patch v1a (6.83 KB, patch)
2011-11-10 22:53 PST, Brian Birtles (:birtles)
dholbert: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-10-26 19:01:01 PDT
Created attachment 569882 [details]
testcase (asserts fatally when loaded)

###!!! 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.
Comment 1 Jesse Ruderman 2011-10-26 19:01:30 PDT
Created attachment 569883 [details]
stack trace
Comment 2 Brian Birtles (:birtles) 2011-11-10 22:53:16 PST
Created attachment 573755 [details] [diff] [review]
Proposed patch v1a

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.
Comment 3 Mozilla RelEng Bot 2011-11-11 04:00:38 PST
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 Daniel Holbert [:dholbert] 2011-11-13 13:07:29 PST
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.
Comment 5 Brian Birtles (:birtles) 2011-11-13 20:04:42 PST
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
Comment 6 Ed Morley [:emorley] 2011-11-14 19:37:46 PST
https://hg.mozilla.org/mozilla-central/rev/b1e2ee0bbdb5
Comment 7 Brian Birtles (:birtles) 2011-11-23 18:37:33 PST
*** Bug 704847 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.