Closed Bug 1322849 Opened 7 years ago Closed 7 years ago

SVG triggers assertion: "aActiveTime >= 0"

Categories

(Core :: SVG, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: tsmith, Assigned: longsonr)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 1 obsolete file)

Attached file log.txt
Assertion failure: aActiveTime >= 0 (Expecting non-negative active time), at /home/worker/workspace/build/src/dom/smil/nsSMILTimedElement.cpp:1978

==26403==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fc5f57db65e bp 0x7ffc79f082b0 sp 0x7ffc79f08280 T0)
    #0 0x7fc5f57db65d in nsSMILTimedElement::ActiveTimeToSimpleTime(long, unsigned int&) /home/worker/workspace/build/src/dom/smil/nsSMILTimedElement.cpp:1976:3
    #1 0x7fc5f57dab9f in nsSMILTimedElement::SampleFillValue() /home/worker/workspace/build/src/dom/smil/nsSMILTimedElement.cpp:2194:5
    #2 0x7fc5f57d8ab5 in nsSMILTimedElement::DoSampleAt(long, bool) /home/worker/workspace/build/src/dom/smil/nsSMILTimedElement.cpp:687:11
    #3 0x7fc5f57bc042 in nsSMILAnimationController::DoMilestoneSamples() /home/worker/workspace/build/src/dom/smil/nsSMILAnimationController.cpp:576:9
    #4 0x7fc5f57bb0bb in nsSMILAnimationController::DoSample(bool) /home/worker/workspace/build/src/dom/smil/nsSMILAnimationController.cpp:339:3
    #5 0x7fc5f611e22b in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) /home/worker/workspace/build/src/layout/base/nsPresShell.cpp:4105:9
    #6 0x7fc5f611db3b in PresShell::FlushPendingNotifications(mozFlushType) /home/worker/workspace/build/src/layout/base/nsPresShell.cpp:4007:3
    #7 0x7fc5f2e8e6a1 in nsDocument::FlushPendingNotifications(mozFlushType) /home/worker/workspace/build/src/dom/base/nsDocument.cpp:7755:7
    #8 0x7fc5f21f40a3 in nsDocLoader::DocLoaderIsEmpty(bool) /home/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:683:9
...
see log.txt
Attached file test_case.html
Priority: -- → P3
Has Regression Range: --- → no
Attached patch patch with crashtest (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #8918542 - Flags: review?(bbirtles)
Comment on attachment 8918542 [details] [diff] [review]
patch with crashtest

Review of attachment 8918542 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: dom/smil/nsSMILTimedElement.cpp
@@ +1927,4 @@
>  {
>    nsSMILTimeValue multipliedDuration;
>    if (mRepeatCount.IsDefinite() && mSimpleDur.IsDefinite()) {
> +    if (mRepeatCount * double(mSimpleDur.GetMillis()) <= INT64_MAX) {

Would std::numeric_limits<nsSMILTime>::max() be more appropriate here? I guess we use INT64_MAX elsewhere in this file, but maybe we should change that?

And in the overflow case would it be better to set multipliedDuration to Indefinite()? (i.e. combine this clause with the above if condition?

@@ +1928,5 @@
>    nsSMILTimeValue multipliedDuration;
>    if (mRepeatCount.IsDefinite() && mSimpleDur.IsDefinite()) {
> +    if (mRepeatCount * double(mSimpleDur.GetMillis()) <= INT64_MAX) {
> +      multipliedDuration.SetMillis(
> +        nsSMILTime(mRepeatCount * mSimpleDur.GetMillis()));

Any reason we drop the cast to double() before doing multiplication here?

(I assume we added that so that the multiplication happens in floating point space.)
Attachment #8918542 - Flags: review?(bbirtles) → review+
Attachment #8918542 - Attachment is obsolete: true
(In reply to Brian Birtles (:birtles) from comment #4)
> 
> Would std::numeric_limits<nsSMILTime>::max() be more appropriate here? I
> guess we use INT64_MAX elsewhere in this file, but maybe we should change
> that?

I've done that for all nsSMILTime tests.

> 
> And in the overflow case would it be better to set multipliedDuration to
> Indefinite()? (i.e. combine this clause with the above if condition?
> 

Well the default value is unresolved, is indefinite better. Unresolved would
seem to be logically what we have here as we can't calculate a value.
I can change it to indefinite if you think it makes more sense.

> 
> Any reason we drop the cast to double() before doing multiplication here?
> 

Once we've determined the calculation won't overflow doing it as a double and
then implicitly casting it back to an int64 just adds overhead doesn't it?
Flags: needinfo?(bbirtles)
(In reply to Robert Longson [:longsonr] from comment #6)
> (In reply to Brian Birtles (:birtles) from comment #4)
> > 
> > Would std::numeric_limits<nsSMILTime>::max() be more appropriate here? I
> > guess we use INT64_MAX elsewhere in this file, but maybe we should change
> > that?
> 
> I've done that for all nsSMILTime tests.

Great, thank you!

> > And in the overflow case would it be better to set multipliedDuration to
> > Indefinite()? (i.e. combine this clause with the above if condition?
> > 
> 
> Well the default value is unresolved, is indefinite better. Unresolved would
> seem to be logically what we have here as we can't calculate a value.
> I can change it to indefinite if you think it makes more sense.

I suspect in practice they work out to the same thing so it probably doesn't matter.

> > Any reason we drop the cast to double() before doing multiplication here?
> > 
> 
> Once we've determined the calculation won't overflow doing it as a double and
> then implicitly casting it back to an int64 just adds overhead doesn't it?

Actually, I checked and nsSMILRepeatCount implicitly converts to a double so this calculation is going to happen in double space anyway.
Flags: needinfo?(bbirtles)
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11fcd28ed76e
Add a range check when the duration is multiplied by the repeat count. r=birtles
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/11fcd28ed76e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: