Closed Bug 669225 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: jruderman, Assigned: birtles)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, crash, testcase, Whiteboard: [inbound])

Crash Data

Attachments

(8 files, 3 obsolete files)

333 bytes, image/svg+xml
Details
1.61 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
1.56 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
1.60 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
1.77 KB, patch
Details | Diff | Splinter Review
3.01 KB, patch
Details | Diff | Splinter Review
4.72 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
3.96 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
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
Thanks for adding the abort, btw.  Made it much easier for my fuzzer to catch the bug.
The too-much-recursion crash must be somewhere else, since the abort is associated with an early return in opt.
Assignee: nobody → birtles
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86_64 → All
Attachment #546078 - Flags: review?(dholbert)
Attachment #546079 - Flags: review?(dholbert)
Attachment #546085 - Flags: review?(dholbert)
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 on attachment 546078 [details] [diff] [review]
Part 1 - Crash tests

Review of attachment 546078 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #546078 - Flags: review?(dholbert) → review+
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)
Attachment #546079 - Flags: review?(dholbert) → review+
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]:
-----------------------------------------------------------------
Attachment #546082 - Flags: review?(dholbert) → review+
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.
Attachment #546083 - Flags: review?(dholbert) → review+
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]:
-----------------------------------------------------------------
Attachment #546084 - Flags: review?(dholbert) → review+
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?
Address review feedback
Attachment #546079 - Attachment is obsolete: true
Address review feedback, factored out common interval cleanup code.
Attachment #546085 - Attachment is obsolete: true
Attachment #548101 - Flags: review?(dholbert)
Attachment #546085 - Flags: review?(dholbert)
Follow-up patch to mark stack classes as NS_STACK_CLASS
Attachment #548102 - Flags: review?(dholbert)
Attachment #548102 - Flags: review?(dholbert) → review+
Attachment #548101 - Flags: review?(dholbert) → review+
Crash Signature: [@ nsSMILTimeValueSpec::HandleNewInterval ]
You need to log in before you can comment on or make changes to this bug.