"ABORT: Update current interval recursion depth exceeded threshold"

RESOLVED FIXED in mozilla11



6 years ago
5 years ago


(Reporter: Jesse Ruderman, Assigned: birtles)


(Blocks: 1 bug, {assertion, testcase})

assertion, testcase
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)



(3 attachments)



6 years ago
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

6 years ago
Created attachment 569883 [details]
stack trace
Assignee: nobody → birtles
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.
Attachment #573755 - Flags: review?(dholbert)

Comment 3

6 years ago
Try run for 0a39029ce1eb is complete.
Detailed breakdown of the results available here:
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 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.

>+  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+
Thanks again Daniel!

Pushed with review feedback addressed:

I also took the liberty of changing EndTimesAreDependentOn to AreEndTimesDependentOn
OS: Mac OS X → All
Hardware: x86_64 → All
Whiteboard: [inbound]
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Flags: in-testsuite+
Whiteboard: [inbound]
Duplicate of this bug: 704847
You need to log in before you can comment on or make changes to this bug.