665334 - Crash with two <svg:animate> elements with the same ID

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Comment 1

[Jesse Ruderman]

Attachment: stack trace (repeating portion only)

Comment 2

[Daniel Holbert [:dholbert]]

Brian, looks like this is syncbase stuff (<animate id="a" end="a.begin" />) -- perhaps you could take this?

Comment 3

[Brian Birtles (:birtles)]

Looks like the sort of thing I was looking at in bug 664343. I'm thinking of adding some recursion limits within UpdateCurrentInterval. We currently break cycles for change notifications but not when we end up in a situation where deleting one interval creates a new one and so on.

Comment 4

[Brian Birtles (:birtles)]

Attachment: Part 1 - Crashtest v1a

Comment 5

[Brian Birtles (:birtles)]

Attachment: Part 2 - Fallback to detect infinite recursion when updating intervals v1a

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 542345 [details] [diff] [review]
Part 1 - Crashtest v1a

r+ on the crashtest (assuming you've verified that it currently crashes when run via the crashtest harness :))

Comment 7

[Brian Birtles (:birtles)]

Attachment: Part 3 - Keep instance times that are used as a fixed endpoint of an interval

Comment 8

[Brian Birtles (:birtles)]

(In reply to comment #6)
> Comment on attachment 542345 [details] [diff] [review] [review]
> Part 1 - Crashtest v1a
> 
> r+ on the crashtest (assuming you've verified that it currently crashes when
> run via the crashtest harness :))

Wow, that was fast! Yep, it sure does!

Comment 9

[Brian Birtles (:birtles)]

Some analysis for future reference.

1) SMIL doesn't allow two zero-length intervals at the same time

2) If an animation has an end condition and zero end times then it is ok to create an interval with an indefinite end. But if it has 1 or more end times, those end times must be used or else no interval can be made.

This test case was getting us into an odd situation where we had:

a) One zero-length interval, but

b) No end instance times (because RemoveByCreator was removing all of them, even the fixed end time of the one zero-length interval that had finished).

Since there are no instance times we'd create a new interval with an indefinite end.

However, that would cause the end condition "a.begin" to generate a new instance time at the same time as the begin time.

That of course would produce a zero-length interval and we can't have two zero-length intervals at the same time so we'd delete the new interval. And so on.

This patch (specifically, part 3) works by preserving the instance times which form fixed endpoints of intervals. We probably should have been doing this all along since this is what we do elsewhere in this class. As a result, (b) doesn't happen. That is, we don't create a new interval of indefinite end because the end instance time list is not empty.

Part 2 will hopefully help us catch this kind of problem more readily in the future. Also, it should release builds from crashing. I've verified that the assertion fails as expected with this test case when only part 2 is applied.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 542346 [details] [diff] [review]
Part 2 - Fallback to detect infinite recursion when updating intervals v1a

>   AutoIntervalUpdateBatcher(nsSMILTimedElement& aTimedElement)
>     : mTimedElement(aTimedElement)
>   {
>-    mTimedElement.mDeferIntervalUpdates = PR_TRUE;
>+    mTimedElement.mIntervalUpdateBlockCount++;

(per IRC discussion, I think this was cruft that slipped in)

>+// Detect if we arrive in some sort undetected of recursive syncbase dependency
>+// relationship
>+const PRUint16 nsSMILTimedElement::sMaxUpdateIntervalRecursionDepth = 20;

s/sort undetected of/sort of undetected/

>+  // Check we aren't stuck in infinite recursion updating some syncbase
>+  // dependencies. Generally such situations should be detected in advance and
>+  // the chain broken in a sensible and predictable manner so if we're hitting
>+  // this assertion we need to work out how to detect the case that's causing
>+  // it. In release builds just bail out before we overflow the stack.
>+  if (++mUpdateIntervalRecursionDepth > sMaxUpdateIntervalRecursionDepth) {
>+    NS_ABORT_IF_FALSE(PR_FALSE,
>+        "Update current interval recursion depth exceeded threshold");
>+    return;
>+  }

Nits: s/Check we/Check that we/
Add commas after "manner" and "In release builds"

> 
>@@ -1932,16 +1948,22 @@ nsSMILTimedElement::UpdateCurrentInterva
>       // The transition to the postactive state will take place on the next
>       // sample (along with firing end events, clearing intervals etc.)
>       RegisterMilestone();
>     } else if (mElementState == STATE_WAITING) {
>       mElementState = STATE_POSTACTIVE;
>       ResetCurrentInterval();
>     }
>   }
>+
>+  // If we're hitting this we must have early returns so consider using an
>+  // auto incrementer/decrementer
>+  NS_ABORT_IF_FALSE(mUpdateIntervalRecursionDepth > 0,
>+    "Unbalanced increment/decrement of update interval recursion depth check");
>+  --mUpdateIntervalRecursionDepth;

