crash near NULL in [@SkDraw::drawPath]

RESOLVED FIXED in Firefox 52

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tsmith, Assigned: kechen)

Tracking

(Blocks 2 bugs, {crash, csectype-nullptr, testcase})

Trunk
mozilla53
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox51 unaffected, firefox52 fixed, firefox53 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(5 attachments, 2 obsolete attachments)

Reporter

Description

3 years ago
Posted file test_case.html
==9334==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000032 (pc 0x7fdc2bdcd769 bp 0x7fff3d266800 sp 0x7fff3d266580 T0)
    #0 0x7fdc2bdcd768 in isEmpty /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkRasterClip.h:49:16
    #1 0x7fdc2bdcd768 in SkDraw::drawPath(SkPath const&, SkPaint const&, SkMatrix const*, bool, bool, SkBlitter*) const /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkDraw.cpp:1106
    #2 0x7fdc2bbff887 in drawPath /home/worker/workspace/build/src/gfx/skia/skia/include/core/SkDraw.h:59:9
    #3 0x7fdc2bbff887 in GrSWMaskHelper::drawShape(GrShape const&, SkRegion::Op, bool, unsigned char) /home/worker/workspace/build/src/gfx/skia/skia/src/gpu/GrSWMaskHelper.cpp:70
    #4 0x7fdc2bb89dbe in GrClipStackClip::CreateSoftwareClipMask(GrTextureProvider*, GrReducedClip const&) /home/worker/workspace/build/src/gfx/skia/skia/src/gpu/GrClipStackClip.cpp:467:13
    #5 0x7fdc2bb8726a in GrClipStackClip::apply(GrContext*, GrDrawContext*, bool, bool, GrAppliedClip*) const /home/worker/workspace/build/src/gfx/skia/skia/src/gpu/GrClipStackClip.cpp:336:22
    #6 0x7fdc2bb9bf36 in GrDrawTarget::drawBatch(GrPipelineBuilder const&, GrDrawContext*, GrClip const&, GrDrawBatch*) /home/worker/workspace/build/src/gfx/skia/skia/src/gpu/GrDrawTarget.cpp:334:10
    #7 0x7fdc2b92d164 in GrDrawContext::drawFilledRect(GrClip const&, GrPaint const&, SkMatrix const&, SkRect const&, GrUserStencilSettings const*) /home/worker/workspace/build/src/gfx/skia/skia/src/gpu/GrDrawContext.cpp:426:17
...
see log.txt
Flags: in-testsuite?
Reporter

Comment 1

3 years ago
Posted file log.txt
Reporter

Comment 2

3 years ago
Posted file debug_log.txt
Debug builds have a different stack.

Missed an early reject. Bailing on draw from setupDstReadIfNecessary.
/home/worker/workspace/build/src/gfx/skia/skia/src/gpu/GrReducedClip.cpp:93: fatal error: "assert(!fIBounds.isEmpty())"
/home/worker/workspace/build/src/gfx/skia/skia/src/core/SkDraw.cpp:2002: fatal error: "assert(fMatrix != nullptr)"
/home/worker/workspace/build/src/gfx/skia/skia/src/core/SkDraw.cpp:2003: fatal error: "assert(fRC != nullptr)"
ASAN:DEADLYSIGNAL
=================================================================
==18972==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000031 (pc 0x7fa733a7c349 bp 0x7ffc1e8a8c50 sp 0x7ffc1e8a8c50 T0)
    #0 0x7fa733a7c348 in SkRasterClip::getBounds() const /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkRasterClip.cpp:76:12
    #1 0x7fa7338d1e87 in SkDraw::validate() const /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkDraw.cpp:2005:26
    #2 0x7fa7338d739a in SkDraw::drawPath(SkPath const&, SkPaint const&, SkMatrix const*, bool, bool, SkBlitter*) const /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkDraw.cpp:1103:5
    #3 0x7fa73357313b in SkDraw::drawPathCoverage(SkPath const&, SkPaint const&, SkBlitter*) const /home/worker/workspace/build/src/gfx/skia/skia/include/core/SkDraw.h:85:9
    #4 0x7fa73379ca16 in GrSWMaskHelper::drawShape(GrShape const&, SkRegion::Op, bool, unsigned char) /home/worker/workspace/build/src/gfx/skia/skia/src/gpu/GrSWMaskHelper.cpp:66:9
...
Hi Kevin,
Could you reproduce comment 0 or comment 3 with attachment 8804830 [details] ?
Flags: needinfo?(kechen)
Whiteboard: [gfx-noted]
Priority: -- → P1
Assignee

Comment 4

3 years ago
Hello Tyson,

I failed to reproduce the crash, can you give more details about the configuration and STR ?

Thank you.

My configuration:
  ubuntu 14.04 / asan-debug build / rev: 908557c762f798605a2f96e4c943791cbada1b50

My STR:
1. Launch Firefox
2. Open attachment 8804830 [details]
Flags: needinfo?(kechen) → needinfo?(twsmith)
Reporter

Comment 5

3 years ago
Posted file prefs-default.js
Reporter

Comment 6

