This bug seems only to repro when I run Firefox under Xvfb. I'm guessing it's just forcing it in to a rendering mode other than default. It would be nice to know how/why to help fuzzing and logging issues in the future. ==6653==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x62e0001bb3c8 at pc 0x7f3f25ca6be5 bp 0x7ffc1585ed40 sp 0x7ffc1585ed38 READ of size 4 at 0x62e0001bb3c8 thread T0 #0 0x7f3f25ca6be4 in SkA8_Shader_Blitter::blitH(int, int, int) /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkBlitter_A8.cpp:265:29 #1 0x7f3f25c964bf in SkBlitter::blitRect(int, int, int, int) /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkBlitter.cpp:70:9 #2 0x7f3f264c7cef in walk_convex_edges(SkEdge*, SkPath::FillType, SkBlitter*, int, int, void (*)(SkBlitter*, int, bool)) /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkScan_Path.cpp:287:17 #3 0x7f3f264bd240 in sk_fill_path(SkPath const&, SkIRect const*, SkBlitter*, int, int, int, SkRegion const&) /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkScan_Path.cpp:505:9 #4 0x7f3f264bc368 in SkScan::FillPath(SkPath const&, SkRegion const&, SkBlitter*) /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkScan_Path.cpp:670:9 #5 0x7f3f264be502 in SkScan::FillPath(SkPath const&, SkRasterClip const&, SkBlitter*) /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkScan_AntiPath.cpp:740:9 #6 0x7f3f2620b0f6 in SkDraw::drawDevPath(SkPath const&, SkPaint const&, bool, SkBlitter*, bool) const /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkDraw.cpp:1097:5 #7 0x7f3f2620bc3b 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:1190:5 #8 0x7f3f2620831f in drawPath /home/worker/workspace/build/src/gfx/skia/skia/include/core/SkDraw.h:54:9 #9 0x7f3f2620831f in SkDraw::drawRect(SkRect const&, SkPaint const&, SkMatrix const*, SkRect const*) const /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkDraw.cpp:825 #10 0x7f3f25f11d12 in SkCanvas::onDrawRect(SkRect const&, SkPaint const&) /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkCanvas.cpp:2165:13 #11 0x7f3f2620cd27 in SkDraw::drawBitmapAsMask(SkBitmap const&, SkPaint const&) const /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkDraw.cpp:1267:13 #12 0x7f3f2620e2d3 in SkDraw::drawBitmap(SkBitmap const&, SkMatrix const&, SkRect const*, SkPaint const&) const /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkDraw.cpp:1346:9 #13 0x7f3f25ee5f3a in SkBitmapDevice::drawBitmap(SkDraw const&, SkBitmap const&, SkMatrix const&, SkPaint const&) /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkBitmapDevice.cpp:233:5 #14 0x7f3f261fb52a in SkBaseDevice::drawImage(SkDraw const&, SkImage const*, float, float, SkPaint const&) /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkDevice.cpp:185:9 #15 0x7f3f25f1948f in SkCanvas::onDrawImage(SkImage const*, float, float, SkPaint const*) /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkCanvas.cpp:2377:13 #16 0x7f3f1e4d9816 in drawImage /home/worker/workspace/build/src/gfx/skia/skia/include/core/SkCanvas.h:816:9 #17 0x7f3f1e4d9816 in mozilla::gfx::DrawTargetSkia::PopLayer() /home/worker/workspace/build/src/gfx/2d/DrawTargetSkia.cpp:2112 #18 0x7f3f1ebbc2d9 in gfxContext::PopGroupAndBlend() /home/worker/workspace/build/src/gfx/thebes/gfxContext.cpp:813:3 #19 0x7f3f238316e5 in nsSVGIntegrationUtils::PaintMaskAndClipPath(nsSVGIntegrationUtils::PaintFramesParams const&) /home/worker/workspace/build/src/layout/svg/nsSVGIntegrationUtils.cpp:1054:5 ... see log.txt

This bug has existed since we added even rounding in bug 996108. The situation is that when the path bounds are rounded to integer values via the normal rounding, they use a more precise rounding that what happens when SK_RASTERIZE_EVEN_ROUNDING generates rounded fixed-point for scan-converted path edges. So when those edge extents later get rounded from fixed-point to integer, they can effectively get bumped up by 1 outside of the bounds computed for the path. This patch biases the rounding of the path bounds so the rounding behaves as if we rounded to FDot6 first then rounded to integer. See comments in patch for more details.

