1325518 - SkPath crash viewing PDF using pdf.js

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Jonathan Watt [:jwatt]]

Attachment: PDF testcase

Comment 1

[Ethan Lin[:ethlin]]

I can reproduce the problem on MacOS.

Comment 2

[Ethan Lin[:ethlin]]

I used mozregression to find the regression window.

[Regression range]
Last good revision: 8fcf8b6ea2c9f25198ae807fb1eb0b96ab43c8f4
First bad revision: f092853e564086a98e14f0018ce784bb033e1d85
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8fcf8b6ea2c9f25198ae807fb1eb0b96ab43c8f4&tochange=f092853e564086a98e14f0018ce784bb033e1d85

Comment 3

[Ethan Lin[:ethlin]]

This is the code stack on my device:

* thread #1: tid = 0x1471c9, 0x00000001000097f2 libmozglue.dylib`mozalloc_abort(msg=<unavailable>) + 98 at mozalloc_abort.cpp:33, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001000097f2 libmozglue.dylib`mozalloc_abort(msg=<unavailable>) + 98 at mozalloc_abort.cpp:33
    frame #1: 0x000000010567af8b XUL`sk_abort_no_print() + 43 at SkMemory_mozalloc.cpp:23
    frame #2: 0x00000001056d94ac XUL`SkPath::conservativelyContainsRect(this=<unavailable>, rect=0x00007fff5fbf8158) const + 1580 at SkPath.cpp:278
    frame #3: 0x00000001054ff826 XUL`SkClipStack::Element::contains(this=<unavailable>, rect=0x00007fff5fbf8158) const + 278 at SkClipStack.h:153
    frame #4: 0x0000000105558532 XUL`GrReducedClip::walkStack(this=0x00007fff5fbf8408, stack=<unavailable>, queryBounds=0x00007fff5fbf81f0, maxWindowRectangles=0) + 594 at GrReducedClip.cpp:180
    frame #5: 0x00000001055580af XUL`GrReducedClip::GrReducedClip(this=0x00007fff5fbf8408, stack=0x00000001312ac920, queryBounds=<unavailable>, maxWindowRectangles=0) + 1903 at GrReducedClip.cpp:98
    frame #6: 0x0000000105530c29 XUL`GrClipStackClip::apply(this=0x00000001354a4ad8, context=0x00000001378e36f0, drawContext=0x00000001358faee0, useHWAA=false, hasUserStencilSettings=false, out=0x00007fff5fbf9050) const + 489 at GrClipStackClip.cpp:283
    frame #7: 0x0000000105537cdc XUL`GrDrawTarget::drawBatch(this=0x0000000135ef9000, pipelineBuilder=0x00007fff5fbf9158, drawContext=0x00000001358faee0, clip=0x00000001354a4ad8, batch=0x0000000135da3050) + 588 at GrDrawTarget.cpp:334
    frame #8: 0x000000010543bb85 XUL`GrDrawContext::drawFilledRect(this=0x00000001358faee0, clip=0x00000001354a4ad8, paint=<unavailable>, viewMatrix=<unavailable>, rect=<unavailable>, ss=<unavailable>) + 1109 at GrDrawContext.cpp:426
    frame #9: 0x000000010543a8a5 XUL`GrDrawContext::drawRect(this=0x00000001358faee0, clip=0x00000001354a4ad8, paint=0x00007fff5fbf94b0, viewMatrix=0x0000000135da81d0, rect=0x00007fff5fbf9608, style=<unavailable>) + 2645 at GrDrawContext.cpp:487
    frame #10: 0x0000000105570fdc XUL`SkGpuDevice::drawRect(this=<unavailable>, draw=0x00007fff5fbf9718, rect=0x00007fff5fbf9608, paint=0x00007fff5fbf9970) + 940 at SkGpuDevice.cpp:407
    frame #11: 0x00000001055730cf XUL`SkGpuDevice::drawPath(this=0x00000001354a4a80, draw=0x00007fff5fbf9718, origSrcPath=<unavailable>, paint=0x00007fff5fbf9970, prePathMatrix=0x0000000000000000, pathIsMutable=false) + 319 at SkGpuDevice.cpp:658
    frame #12: 0x00000001054f1162 XUL`SkCanvas::onDrawPath(this=0x0000000135da8000, path=<unavailable>, paint=<unavailable>) + 946 at SkCanvas.cpp:2310
    frame #13: 0x0000000101d741b9 XUL`mozilla::gfx::DrawTargetSkia::Fill(this=<unavailable>, aPath=<unavailable>, aPattern=0x00007fff5fbf9a70, aOptions=<unavailable>) + 217 at DrawTargetSkia.cpp:839
    frame #14: 0x0000000103345cca XUL`mozilla::dom::CanvasRenderingContext2D::Fill(this=0x00000001355a0800, aWinding=<unavailable>) + 490 at CanvasRenderingContext2D.cpp:3172
    frame #15: 0x0000000102ba85e4 XUL`mozilla::dom::CanvasRenderingContext2DBinding::fill(cx=0x0000000111e43000, obj=<unavailable>, self=0x00000001355a0800, args=0x00007fff5fbf9b80) + 740 at CanvasRenderingContext2DBinding.cpp:2821