Good intuition, to be worried about early-returns messing this up.  However, I don't think the assertion checks the right thing.  If we did add an early return to this function, that would leave mUpdateIntervalRecursionDepth **too high**, not too low.

I can't immediately think of anything useful that we can assert to check that, actually :-/  So maybe delete the assertion & comment, and instead add a comment after the "++mUpdateIntervalRecursionDepth", along the lines of:
  // NO EARLY RETURNS ALLOWED AFTER THIS POINT! (If we need one, then switch
  // mUpdateIntervalRecursionDepth to use an auto incrementer/decrementer.)

(Side-note: we already have an AutoIncrement impl at
 http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsIOService.cpp#532
Perhaps that should move to mfbt, along with the handy AutoBoolSetter that you added in another bug a while back...)

Comment 11

[Brian Birtles (:birtles)]

Attachment: Part 2 - Fallback to detect infinite recursion when updating intervals v1b, r=dholbert

Address review feedback.

Comment 12

[Brian Birtles (:birtles)]

Attachment: Part 4 - Move AutoIncrement to mfbt

Move AutoIncrement to mfbt as suggested in comment 10. Probably there's someone else I ought to get to review changes to mfbt/netwerk. Any suggestions?

Comment 13

[Daniel Holbert [:dholbert]]

cjones I think?  Or you could look at hg blame for that directory for recent committers/reviewers there.

Comment 14

[Daniel Holbert [:dholbert]]

(In case cjones objects or doesn't get to this right away -- the first 3 parts of this bug can land on their own without part 4, right?)

Comment 15

[Brian Birtles (:birtles)]

(In reply to comment #14)
> (In case cjones objects or doesn't get to this right away -- the first 3
> parts of this bug can land on their own without part 4, right?)

Yes, they can. I was under the impression we tried to land all the patches for a bug rather than have it in a half-landed state, but I guess that's not the case after all.

I've pushed the first three to m-i for now:
http://hg.mozilla.org/integration/mozilla-inbound/rev/fba10b407cf9
http://hg.mozilla.org/integration/mozilla-inbound/rev/df0884806f2f
http://hg.mozilla.org/integration/mozilla-inbound/rev/b48ff0b329b6

I'm not marking the whiteboard as [inbound] since I guess I don't want this bug marked fixed until patch 4 lands. (Is that how we do things here?) Hopefully patch 4 is straightforward but if it turns out to be contentious I'll spin off another bug for that patch.

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to comment #15)
> Yes, they can. I was under the impression we tried to land all the patches
> for a bug rather than have it in a half-landed state, but I guess that's not
> the case after all.

Your impression is right -- generally that is the case.  In this case, though, patch 4 isn't related to the actual fix. (In fact, I'd initially imagined that that change would happen in a separate bug -- sorry for not being clearer about that.)  

If cjones doesn't get to this in the next day or so[1], I think it would indeed be best to give patch 4 its own bug, so that we can close outthis bug out once its already-landed parts are merged from m-i to m-c.  Normally it'd be fine to wait for the followup, either way, but given that we're *about* to launch the next Aurora train, I think it'll be saner bug-tracking-wise if we didn't leave some part of this bug as being m-c-only.

[1] (I wouldn't bet on him getting to it, given that it's a low-priority change & it's a holiday weekend in the US)

Comment 17

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/fba10b407cf9
http://hg.mozilla.org/mozilla-central/rev/df0884806f2f
http://hg.mozilla.org/mozilla-central/rev/b48ff0b329b6

Comment 18

[Brian Birtles (:birtles)]

(In reply to comment #16)
> If cjones doesn't get to this in the next day or so[1], I think it would
> indeed be best to give patch 4 its own bug, so that we can close outthis bug
> out once its already-landed parts are merged from m-i to m-c.  Normally it'd
> be fine to wait for the followup, either way, but given that we're *about*
> to launch the next Aurora train, I think it'll be saner bug-tracking-wise if
> we didn't leave some part of this bug as being m-c-only.

Ok, thanks for explaining that. That helps a lot.

I'll file a new bug when I get to work on Tuesday morning (Japan time) which will probably be before cjones gets to it anyway since it's a holiday weekend as you say.

Thanks again Daniel!

Comment 19

[Brian Birtles (:birtles)]

(In reply to comment #18)
> I'll file a new bug when I get to work on Tuesday morning (Japan time) which
> will probably be before cjones gets to it anyway since it's a holiday
> weekend as you say.

Filed bug 669234 for part 4.

Comment 20

[Vlad [QA]]

Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0

The test case from the description doesn't crash Firefox anymore.

Comment 21

[Jesse Ruderman]

http://hg.mozilla.org/mozilla-central/rev/fba10b407cf9 added the testcase as a crashtest.