1286458 - MOZ_CRASH "0 <= dot && dot <= 1.0f + (1.0f / (1 << 12))" in [@SkConic::BuildUnitArc]

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Found with debug build.

/builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/skia/skia/src/core/SkGeometry.cpp:1319: fatal error: ""0 <= dot && dot <= 1.0f + (1.0f / (1 << 12))""
Abort from sk_abort
Hit MOZ_CRASH() at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/memory/mozalloc/mozalloc_abort.cpp:33
ASAN:DEADLYSIGNAL
=================================================================
==7110==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0000004e19d3 bp 0x7fff3c3c4320 sp 0x7fff3c3c4310 T0)
    #0 0x4e19d2 in mozalloc_abort(char const*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/memory/mozalloc/mozalloc_abort.cpp:33:5
    #1 0x7f6f6e81b3f4 in sk_abort_no_print() /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/skia/skia/src/ports/SkMemory_mozalloc.cpp:16:5
    #2 0x7f6f6e84306b in SkConic::BuildUnitArc(SkPoint const&, SkPoint const&, SkRotationDirection, SkMatrix const*, SkConic*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/skia/skia/src/core/SkGeometry.cpp:1319:5
    #3 0x7f6f6e98d698 in RoundJoiner(SkPath*, SkPath*, SkPoint const&, SkPoint const&, SkPoint const&, float, float, bool, bool) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/skia/skia/src/core/SkStrokerPriv.cpp:136:17
    #4 0x7f6f6e9816e0 in SkPathStroker::preJoinTo(SkPoint const&, SkPoint*, SkPoint*, bool) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/skia/skia/src/core/SkStroke.cpp:273:9
    #5 0x7f6f6e982d50 in SkPathStroker::lineTo(SkPoint const&, SkPath::Iter const*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/skia/skia/src/core/SkStroke.cpp:419:10
    #6 0x7f6f6e989704 in SkPathStroker::cubicTo(SkPoint const&, SkPoint const&, SkPoint const&) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/skia/skia/src/core/SkStroke.cpp:1246:9
    #7 0x7f6f6e98a16b in SkStroke::strokePath(SkPath const&, SkPath*) const /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/skia/skia/src/core/SkStroke.cpp:1403:17
    #8 0x7f6f6e98bf5e in SkStrokeRec::applyToPath(SkPath*, SkPath const&) const /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/skia/skia/src/core/SkStrokeRec.cpp:121:5
    #9 0x7f6f6e8ea010 in SkPaint::getFillPath(SkPath const&, SkPath*, SkRect const*, float) const /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/skia/skia/src/core/SkPaint.cpp:1975:10
    #10 0x7f6f6e78354c in SkDraw::drawPath(SkPath const&, SkPaint const&, SkMatrix const*, bool, bool, SkBlitter*) const /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/skia/skia/src/core/SkDraw.cpp:1091:18
...
see log.txt for full log.

Comment 1

[Tyson Smith [:tsmith]]

Attachment: log.txt

Comment 2

[Vincent Liu[:vliu]]

Attachment: Check infinity before setResScale().

Crash happens since there is no infinity check before calling setResScale(). Jeff, could you please have a review for me? Thanks

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Lee is a more appropriate reviewer

Comment 4

[Lee Salzman [:lsalzman]]

Comment on attachment 8770799 [details] [diff] [review]
Check infinity before setResScale().

I would prefer something like:

SkScalar scale = SkScalarAbs(viewMatrix.getMaxScale());
if (!SkScalarIsFinite(scale)) { ... }
dashlessStrokeInfo.setResScale(scale);

Also for the comment I think something like "View matrix scale is not finite." would be clearer.

Also of note, this code section of code no longer exists upstream at present, so I will have to revise it in a couple of weeks if/when I start the next Skia update. We can still put it in the Firefox tree, but we can't upstream this right now at all.

Comment 5

[Vincent Liu[:vliu]]

Attachment: Check infinity before setResScale().

Comment 6

[Vincent Liu[:vliu]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=23b8e87e3b86&selectedJob=24098120

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c805161da79c
Check infinity before setResScale(). r=lsalzman

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c805161da79c

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Is there a reason the testcase wasn't landed as a crashtest?

Comment 10

[Vincent Liu[:vliu]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> Is there a reason the testcase wasn't landed as a crashtest?

I will add crashtest ASAP.

Comment 11

[Vincent Liu[:vliu]]

Attachment: Add crashtest.

HI Lee,

I'd missing crashtest for this bug. Could you please have a review? Thanks

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f50eb01832c
Add crashtest. r=lsalzman

Comment 13

[Iris Hsiao [:ihsiao]]

Sorry had to back out your push for Reftest failures, e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=32992914&repo=mozilla-inbound#L6764

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b7cac6613e1
Backed out changeset 6f50eb01832c for Reftest failures

Comment 15

[Vincent Liu[:vliu]]

(In reply to Pulsebot from comment #14)
> Backout by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4b7cac6613e1
> Backed out changeset 6f50eb01832c for Reftest failures

It seems that this crash test hit different crash point in linux or windows build. I will reopen this bug and add a patch to fix it.

Comment 16

[Vincent Liu[:vliu]]

Attachment: Check SkScalarIsNaN for SkVector::DotProduct().

Hi Lee,
The patch intends to add NaN check to fix the crash. Could you please have a review? Really thanks.

Comment 17

[Lee Salzman [:lsalzman]]

Comment on attachment 8776798 [details] [diff] [review]
Check SkScalarIsNaN for SkVector::DotProduct().

Review of attachment 8776798 [details] [diff] [review]:
-----------------------------------------------------------------

I would prefer !SkScalarIsFinite() instead of SkScalarIsNaN() since it is also possible an Inf could sneak in there.

Comment 18

[Vincent Liu[:vliu]]

Attachment: Check SkScalarIsFinite for SkVector::DotProduct().

Hi Lee,
Could you please have a review? Thanks

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc7ac901d275
Check SkScalarIsFinite for SkVector::DotProduct(). r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebadba7031f6
Add crashtest. r=lsalzman

Comment 20

[Vincent Liu[:vliu]]

(In reply to Vincent Liu[:vliu] from comment #18)
> Created attachment 8778026 [details] [diff] [review]
> Check SkScalarIsFinite for SkVector::DotProduct().
> 
> Hi Lee,
> Could you please have a review? Thanks

try result for crash test in [1]

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb013d49f128

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cc7ac901d275
https://hg.mozilla.org/mozilla-central/rev/ebadba7031f6