Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla8

Status

()

Core
SVG
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: birtles)

Tracking

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

Trunk
mozilla8
assertion, crash, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound], crash signature)

Attachments

(8 attachments, 3 obsolete attachments)

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
(Reporter)

Description

6 years ago
Created attachment 543828 [details]
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
(Reporter)

Comment 1

6 years ago
Thanks for adding the abort, btw.  Made it much easier for my fuzzer to catch the bug.
(Reporter)

Comment 2

6 years ago
The too-much-recursion crash must be somewhere else, since the abort is associated with an early return in opt.
(Assignee)

Updated

6 years ago
Assignee: nobody → birtles
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86_64 → All
(Assignee)

Comment 3

6 years ago
Created attachment 546078 [details] [diff] [review]
Part 1 - Crash tests
Attachment #546078 - Flags: review?(dholbert)
(Assignee)

Comment 4

6 years ago
Created attachment 546079 [details] [diff] [review]
Part 2 - Preserve instance times that belong to intervals
Attachment #546079 - Flags: review?(dholbert)
(Assignee)

Comment 5

6 years ago
Created attachment 546082 [details] [diff] [review]
Part 3 - Add assertion to catch potentially problematic instance time deletion
Attachment #546082 - Flags: review?(dholbert)
(Assignee)

Comment 6

6 years ago
Created attachment 546083 [details] [diff] [review]
Part 4 - Make sure filtering of instance times doesn't remove the previous interval's end time
Attachment #546083 - Flags: review?(dholbert)
(Assignee)

Comment 7

6 years ago
Created attachment 546084 [details] [diff] [review]
Part 5 - Make sure other removal functors don't remove instance times that should be preserved
Attachment #546084 - Flags: review?(dholbert)
(Assignee)

Comment 8

6 years ago
Created attachment 546085 [details] [diff] [review]
Part 6 - Break dependencies sooner on unlink
Attachment #546085 - Flags: review?(dholbert)
(Assignee)

Comment 9

6 years ago
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?
(Assignee)

Comment 16

6 years ago
Created attachment 548099 [details] [diff] [review]
Part 2 - Preserve instance times that belong to intervals, r=dholbert

Address review feedback
Attachment #546079 - Attachment is obsolete: true
(Assignee)

Comment 17

6 years ago
Created attachment 548100 [details] [diff] [review]
Part 4 - Make sure filtering of instance times doesn't remove the previous interval's end time, r=dholbert

Address review feedback
Attachment #546083 - Attachment is obsolete: true
(Assignee)

Comment 18

6 years ago
Created attachment 548101 [details] [diff] [review]
Part 6 - Break dependencies sooner on unlink

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)
(Assignee)

Comment 19

6 years ago
Created attachment 548102 [details] [diff] [review]
Part 7 - Mark stack classes as such

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+
(Assignee)

Comment 20

6 years ago
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
Whiteboard: [inbound]
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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8

Updated

6 years ago
Crash Signature: [@ nsSMILTimeValueSpec::HandleNewInterval ]
You need to log in before you can comment on or make changes to this bug.