Closed Bug 1290628 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Canvas: 2D, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: tsmith, Assigned: ethlin)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 3 obsolete files)

Attached file 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.
Flags: in-testsuite?
Attached patch use Scalar to check distance (obsolete) — Splinter Review
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
Attachment #8776274 - Flags: review?(mchang)
Attached patch bug_1290628.patch (obsolete) — Splinter Review
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.
Attachment #8776274 - Attachment is obsolete: true
Attachment #8776274 - Flags: review?(mchang)
Attachment #8776277 - Flags: review?(mchang)
Assignee: nobody → ethlin
Attachment #8776277 - Flags: review?(mchang) → review?(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 <=
Attached patch change the assertion (obsolete) — Splinter Review
Change the assertion to '<=' to prevent hitting the assertion when large number.
Attachment #8776277 - Attachment is obsolete: true
Attachment #8776277 - Flags: review?(lsalzman)
Attachment #8776790 - Flags: review?(lsalzman)
Attachment #8776790 - Flags: review?(lsalzman) → review+
Rebase to master.
Attachment #8776790 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67b0bcd172f3
Change the assertion rule to prevent assert with large number. r=lsalzman
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/67b0bcd172f3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Flags: in-testsuite? → in-testsuite+
Ethan, we probably want to uplift that to aurora. Could you fill the uplift request? Thanks
Flags: needinfo?(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
Flags: needinfo?(ethlin)
Attachment #8777186 - Flags: approval-mozilla-aurora?
Comment on attachment 8777186 [details] [diff] [review]
change the assertion (carry r+: lsalzman)

Has stabilized on Nightly for 2 weeks, Aurora50+
Attachment #8777186 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.