Closed Bug 1313135 Opened 4 years ago Closed 4 years ago

crash near NULL in [@SkDraw::drawPath]

Categories

(Core :: Canvas: 2D, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 --- unaffected
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: tsmith, Assigned: kechen)

References

(Blocks 1 open bug)

Details

(Keywords: crash, csectype-nullptr, testcase, Whiteboard: [gfx-noted])

Attachments

(5 files, 2 obsolete files)

Attached 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?
Attached file log.txt
Attached 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
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)
Attached file prefs-default.js
(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.
Flags: needinfo?(twsmith)
Assignee: nobody → kechen
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
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.
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)
Carry r+ from comment 12.
Attachment #8812678 - Attachment is obsolete: true
Attachment #8812998 - Flags: review+
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)
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
Filed upstream Skia bug report: https://bugs.chromium.org/p/skia/issues/detail?id=5990
Flags: needinfo?(kechen)
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ba3c76b3359f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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.