966634 - SVG ABORT: To value shouldn't be before from value on path: 'fromParams.mDistToPoint <= toParams.mDistToPoint', file [...]/SVGMotionSMILType.cpp, line 360)

Status: RESOLVED WORKSFORME

(Core :: SVG, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

Tested with http://hg.mozilla.org/integration/mozilla-inbound/rev/1e7b2bb3041b

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Daniel Holbert [:dholbert]]

I can reproduce on Linux.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Here's a significantly reduced testcase.

I think the issue is the decreasing stretch of keyPoints (which indeed puts the "to" value earlier on the path than the "from" value, for a segment of animation).

So it may be that this assertion should just go away. (assuming we handle things correctly)

Comment 4

[Daniel Holbert [:dholbert]]

AFAICT, this doesn't actually crash; it just fails an ABORT_IF_FALSE (a fatal assertion). Nothing actually goes wrong in release builds, and we even proceed just fine in debug builds if I soften the ABORT_IF_FALSE, locally.

Hence, downgrading "crash" keyword to "assertion".

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (with some movement)

(Now I've added a "dur" attribute to the testcase, so that it actually animates visibly. This lets you see the "backwards" stretch of the animation that we seem to be complaining about. This spams the assertion many more times, too (because we evaluate this chunk of the animation many more times, now that we're actually playing through it.)

Comment 6

[Daniel Holbert [:dholbert]]

[not sure if this has been around ever since animateMotion support was added, or was introduced more recently than that.  Adding dependency on that bug anyway, since it's related at least]

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 4 (with keyTimes added)

The previous testcases no longer trigger this bug, but that's only because they've become invalid due to bug 974698 (i.e. now we require a keyTimes attribute)

Here's an updated version of testcase 3, modified to add |keyTimes| so that we honor it as valid.  This still reproduces this bug's assertion.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.

Comment 10

[Daniel Holbert [:dholbert]]

Reopening -- we have a testcase that reliably reproduces the crash (i.e. we have reliable STR).  Please don't close just due to crash volume. (Also note: this is an abort, i.e. only crashes in debug builds, so it's not surprising that it's not being hit in the wild.)

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Yeah, we noticed this issue in the bot, sorry, we will fix that (ignoring but with testcase). Thanks for the input!

Comment 12

[Rares Doghi, Desktop QA]

Hi Daniel, I tried to reproduce this issue in a Debug build using the attached testcase 4 but I was simply dragging it into tab and loading the page but no crashes or errors would occur in Browser Console, where could I check the Assertion error ? does this issue still occur in our latest debug builds ?

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Rares Doghi from comment #12)

in Browser Console, where could I check the Assertion error ?

The assertion text show up in your command-line, i.e. your terminal app (the Terminal app on Mac, or e.g. gnome-terminal on Linux, or cmd.exe on Windows), if you start Firefox from there.

does this issue still occur in our latest debug builds ?

Doesn't seem to! I can load all testcases just fine, with no abort and no assertions that I'm seeing go by in my terminal-spew.

--> closing as WFM.