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

RESOLVED FIXED in Firefox 50

Status

()

Core
Canvas: 2D
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tsmith, Assigned: vliu)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

50 Branch
mozilla50
x86_64
Linux
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox50 fixed, firefox51 fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8770414 [details]
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.
(Reporter)

Comment 1

2 years ago
Created attachment 8770415 [details]
log.txt
(Assignee)

Updated

2 years ago
Assignee: nobody → vliu
(Assignee)

Comment 2

2 years ago
Created attachment 8770799 [details] [diff] [review]
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
Attachment #8770799 - Flags: review?(jmuizelaar)
Attachment #8770799 - Flags: review?(jmuizelaar) → review?(lsalzman)
Lee is a more appropriate reviewer
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.
(Assignee)

Comment 5

2 years ago
Created attachment 8771831 [details] [diff] [review]
Check infinity before setResScale().
Attachment #8771831 - Flags: review?(lsalzman)
(Assignee)

Updated

2 years ago
Attachment #8770799 - Attachment is obsolete: true
Attachment #8770799 - Flags: review?(lsalzman)
Attachment #8771831 - Flags: review?(lsalzman) → review+

Comment 7

2 years ago
Pushed by vliu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c805161da79c
Check infinity before setResScale(). r=lsalzman

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c805161da79c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Reporter)

Updated

2 years ago
Blocks: 1289929, 1289609
Is there a reason the testcase wasn't landed as a crashtest?
Flags: needinfo?(vliu)
Flags: in-testsuite?
(Assignee)

Comment 10

2 years ago
(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.
Flags: needinfo?(vliu)
(Assignee)

Comment 11

2 years ago
Created attachment 8776444 [details] [diff] [review]
Add crashtest.

HI Lee,

I'd missing crashtest for this bug. Could you please have a review? Thanks
Attachment #8776444 - Flags: review?(lsalzman)
Attachment #8776444 - Flags: review?(lsalzman) → review+
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
Flags: needinfo?(vliu)

Comment 14

2 years ago
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b7cac6613e1
Backed out changeset 6f50eb01832c for Reftest failures
(Assignee)

Comment 15

2 years ago
(In reply to Pulsebot from comment #14)
> Backout by ihsiao@mozilla.com:
> 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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 16

2 years ago
Created attachment 8776798 [details] [diff] [review]
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.
Attachment #8776798 - Flags: review?(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.
Attachment #8776798 - Flags: review?(lsalzman) → review-
(Assignee)

Comment 18

2 years ago
Created attachment 8778026 [details] [diff] [review]
Check SkScalarIsFinite for SkVector::DotProduct().

Hi Lee,
Could you please have a review? Thanks
Attachment #8778026 - Flags: review?(lsalzman)
(Assignee)

Updated

2 years ago
Attachment #8776798 - Attachment is obsolete: true
Attachment #8778026 - Flags: review?(lsalzman) → review+

Comment 19

2 years ago
Pushed by vliu@mozilla.com:
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
(Assignee)

Comment 20

2 years ago
(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
Flags: needinfo?(vliu)

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cc7ac901d275
https://hg.mozilla.org/mozilla-central/rev/ebadba7031f6
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.