594653 - "ABORT: Instance times have not been assigned serial numbers" with <svg:animate>, setCurrentTime

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (triggers abort when loaded)

###!!! ABORT: Instance times have not been assigned serial numbers: 'aElem1->Serial() && aElem2->Serial()', file content/smil/nsSMILTimedElement.cpp, line 91

Comment 1

[Brian Birtles (:birtles)]

I haven't dug into this with a debugger but my initial suspicion is that the problem is nsSMILTimedElement::UnpreserveInstanceTimes where we use the InstanceTimeComparator with instance times that aren't necessarily from the instance time list (e.g. active ends, early ends, default begins... all these are generated as needed and not given a serial number. Instance times in the lists all have serial numbers.)

The best solution is probably to remove the NS_ABORT_IF_FALSE and define some sensible behaviour if there's no serial number. An alternative would be to assign a serial number. I'll look into it a bit more to see what make sense.

For now I'm nominating as a blocker since it blocks bug 369230 which appears to be security critical. I'm not on the CC list for that one but I assume it's serious enough to warrant this being a blocker.

I expect this will be about 1 day's work to fix.

Comment 2

[Jesse Ruderman]

Bug 369230 is a secret metabug, not a security hole.

Comment 3

[Brian Birtles (:birtles)]

Attachment: Patch v1a

Proposed patch.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 477381 [details] [diff] [review]
Patch v1a

>-    if (!cutoff || cmp.LessThan(cutoff, instance)) {
>+    if (!cutoff || cutoff->Time().CompareTo(instance->Time())) {

So this replaces a LessThan call (which returns PRBool) with a CompareTo call (which returns PRInt8 -- 0 if equal, negative if lessthan, positive if morethan).

To preserve existing behavior, don't you need a "< 0" after the CompareTo call?

Comment 5

[Brian Birtles (:birtles)]

Attachment: Patch v1b

(In reply to comment #4)
> So this replaces a LessThan call (which returns PRBool) with a CompareTo call
> (which returns PRInt8 -- 0 if equal, negative if lessthan, positive if
> morethan).
> 
> To preserve existing behavior, don't you need a "< 0" after the CompareTo call?

Yes, that was a silly mistake. Unfortunately, this mistake didn't cause any tests to fail so I've added an extra test in this patch to check this behaviour.

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 480810 [details] [diff] [review]
Patch v1b

Looks great!

Comment 7

[Brian Birtles (:birtles)]

Comment on attachment 480810 [details] [diff] [review]
Patch v1b

(In reply to comment #6)
> Looks great!
Thanks Daniel!

Requesting approval to land. This patch makes previously undefined behaviour properly defined and fixes a failing abort allowing further testing to be performed.

Comment 8

[Brian Birtles (:birtles)]

Pushed: http://hg.mozilla.org/mozilla-central/rev/d6c0ae227b3c