1290628 - MOZ_CRASH "d0 < fPathLength" in [@SpecialLineRec::addSegment()]

Status: RESOLVED FIXED

(Core :: Canvas: 2D, defect)

Description

[Tyson Smith [:tsmith] (PTO until Sept 4)]

Attachment: log.txt

Found with debug build.

/builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/skia/skia/src/utils/SkDashPath.cpp:188: fatal error: ""d0 < fPathLength""
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
=================================================================
==29173==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0000004e19d3 bp 0x7ffd2380df00 sp 0x7ffd2380def0 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 0x7f7efd883e24 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 0x7f7efd887c81 in SpecialLineRec::addSegment(float, float, SkPath*) const /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/skia/skia/src/utils/SkDashPath.cpp:188:9
    #3 0x7f7efd887053 in SkDashPath::InternalFilter(SkPath*, SkPath const&, SkStrokeRec*, SkRect const*, float const*, int, float, int, float) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/skia/skia/src/utils/SkDashPath.cpp:275:21
    #4 0x7f7efd60e33e in SkDashPathEffect::filterPath(SkPath*, SkPath const&, SkStrokeRec*, SkRect const*) const /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/skia/skia/src/effects/SkDashPathEffect.cpp:40:12
    #5 0x7f7efd952a1d 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:1971:24
    #6 0x7f7efd7ebf7c 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
    #7 0x7f7efd5a8adb in SkDraw::drawPath(SkPath const&, SkPaint const&, SkMatrix const*, bool) const /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/skia/skia/include/core/SkDraw.h:54:9
    #8 0x7f7efd7e9668 in SkDraw::drawPoints(SkCanvas::PointMode, unsigned long, SkPoint const*, SkPaint const&, bool) const /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/skia/skia/src/core/SkDraw.cpp:730:25
    #9 0x7f7efd5e3534 in SkCanvas::onDrawPoints(SkCanvas::PointMode, unsigned long, SkPoint const*, SkPaint const&) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/skia/skia/src/core/SkCanvas.cpp:2082:9
...
see log.txt for full log.

Comment 1

[Tyson Smith [:tsmith] (PTO until Sept 4)]

Attachment: test_case.html

Comment 2

[Ethan Lin[:ethlin]]

Attachment: use Scalar to check distance

The length check is different between [1] and [2]. One is using Scalar and the other one is using double. So using Scalar to do the check can fix the problem.

[1] https://hg.mozilla.org/mozilla-central/annotate/2ea3d51ba1bb9f5c3b6921c43ea63f70b4fdf5d2/gfx/skia/skia/src/utils/SkDashPath.cpp#l267
[2] https://hg.mozilla.org/mozilla-central/annotate/2ea3d51ba1bb9f5c3b6921c43ea63f70b4fdf5d2/gfx/skia/skia/src/utils/SkDashPath.cpp#l189

Comment 3

[Ethan Lin[:ethlin]]

Attachment: bug_1290628.patch

Using Scalar to do the length checking may cause infinite loop problem. I think we should just remove the assertion in 'addSegment' since there is only one place use the function and the loop already checked the length in double.

Comment 4

[Lee Salzman [:lsalzman]]

(In reply to Ethan Lin[:ethlin] from comment #2)
> Created attachment 8776274 [details] [diff] [review]
> use Scalar to check distance
> 
> The length check is different between [1] and [2]. One is using Scalar and
> the other one is using double. So using Scalar to do the check can fix the
> problem.
> 
> [1]
> https://hg.mozilla.org/mozilla-central/annotate/
> 2ea3d51ba1bb9f5c3b6921c43ea63f70b4fdf5d2/gfx/skia/skia/src/utils/SkDashPath.
> cpp#l267
> [2]
> https://hg.mozilla.org/mozilla-central/annotate/
> 2ea3d51ba1bb9f5c3b6921c43ea63f70b4fdf5d2/gfx/skia/skia/src/utils/SkDashPath.
> cpp#l189

Rather than get rid of the assert, I think it would be better to change the < to <=

Comment 5

[Ethan Lin[:ethlin]]

Attachment: change the assertion

Change the assertion to '<=' to prevent hitting the assertion when large number.

Comment 6

[Ethan Lin[:ethlin]]

Attachment: change the assertion (carry r+: lsalzman)

Rebase to master.

Comment 7

[Ethan Lin[:ethlin]]

try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd036fb5c52e

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67b0bcd172f3
Change the assertion rule to prevent assert with large number. r=lsalzman

Comment 9

[Lee Salzman [:lsalzman]]

Upstream Skia bug report: https://codereview.chromium.org/2209303004

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/67b0bcd172f3

Comment 11

[Sylvestre Ledru [:sylvestre]]

Ethan, we probably want to uplift that to aurora. Could you fill the uplift request? Thanks

Comment 12

[Ethan Lin[:ethlin]]

Comment on attachment 8777186 [details] [diff] [review]
change the assertion (carry r+: lsalzman)

Approval Request Comment
[Feature/regressing bug #]: none
[User impact if declined]: Debug build may crash when using canvas.
[Describe test coverage new/current, TreeHerder]: test on locally and tryserver
[Risks and why]: low risk
[String/UUID change made/needed]: none

Comment 13

[Ritu Kothari (:ritu)]

Comment on attachment 8777186 [details] [diff] [review]
change the assertion (carry r+: lsalzman)

Has stabilized on Nightly for 2 weeks, Aurora50+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1955c0608854