1313135 - crash near NULL in [@SkDraw::drawPath]

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect, P1)

Description

[Tyson Smith [:tsmith]]

Attachment: 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

Comment 1

[Tyson Smith [:tsmith]]

Attachment: log.txt

Comment 2

[Tyson Smith [:tsmith]]

Attachment: 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
...

Comment 3

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Hi Kevin,
Could you reproduce comment 0 or comment 3 with attachment 8804830 [details] ?

Comment 4

[Kevin Chen[:kechen] (UTC + 8)]

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]

Comment 5

[Tyson Smith [:tsmith]]

Attachment: prefs-default.js

Comment 6

[Tyson Smith [:tsmith]]

(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.

Comment 7

[Kevin Chen[:kechen] (UTC + 8)]

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

Comment 8

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Make an early return if the clip rect can be ignored.

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

Comment 9

[Kevin Chen[:kechen] (UTC + 8)]

Try run looks good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=05a3c0c069eff3e88de2339266411c52da0db83c

Comment 10

[Mason Chang [Inactive] [: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.

Comment 11

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: 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 ?

Comment 12

[Mason Chang [Inactive] [: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?

Comment 13

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Make an early return if the clip rect can be ignored.

Carry r+ from comment 12.

Comment 14

[Lee Salzman [:lsalzman]]

(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.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba3c76b3359f
Make an early return if the clip rect can be ignored. r=mchang

Comment 16

[Kevin Chen[:kechen] (UTC + 8)]

Filed upstream Skia bug report: https://bugs.chromium.org/p/skia/issues/detail?id=5990

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 19

[Marco Castelluccio [:marco]]

Should we uplift to 52? Is 51 affected?

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

51 doesn't have Skia content, so we should be fine.

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 22

[Julien Cristau [:jcristau]]

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

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a88789dae8ee