697640 - "ABORT: Update current interval recursion depth exceeded threshold"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: 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]

Attachment: stack trace

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: 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]

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/[e-mail]-0a39029ce1eb

Comment 4

[Daniel Holbert [:dholbert]]

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, Feb 26~ parental leave)]

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]]

https://hg.mozilla.org/mozilla-central/rev/b1e2ee0bbdb5