Comment 4

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Looks like bug 1309913?

Comment 5

[Ethan Lin[:ethlin]]

Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1309913

Comment 6

[Vincent Liu[:vliu]]

(In reply to Ethan Lin[:ethlin] from comment #5)
> Looks like the following bug has the changes which introduced the regression:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1309913
Hi Lee,

I also confirmed that the problem happens since the patches of Bug 1309913 landed. But Bug 1309913 just passed the compositor type to Canvas to make sure SkiaGL canvas was created when the acceleration was reported. It seems that Bug 1309913 exposed this problem but not the root cause.

Also, I tried to remove Bug 1299435 and the problem still exist, so the source code updating to m55 was not the root cause, neither.

When the crash happens in [1], verb contains one kMove_Verb, three kLine_Verb and one kClose_Verb. Do you have any idea why skia had this pattern to hit the crash? Really thanks

[1]: https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/core/SkPath.cpp#278

Comment 7

[Vincent Liu[:vliu]]

(In reply to Vincent Liu[:vliu] from comment #6)

> When the crash happens in [1], verb contains one kMove_Verb, three
> kLine_Verb and one kClose_Verb. Do you have any idea why skia had this
> pattern to hit the crash? Really thanks

Sorry to correct the information. When the crash happens, we had patterns like

kMove_Verb 
kLine_Verb 
kLine_Verb 
kLine_Verb 
kClose_Verb 
kMove_Verb

Apparently it didn't allow kMove_Verb after kLine_Verb or kClose_Verb.

Comment 8

[Lee Salzman [:lsalzman]]

(In reply to Vincent Liu[:vliu] from comment #7)
> (In reply to Vincent Liu[:vliu] from comment #6)
> 
> > When the crash happens in [1], verb contains one kMove_Verb, three
> > kLine_Verb and one kClose_Verb. Do you have any idea why skia had this
> > pattern to hit the crash? Really thanks
> 
> Sorry to correct the information. When the crash happens, we had patterns
> like
> 
> kMove_Verb 
> kLine_Verb 
> kLine_Verb 
> kLine_Verb 
> kClose_Verb 
> kMove_Verb
> 
> Apparently it didn't allow kMove_Verb after kLine_Verb or kClose_Verb.

So the problem here is that when conservativelyContainsRect iterates the path, it doesn't skip degenerate sub-paths. The check at the top should ideally guarantee there are no degenerates: https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/core/SkPath.cpp?q=conservativelyContainsRect&redirect_type=direct#256

If there were (multiple) degenerate sub-paths, then the convexity would be decided as concave, as in here where it determines the path's convexity: https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/core/SkPath.cpp?q=internalGetConvexity&redirect_type=direct#2439

So, in this case it's just a matter of iter.next(pts, true, true) needing to be calling to consume those degenerates rather than simply iter.next(pts).

I can whip up a patch for this.

Comment 9

[Lee Salzman [:lsalzman]]

(In reply to Lee Salzman [:lsalzman] from comment #8)
> (In reply to Vincent Liu[:vliu] from comment #7)
> > (In reply to Vincent Liu[:vliu] from comment #6)
> > 
> > > When the crash happens in [1], verb contains one kMove_Verb, three
> > > kLine_Verb and one kClose_Verb. Do you have any idea why skia had this
> > > pattern to hit the crash? Really thanks
> > 
> > Sorry to correct the information. When the crash happens, we had patterns
> > like
> > 
> > kMove_Verb 
> > kLine_Verb 
> > kLine_Verb 
> > kLine_Verb 
> > kClose_Verb 
> > kMove_Verb
> > 
> > Apparently it didn't allow kMove_Verb after kLine_Verb or kClose_Verb.
> 
> So the problem here is that when conservativelyContainsRect iterates the
> path, it doesn't skip degenerate sub-paths. The check at the top should
> ideally guarantee there are no degenerates:
> https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/core/SkPath.
> cpp?q=conservativelyContainsRect&redirect_type=direct#256
> 
> If there were (multiple) degenerate sub-paths, then the convexity would be
> decided as concave, as in here where it determines the path's convexity:
> https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/core/SkPath.
> cpp?q=internalGetConvexity&redirect_type=direct#2439
> 
> So, in this case it's just a matter of iter.next(pts, true, true) needing to
> be calling to consume those degenerates rather than simply iter.next(pts).
> 
> I can whip up a patch for this.

Actually, upon further inspection, the defaults to iter.next() should actually be consuming generates, but rather just inexactly, which should subsume consuming degenerates exactly. So this degenerate sub-path after the valid one should never be showing up.

The only things I can think of is that something erroneously is setting the path's convexity to convex outside of internalGetConvexity, or there is something in the inexact degenerate check that fails upon some inputs, like maybe NaNs or Infs.

Can you investigate why the path is getting determined to be convex, despite the multiple sub-paths, and/or why iter.next() is not consuming this degenerate path inside conservativelyContainsRect for some reason?

Comment 10

[Lee Salzman [:lsalzman]]

Attachment: make SkPath::conservativelyContainsRect consume degenerate segments

Okay, finally found the issue. SkPath::conservativelyContainsRect was using RawIter, which never consumes degenerate segments. However, internalGetConvexity uses SkPath::Iter, which does consume degenerates. So even though conservativelyContainsRect does check if the path is convex, that did not actually guarantee it would not encounter degenerates unless it also used SkPath::Iter with the same arguments. So thus we need to make it use SkPath::Iter to match up with the guarantee of being convex.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c19ad4565f42
make SkPath::conservativelyContainsRect consume degenerate segments. r=mchang

Comment 12

[Lee Salzman [:lsalzman]]

Upstream Skia bug report: https://skia-review.googlesource.com/c/6900/

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you get a chance.

Comment 15

[Lee Salzman [:lsalzman]]

Comment on attachment 8825525 [details] [diff] [review]
make SkPath::conservativelyContainsRect consume degenerate segments

Approval Request Comment
[Feature/Bug causing the regression]: bug 1309913
[User impact if declined]: pdf.js crashes
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: aurora (52)
[Is the change risky?]: no
[Why is the change risky/not risky?]: Just fixes a containment test inside Skia to work like it was actually supposed to.
[String changes made/needed]: None

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8825525 [details] [diff] [review]
make SkPath::conservativelyContainsRect consume degenerate segments

crash fix for aurora52

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/63378496b740

Comment 18

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Lee's assessment on manual testing needs (see Comment 15) and the fact that this fix has automated coverage.