3 years ago
(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #4)
> Hello Tyson,
> 
> I failed to reproduce the crash, can you give more details about the
> configuration and STR ?
> 
> Thank you.
> 
> My configuration:
>   ubuntu 14.04 / asan-debug build / rev:
> 908557c762f798605a2f96e4c943791cbada1b50
> 
> My STR:
> 1. Launch Firefox
> 2. Open attachment 8804830 [details]

Hi Kevin,

Your STR look good. You will need the attached prefs.js file (sorry I missed this when filing the bug).

Also I verified that it is still reproducible with the latest m-c debug build available on task cluster.
Reporter

Updated

3 years ago
Flags: needinfo?(twsmith)
Assignee

Updated

3 years ago
Assignee: nobody → kechen
Assignee

Comment 7

3 years ago
When converting the SkRect to SkIRect[1] , we will add a value to avoid some limit cases[2]. This makes the original bound with width or height which is smaller than 0.001 treated as an empty bound and run into the assertion.

The easier way to solve this is to assert tighterQuery rather than fIBounds; however I am thinking about blocking these small-width bounds in the first place since it's meaningless to run this function. But I am not sure if this change of code flow will cause any side effects. Still need more investigations.

[1] https://dxr.mozilla.org/mozilla-central/rev/336759fad4621dfcd0a3293840edbed67018accd/gfx/skia/skia/src/gpu/GrReducedClip.cpp#92
[2] https://dxr.mozilla.org/mozilla-central/rev/336759fad4621dfcd0a3293840edbed67018accd/gfx/skia/skia/include/gpu/GrClip.h#94
Assignee

Comment 8

3 years ago
Hello Mason, Can you help me to review the patch ?

The following is the detail of the patch.

In skia clip, kBoundsTolerance[1] is the maximum distance that edges are recognized as two different one.

We don't check this in current code so that a rect with width / height smaller than kBoundsTolerance will run into this assert; therefore, I add a check before constructing this class.


[1] https://dxr.mozilla.org/mozilla-central/rev/05e5b12f41df270b31955ff7e6d09245c1f83a7a/gfx/skia/skia/include/gpu/GrClip.h#57
Attachment #8812104 - Flags: review?(mchang)
Comment on attachment 8812104 [details] [diff] [review]
Make an early return if the clip rect can be ignored.

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

::: gfx/skia/skia/src/utils/SkLua.cpp
@@ +641,5 @@
>  int SkLua::lcanvas_getReducedClipStack(lua_State* L) {
>  #if SK_SUPPORT_GPU
>      const SkCanvas* canvas = get_ref<SkCanvas>(L, 1);
>      SkRect queryBounds = SkRect::Make(canvas->getTopLayerBounds());
> +    if (GrClip::GetPixelIBounds(devBounds).isEmpty()) {

should this be queryBounds instead of devBounds? This seems like this shouldn't even build here.

Are you sure you need this since this is in SK_SUPPORT_GPU? I don't think we have acceleration on linux.
Assignee

Comment 11

3 years ago
Currently we don't even build the code in SkLua due to SK_SUPPORT_GPU tag.
My concern is that we can prevent someone from running into this assertion again if the code is compiled and used sometime in the future.
Or do you think it's more appropriate to not change it so that we won't change the current code flow and cause any unexpected behavior ?
Attachment #8812104 - Attachment is obsolete: true
Attachment #8812104 - Flags: review?(mchang)
Flags: needinfo?(mchang)
Comment on attachment 8812678 [details] [diff] [review]
Make an early return if the clip rect can be ignored.

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

(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #11)
> Created attachment 8812678 [details] [diff] [review]
> Make an early return if the clip rect can be ignored.
> 
> Currently we don't even build the code in SkLua due to SK_SUPPORT_GPU tag.
> My concern is that we can prevent someone from running into this assertion
> again if the code is compiled and used sometime in the future.
> Or do you think it's more appropriate to not change it so that we won't
> change the current code flow and cause any unexpected behavior ?

How about we just assert that the query bounds aren't empty for now instead of changing the behavior?
Attachment #8812678 - Flags: review+
Flags: needinfo?(mchang)
Assignee

Comment 13

3 years ago
Carry r+ from comment 12.
Attachment #8812678 - Attachment is obsolete: true
Attachment #8812998 - Flags: review+
Assignee

Updated

3 years ago
Keywords: checkin-needed
(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #13)
> Created attachment 8812998 [details] [diff] [review]
> Make an early return if the clip rect can be ignored.
> 
> Carry r+ from comment 12.

Please file an upstream Skia bug report for this issue so that we can reduce future patch maintenance.
Flags: needinfo?(kechen)

Comment 15

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba3c76b3359f
Make an early return if the clip rect can be ignored. r=mchang
Keywords: checkin-needed
Assignee

Comment 16

3 years ago
Filed upstream Skia bug report: https://bugs.chromium.org/p/skia/issues/detail?id=5990
Flags: needinfo?(kechen)
Keywords: checkin-needed
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ba3c76b3359f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

3 years ago
Duplicate of this bug: 1319438
Should we uplift to 52? Is 51 affected?
51 doesn't have Skia content, so we should be fine.
Comment on attachment 8812998 [details] [diff] [review]
Make an early return if the clip rect can be ignored.

Approval Request Comment
[Feature/Bug causing the regression]: Skia content on by default
[User impact if declined]: (Uncommon) crashes
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]: Yes, a while now.
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: Low risk.

I wouldn't necessarily ask for the uplift, except that 52 is ESR in the making.
Attachment #8812998 - Flags: approval-mozilla-aurora?
Comment on attachment 8812998 [details] [diff] [review]
Make an early return if the clip rect can be ignored.

fix possible crash in skia, for aurora52
Attachment #8812998 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.