669225 - Another crash with two <svg:animate> elements with the same ID

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Debug:
###!!! ABORT: Update current interval recursion depth exceeded threshold: 'PR_FALSE', file content/smil/nsSMILTimedElement.cpp, line 1920

(This abort is intended to catch too-much-recursion before it crashes. It was added in rev df0884806f2f, bug 665334 part 2.)

Opt:
Too-much-recursion crash bp-3abda76f-c000-4539-bc64-96c852110704

Comment 1

[Jesse Ruderman]

Thanks for adding the abort, btw.  Made it much easier for my fuzzer to catch the bug.

Comment 2

[Jesse Ruderman]

The too-much-recursion crash must be somewhere else, since the abort is associated with an early return in opt.

Comment 3

[Brian Birtles (:birtles)]

Attachment: Part 1 - Crash tests

Comment 4

[Brian Birtles (:birtles)]

Attachment: Part 2 - Preserve instance times that belong to intervals

Comment 5

[Brian Birtles (:birtles)]

Attachment: Part 3 - Add assertion to catch potentially problematic instance time deletion

Comment 6

[Brian Birtles (:birtles)]

Attachment: Part 4 - Make sure filtering of instance times doesn't remove the previous interval's end time

Comment 7

[Brian Birtles (:birtles)]

Attachment: Part 5 - Make sure other removal functors don't remove instance times that should be preserved

Comment 8

[Brian Birtles (:birtles)]

Attachment: Part 6 - Break dependencies sooner on unlink

Comment 9

[Brian Birtles (:birtles)]

Brief summary of the problem:

This is ultimately the same class of problem as bug 665334.

Basically we get infinite recursion when the following three conditions exist:
 a) A zero-length interval
 b) A self-resolving end="a.begin" cond.
 c) No end instance times

Basically, we get stuck trying to satisfy SMIL's condition that we don't have two zero-length intervals in a row and also SMIL's differing behaviour when we do or don't have any end instance times.

The problem is basically (c). We shouldn't have at least one end instance times if we have a zero-length interval. But we can end up deleting the end instance times if we do tree surgery.

These patches basically just ensure we don't delete such end instance times.

Patch 1 includes the original crash test plus an additional crash test I generated based on the analysis above

Patch 2 fixes the original crash

Patch 3 introduces an assertion to try to catch this class of problem in advance. It doesn't detect the original problem (since it doesn't go via RemoveInstanceTimes) but it does detect the problem in the additional test case and in the test case for bug 665334 (if you back out the fix for that bug).

Patch 4 ensures filtering behaviour doesn't trigger the assertion in patch 3.

Patch 5 fixes the additional crash test

Patch 6 is not strictly necessary and I'm a little hesistant about going with it. It basically just breaks off some of the dependencies when a timed element is unlink (e.g. when it is removed from the DOM tree). This came about when I was debugging this--I noticed a lot of these dependencies don't seem to get broken until the element gets re-bound. It doesn't seem right to have dependencies between elements in the tree and elements no longer bound to the tree. So I made us reset that timing information a little sooner.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 546078 [details] [diff] [review]
Part 1 - Crash tests

Review of attachment 546078 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 546079 [details] [diff] [review]
Part 2 - Preserve instance times that belong to intervals

Review of attachment 546079 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/smil/nsSMILTimedElement.cpp
@@ +433,5 @@
>  #ifdef DEBUG
>    PRBool found =
>  #endif
>      instanceList.RemoveElementSorted(aInstanceTime, InstanceTimeComparator());
>    NS_ABORT_IF_FALSE(found, "Couldn't find instance time to delete");

Beautification nit: while you're touching this chunk, could you fix up the "#ifdef DEBUG" in the contextual code to use DebugOnly?
Like so:
  mozilla::DebugOnly<PRBool> found = instanceList.RemoveElementSorted(...);
(need to #include "mozilla/Util.h" for that)

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 546082 [details] [diff] [review]
Part 3 - Add assertion to catch potentially problematic instance time deletion

Review of attachment 546082 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 546083 [details] [diff] [review]
Part 4 - Make sure filtering of instance times doesn't remove the previous interval's end time

Review of attachment 546083 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/smil/nsSMILTimedElement.cpp
@@ +1557,5 @@
>      }
>  
>    private:
>      PRUint32 mThreshold;
> +    nsTArray<const nsSMILInstanceTime *> mTimesToKeep;

For efficiency, I think we should make mTimesToKeep a const nsTArray& (the exact same type as |aTimesToKeep| in the constructor), rather than an actual nsTArray.

Of course, this could hypothetically cause issues if the nsTArray died before a RemoveBelowThreshold object with a reference to it.  But given that RemoveBelowThreshold is stack-only and receives a stack-allocated nsTArray, that's impossible.

Speaking of which -- all these functor classes like RemoveBelowThreshold should probably be flagged as NS_STACK_CLASS - could you add a followup patch for that? (or file a followup bug)

r=me with that.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 546084 [details] [diff] [review]
Part 5 - Make sure other removal functors don't remove instance times that should be preserved

Review of attachment 546084 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 546085 [details] [diff] [review]
Part 6 - Break dependencies sooner on unlink

> nsSMILTimedElement::Unlink()
> {
[...]
>+
>+  // Clear current interval
>+  mElementState = STATE_POSTACTIVE;
>+  ResetCurrentInterval();
>+
>+  // Remove old intervals
>+  for (PRInt32 i = mOldIntervals.Length() - 1; i >= 0; --i) {
>+    mOldIntervals[i]->Unlink();
>+  }
>+  mOldIntervals.Clear();

The above lines are basically identical to a chunk in ~nsSMILTimedElement().  Would it make sense to split this out into a helper function, or otherwise share code somehow?

Comment 16

[Brian Birtles (:birtles)]

Attachment: Part 2 - Preserve instance times that belong to intervals, r=dholbert

Address review feedback

Comment 17

[Brian Birtles (:birtles)]

Attachment: Part 4 - Make sure filtering of instance times doesn't remove the previous interval's end time, r=dholbert

Address review feedback

Comment 18

[Brian Birtles (:birtles)]

Attachment: Part 6 - Break dependencies sooner on unlink

Address review feedback, factored out common interval cleanup code.

Comment 19

[Brian Birtles (:birtles)]

Attachment: Part 7 - Mark stack classes as such

Follow-up patch to mark stack classes as NS_STACK_CLASS

Comment 20

[Brian Birtles (:birtles)]

Pushed to m-i:
http://hg.mozilla.org/integration/mozilla-inbound/rev/db9bf80216f8
http://hg.mozilla.org/integration/mozilla-inbound/rev/679e1ac7d846
http://hg.mozilla.org/integration/mozilla-inbound/rev/244bc2ab6075
http://hg.mozilla.org/integration/mozilla-inbound/rev/41bf0a2805ab
http://hg.mozilla.org/integration/mozilla-inbound/rev/18fe08f69978
http://hg.mozilla.org/integration/mozilla-inbound/rev/6dffbe5e1b5f
http://hg.mozilla.org/integration/mozilla-inbound/rev/8d53e0e13b6c

Comment 21

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/db9bf80216f8
http://hg.mozilla.org/mozilla-central/rev/679e1ac7d846
http://hg.mozilla.org/mozilla-central/rev/244bc2ab6075
http://hg.mozilla.org/mozilla-central/rev/41bf0a2805ab
http://hg.mozilla.org/mozilla-central/rev/18fe08f69978
http://hg.mozilla.org/mozilla-central/rev/6dffbe5e1b5f
http://hg.mozilla.org/mozilla-central/rev/8d53e0e13b6c