Closed
Bug 1322849
Opened 7 years ago
Closed 7 years ago
SVG triggers assertion: "aActiveTime >= 0"
Categories
(Core :: SVG, defect, P3)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla58
People
(Reporter: tsmith, Assigned: longsonr)
References
(Blocks 1 open bug)
Details
(Keywords: assertion, testcase)
Attachments
(3 files, 1 obsolete file)
9.86 KB,
text/plain
|
Details | |
64 bytes,
text/html
|
Details | |
2.77 KB,
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•7 years ago
|
||
Updated•7 years ago
|
Priority: -- → P3
Comment 2•7 years ago
|
||
Confirmed trunk is still affected.
status-firefox55:
--- → wontfix
status-firefox56:
--- → wontfix
status-firefox57:
--- → wontfix
status-firefox58:
--- → affected
status-firefox-esr52:
--- → wontfix
Updated•7 years ago
|
Has Regression Range: --- → no
Assignee | ||
Comment 3•7 years ago
|
||
Assignee: nobody → longsonr
Attachment #8918542 -
Flags: review?(bbirtles)
Comment 4•7 years ago
|
||
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+
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8918542 -
Attachment is obsolete: true
Assignee | ||
Comment 6•7 years ago
|
||
(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)
Comment 7•7 years ago
|
||
(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)
Assignee | ||
Comment 8•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=efacbc5455a7e953578d3d042460617924411c7c
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
Assignee | ||
Updated•7 years ago
|
Flags: in-testsuite+
Comment 10•7 years ago
|
||
bugherder |
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.
Description
•