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

RESOLVED FIXED in mozilla2.0b7

Status

()

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

People

(Reporter: Jesse Ruderman, Assigned: birtles)

Tracking

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

Trunk
mozilla2.0b7
x86
Mac OS X
assertion, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
Created attachment 473379 [details]
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
(Assignee)

Updated

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

Comment 1

8 years ago
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.
blocking2.0: --- → ?
(Reporter)

Comment 2

8 years ago
Bug 369230 is a secret metabug, not a security hole.
blocking2.0: ? → ---
(Assignee)

Comment 3

8 years ago
Created attachment 477381 [details] [diff] [review]
Patch v1a

Proposed patch.
Attachment #477381 - Flags: review?(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?
(Assignee)

Comment 5

8 years ago
Created attachment 480810 [details] [diff] [review]
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.
Attachment #477381 - Attachment is obsolete: true
Attachment #480810 - Flags: review?(dholbert)
Attachment #477381 - Flags: review?(dholbert)
Comment on attachment 480810 [details] [diff] [review]
Patch v1b

Looks great!
Attachment #480810 - Flags: review?(dholbert) → review+
(Assignee)

Comment 7

8 years ago
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.
Attachment #480810 - Flags: approval2.0?
(Assignee)

Comment 8

8 years ago
Pushed: http://hg.mozilla.org/mozilla-central/rev/d6c0ae227b3c
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.