Closed Bug 1322770 Opened 4 years ago Closed 1 year ago

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

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox53 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- fixed

People

(Reporter: tsmith, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(4 files)

Attached 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
Attached 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: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Flags: in-testsuite? → in-testsuite+
Keywords: regression
You need to log in before you can comment on or make changes to this bug.