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

RESOLVED FIXED in Firefox 67

Status

()

defect
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: tsmith, Assigned: dholbert)

Tracking

(Blocks 1 bug, {assertion, testcase})

Trunk
mozilla67
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox-esr60 wontfix, firefox53 wontfix, firefox56 wontfix, firefox57 wontfix, firefox58 wontfix, firefox59 wontfix, firefox65 wontfix, firefox66 wontfix, firefox67 fixed)

Details

Attachments

(4 attachments)

Posted file 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
Posted file test_case.html
I'll take a look at this, though I might not get to it right away.
Flags: needinfo?(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. :)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED

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

(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.

Flags: needinfo?(dholbert)

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.)

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.

Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5849b867ecf7
Remove bogus assertion from SMIL motion code. r=birtles
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.