1322770 - SVG triggers assertion: fromParams.mDistToPoint <= toParams.mDistToPoint

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: log.txt

Assertion failure: fromParams.mDistToPoint <= toParams.mDistToPoint (To value shouldn't be before from value on path), at /home/worker/workspace/build/src/dom/svg/SVGMotionSMILType.cpp:360

==11425==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fc51b9c3224 bp 0x7ffc2c429e30 sp 0x7ffc2c429e10 T0)
    #0 0x7fc51b9c3223 in mozilla::SVGMotionSMILType::ComputeDistance(nsSMILValue const&, nsSMILValue const&, double&) const /home/worker/workspace/build/src/dom/svg/SVGMotionSMILType.cpp:357:5
    #1 0x7fc51beb842a in nsSMILAnimationFunction::ComputePacedTotalDistance(FallibleTArray<nsSMILValue> const&) const /home/worker/workspace/build/src/dom/smil/nsSMILAnimationFunction.cpp:610:19
    #2 0x7fc51beb7e5c in nsSMILAnimationFunction::ComputePacedPosition(FallibleTArray<nsSMILValue> const&, double, double&, nsSMILValue const*&, nsSMILValue const*&) /home/worker/workspace/build/src/dom/smil/nsSMILAnimationFunction.cpp:527:26
    #3 0x7fc51beb7256 in nsSMILAnimationFunction::InterpolateResult(FallibleTArray<nsSMILValue> const&, nsSMILValue&, nsSMILValue&) /home/worker/workspace/build/src/dom/smil/nsSMILAnimationFunction.cpp:415:12
    #4 0x7fc51beb631a in nsSMILAnimationFunction::ComposeResult(nsISMILAttr const&, nsSMILValue&) /home/worker/workspace/build/src/dom/smil/nsSMILAnimationFunction.cpp:263:9
    #5 0x7fc51beb3dfa in nsSMILCompositor::ComposeAttribute(bool&) /home/worker/workspace/build/src/dom/smil/nsSMILCompositor.cpp:96:5
    #6 0x7fc51beb2532 in nsSMILAnimationController::DoSample(bool) /home/worker/workspace/build/src/dom/smil/nsSMILAnimationController.cpp:450:5
...
see log.txt

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Comment 2

[Daniel Holbert [:dholbert]]

I'll take a look at this, though I might not get to it right away.

Comment 3

[Daniel Holbert [:dholbert]]

This assertion is just bogus.  Hooray! easy fix. :)

It's asserting (purely as a sanity-check, not as an important requirement) that we're always moving in a positive direction along the animateMotion path.  And in this case (as directed by the keyPoints attribute) that's not the case:
> keyPoints='.6;1;.7'

This keyPoints attribute has us starting 60% of the way along the path, then moving to 100%, and then moving back to 70%.

We actually have a web-platform-test that exercises this scenario (keyPoints going down in value, i.e. moving backwards along the motion path):
>  <animateMotion [...] keyPoints="0.8; 1; 0; 0.2" [...] "/>
https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/testing/web-platform/tests/conformance-checkers/html-svg/animate-elem-33-t-isvalid.html#113

... BUT it doesn't exercise this "ComputePacedPosition" codepath [and hence doesn't trigger this assertion] because it uses calcMode="linear", rather than the default calcMode="paced". If I remove calcMode=linear, it trips this same assertion. So we *almost* have test coverage for this already, but not quite. :)

Comment 4

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 5

[Pascal Chevrel:pascalc]

Daniel, is that still an issue today? It seemed you had a lead in comment #3. Thanks

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Pascal Chevrel:pascalc from comment #5)

Daniel, is that still an issue today? It seemed you had a lead in comment #3. Thanks

The assertion still fires, yeah. Sorry for the delay & thanks for the reminder; I'll post a trivial patch.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (based on animate-elem-33-t-isvalid.html )

Here's a more visual testcase that triggers this error -- this is just a reduced version of the broader testcase that I linked in comment 3, with calcMode="paced" added to trigger the assertion failure.

I'm just posting this so folks can see that we still behave sensibly regardless of the assertion.

(Notably, Chrome seems to disregard all keyPoints after 1.0, or something like that -- they behave functionally different from us on this testcase and on animate-elem-33-t-isvalid.html , and per the test's expectations, that appears to be a Chrome bug.)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: Bug 1322770: Remove bogus assertion from SMIL motion code. r?birtles

We had an assertion here that assumed progression along a motion path must be
positive, but that's not the case when we have decreasing values of
the keyPoints attribute. The assertion wasn't justified and nothing
depended on it being valid, so let's just remove it.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5849b867ecf7
Remove bogus assertion from SMIL motion code. r=birtles

Comment 10

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/5849b867ecf7