Comment on attachment 8826027 [details] [diff] [review] ensure path bounds after rounding contain path edges when using SK_RASTERIZE_EVEN_ROUNDING Review of attachment 8826027 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/skia/skia/src/core/SkScan_Path.cpp @@ +645,5 @@ > static void round_asymmetric_to_int(const SkRect& src, SkIRect* dst) { > SkASSERT(dst); > dst->set(round_down_to_int(src.fLeft), round_down_to_int(src.fTop), > +#ifdef SK_RASTERIZE_EVEN_ROUNDING > + round_biased_to_int(src.fRight, 0.5f / SK_FDot6One), round_biased_to_int(src.fBottom, 0.5f / SK_FDot6One)); Would it be better to just go from double to FDot6 and then from that to int? That seems easier to verify the correctness of.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #3) > Comment on attachment 8826027 [details] [diff] [review] > ensure path bounds after rounding contain path edges when using > SK_RASTERIZE_EVEN_ROUNDING > > Review of attachment 8826027 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/skia/skia/src/core/SkScan_Path.cpp > @@ +645,5 @@ > > static void round_asymmetric_to_int(const SkRect& src, SkIRect* dst) { > > SkASSERT(dst); > > dst->set(round_down_to_int(src.fLeft), round_down_to_int(src.fTop), > > +#ifdef SK_RASTERIZE_EVEN_ROUNDING > > + round_biased_to_int(src.fRight, 0.5f / SK_FDot6One), round_biased_to_int(src.fBottom, 0.5f / SK_FDot6One)); > > Would it be better to just go from double to FDot6 and then from that to > int? That seems easier to verify the correctness of. Can't in this case because FDot6 is only 32 bits total, giving only 26 bits for the integer portion. the SkIRect generated here can span the whole 32 bit range before later getting clipped to fit in the fixed-point format. This was ultimately a safer way to go about it, I think.

(In reply to Lee Salzman [:lsalzman] from comment #4) > (In reply to Jeff Muizelaar [:jrmuizel] from comment #3) > > Comment on attachment 8826027 [details] [diff] [review] > > ensure path bounds after rounding contain path edges when using > > SK_RASTERIZE_EVEN_ROUNDING > > > > Review of attachment 8826027 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: gfx/skia/skia/src/core/SkScan_Path.cpp > > @@ +645,5 @@ > > > static void round_asymmetric_to_int(const SkRect& src, SkIRect* dst) { > > > SkASSERT(dst); > > > dst->set(round_down_to_int(src.fLeft), round_down_to_int(src.fTop), > > > +#ifdef SK_RASTERIZE_EVEN_ROUNDING > > > + round_biased_to_int(src.fRight, 0.5f / SK_FDot6One), round_biased_to_int(src.fBottom, 0.5f / SK_FDot6One)); > > > > Would it be better to just go from double to FDot6 and then from that to > > int? That seems easier to verify the correctness of. > > Can't in this case because FDot6 is only 32 bits total, giving only 26 bits > for the integer portion. the SkIRect generated here can span the whole 32 > bit range before later getting clipped to fit in the fixed-point format. > This was ultimately a safer way to go about it, I think. Can you add something like this to the comment?

Expanded comment to mention preserving the full integer range rather than losing it with a temporary conversion to FDot6. Carrying r+ from :jrmuizel

Comment on attachment 8826608 [details] [diff] [review] ensure path bounds after rounding contain path edges when using SK_RASTERIZE_EVEN_ROUNDING Approval Request Comment [Feature/Bug causing the regression]: effectively all platforms that we enable Skia content on, from 51+, so: bug 1314090, bug 1278957, bug 725119, bug 1207332 [User impact if declined]: Heap overrun/crash possible on all platforms using Skia content. [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, beta [Is the change risky?]: Not very [Why is the change risky/not risky?]: Just enlarges a clipping region to ensure that paths properly draw inside it, which does not affect rendering results, but does alleviate security issue. [String changes made/needed]: None

Comment on attachment 8826608 [details] [diff] [review] ensure path bounds after rounding contain path edges when using SK_RASTERIZE_EVEN_ROUNDING This issue started with 51 so let's uplift the fix and avoid shipping a security vulnerability to release.

https://hg.mozilla.org/releases/mozilla-aurora/rev/5538b0241b73 https://hg.mozilla.org/releases/mozilla-beta/rev/e309c57e703d