1165710 - Crash when calling CanvasRenderingContext2D.drawImage many times on OS X.

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Tooru Fujisawa [:arai]]

Attachment: (WARNING) Testcase for the crash

When opening a HTML page which calls CanvasRenderingContext2D.drawImage many times, Nightly crashes.

Attached testcase is almost same as following file, but this one calls drawImage 10000 times.
  https://dxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/655836-1.html

Steps to reproduce:
  1. Run Nightly with clean profile on OS X 10.10.3
  2. Open attached HTML

Expected result:
Red and Green rectangles are shown

Actual result:
Tab crashes

Here is crash report
https://crash-stats.mozilla.com/report/index/55070f03-e2d1-41b4-a3c8-764722150517

Regression range, only bug 857895.
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d11b9d29d909&tochange=e01d80922187

I'm not sure the impact of this bug, so marking as security.

Comment 1

[Tooru Fujisawa [:arai]]

Here is debug log for debug build

Assertion failure: int32_t(mRefCnt) >= 0, at ../../../../dist/include/mozilla/RefPtr.h:110
#01: mozilla::detail::RefCounted<mozilla::gfx::SourceSurface, (mozilla::detail::RefCountAtomicity)0>::AddRef() const[/Users/arai/projects/mozilla-central/obj-system-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x12449d0]
#02: mozilla::RefPtr<mozilla::gfx::SourceSurfaceSkia>::ref(mozilla::gfx::SourceSurfaceSkia*)[/Users/arai/projects/mozilla-central/obj-system-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x14a495c]
#03: mozilla::RefPtr<mozilla::gfx::SourceSurfaceSkia>::RefPtr(mozilla::gfx::SourceSurfaceSkia*)[/Users/arai/projects/mozilla-central/obj-system-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x14a49b1]
#04: mozilla::RefPtr<mozilla::gfx::SourceSurfaceSkia>::RefPtr(mozilla::gfx::SourceSurfaceSkia*)[/Users/arai/projects/mozilla-central/obj-system-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x14886cd]
#05: mozilla::gfx::DrawTargetSkia::Snapshot()[/Users/arai/projects/mozilla-central/obj-system-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x145c162]
#06: mozilla::dom::CanvasRenderingContext2D::GetSurfaceSnapshot(bool*)[/Users/arai/projects/mozilla-central/obj-system-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2ad4ab4]
#07: mozilla::dom::HTMLCanvasElement::GetSurfaceSnapshot(bool*)[/Users/arai/projects/mozilla-central/obj-system-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2d6d7f4]
#08: nsLayoutUtils::SurfaceFromElement(mozilla::dom::HTMLCanvasElement*, unsigned int, mozilla::gfx::DrawTarget*)[/Users/arai/projects/mozilla-central/obj-system-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3e4ed86]
#09: nsLayoutUtils::SurfaceFromElement(mozilla::dom::Element*, unsigned int, mozilla::gfx::DrawTarget*)[/Users/arai/projects/mozilla-central/obj-system-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3e4f330]
#10: mozilla::dom::CanvasRenderingContext2D::DrawImage(mozilla::dom::HTMLImageElementOrHTMLCanvasElementOrHTMLVideoElement const&, double, double, double, double, double, double, double, double, unsigned char, mozilla::ErrorResult&)[/Users/arai/projects/mozilla-central/obj-system-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2ae0710]
#11: mozilla::dom::CanvasRenderingContext2D::DrawImage(mozilla::dom::HTMLImageElementOrHTMLCanvasElementOrHTMLVideoElement const&, double, double, mozilla::ErrorResult&)[/Users/arai/projects/mozilla-central/obj-system-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x23d495a]
#12: mozilla::dom::CanvasRenderingContext2DBinding::drawImage(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, JSJitMethodCallArgs const&)[/Users/arai/projects/mozilla-central/obj-system-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x239eba5]
#13: mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*)[/Users/arai/projects/mozilla-central/obj-system-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2a6186b]
Process 14711 stopped
* thread #1: tid = 0x4d0c12, 0x00000001037c99d4 XUL`mozilla::detail::RefCounted<mozilla::gfx::SourceSurface, (mozilla::detail::RefCountAtomicity)0>::AddRef(this=0x0000000125864d78) const + 100 at RefPtr.h:110, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x00000001037c99d4 XUL`mozilla::detail::RefCounted<mozilla::gfx::SourceSurface, (mozilla::detail::RefCountAtomicity)0>::AddRef(this=0x0000000125864d78) const + 100 at RefPtr.h:110
   107 	  void AddRef() const
   108 	  {
   109 	    // Note: this method must be thread safe for AtomicRefCounted.
-> 110 	    MOZ_ASSERT(int32_t(mRefCnt) >= 0);
   111 	#ifndef MOZ_REFCOUNTED_LEAK_CHECKING
   112 	    ++mRefCnt;
   113 	#else
(lldb) bt
* thread #1: tid = 0x4d0c12, 0x00000001037c99d4 XUL`mozilla::detail::RefCounted<mozilla::gfx::SourceSurface, (mozilla::detail::RefCountAtomicity)0>::AddRef(this=0x0000000125864d78) const + 100 at RefPtr.h:110, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001037c99d4 XUL`mozilla::detail::RefCounted<mozilla::gfx::SourceSurface, (mozilla::detail::RefCountAtomicity)0>::AddRef(this=0x0000000125864d78) const + 100 at RefPtr.h:110
    frame #1: 0x0000000103a2995c XUL`mozilla::RefPtr<mozilla::gfx::SourceSurfaceSkia>::ref(aVal=0x0000000125864d70) + 44 at RefPtr.h:302
    frame #2: 0x0000000103a299b1 XUL`mozilla::RefPtr<mozilla::gfx::SourceSurfaceSkia>::RefPtr(this=0x00007fff5fbf7378, aVal=0x0000000125864d70) + 33 at RefPtr.h:241
    frame #3: 0x0000000103a0d6cd XUL`mozilla::RefPtr<mozilla::gfx::SourceSurfaceSkia>::RefPtr(this=0x00007fff5fbf7378, aVal=0x0000000125864d70) + 29 at RefPtr.h:241
    frame #4: 0x00000001039e1162 XUL`mozilla::gfx::DrawTargetSkia::Snapshot(this=0x0000000132bd7180) + 50 at DrawTargetSkia.cpp:140
    frame #5: 0x0000000105059ab4 XUL`mozilla::dom::CanvasRenderingContext2D::GetSurfaceSnapshot(this=0x0000000129d95800, aPremultAlpha=0x0000000000000000) + 132 at CanvasRenderingContext2D.cpp:1257
    frame #6: 0x00000001052f27f4 XUL`mozilla::dom::HTMLCanvasElement::GetSurfaceSnapshot(this=0x0000000129f541f0, aPremultAlpha=0x0000000000000000) + 132 at HTMLCanvasElement.cpp:987
    frame #7: 0x00000001063d3d86 XUL`nsLayoutUtils::SurfaceFromElement(aElement=0x0000000129f541f0, aSurfaceFlags=18, aTarget=0x0000000132bd7880) + 118 at nsLayoutUtils.cpp:6701
    frame #8: 0x00000001063d4330 XUL`nsLayoutUtils::SurfaceFromElement(aElement=0x0000000129f541f0, aSurfaceFlags=18, aTarget=0x0000000132bd7880) + 80 at nsLayoutUtils.cpp:6791
    frame #9: 0x0000000105065710 XUL`mozilla::dom::CanvasRenderingContext2D::DrawImage(this=0x0000000129d82800, image=0x00007fff5fbf7e80, sx=0, sy=0, sw=0, sh=0, dx=0, dy=0, dw=0, dh=0, optional_argc='\0', error=0x00007fff5fbf7d70) + 2688 at CanvasRenderingContext2D.cpp:4488
    frame #10: 0x000000010495995a XUL`mozilla::dom::CanvasRenderingContext2D::DrawImage(this=0x0000000129d82800, image=0x00007fff5fbf7e80, dx=0, dy=0, error=0x00007fff5fbf7d70) + 90 at CanvasRenderingContext2D.h:217
    frame #11: 0x0000000104923ba5 XUL`mozilla::dom::CanvasRenderingContext2DBinding::drawImage(cx=0x0000000126ad3ff0, obj=Handle<JSObject *> at 0x00007fff5fbf7e40, self=0x0000000129d82800, args=0x00007fff5fbf7f00) + 1205 at CanvasRenderingContext2DBinding.cpp:4121
    frame #12: 0x0000000104fe686b XUL`mozilla::dom::GenericBindingMethod(cx=0x0000000126ad3ff0, argc=3, vp=0x00007fff5fbf8008) + 651 at BindingUtils.cpp:2609
    frame #13: 0x00000001322e0739
(lldb)

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: skia-threadsafe

Comment 3

[Matt Woodrow (:mattwoodrow)]

[Tracking Requested - why for this release]:

Comment 6

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

Comment on attachment 8606830 [details] [diff] [review]
skia-threadsafe

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

I'd like this in 40, don't want to ship a security problem with the release that has Skia on by default.

Comment 7

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8606830 [details] [diff] [review]
skia-threadsafe

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

::: gfx/2d/SourceSurfaceSkia.cpp
@@ +135,3 @@
>      SkBitmap temp = mBitmap;
>      mBitmap.reset();
>      temp.copyTo(&mBitmap, temp.colorType());

We never reset mSharingData to false here. Is this expected? I think it's probably a non issue because MarkChanged() nullptrs mSnapshot in DrawTargetSkia and so multiple draw commands to the DrawTarget shouldn't result in the snapshot being erroneously updated, but it may be worth ensuring that can't be the case here as well.

I'm also not sure "mSharingData" is a descriptive name for what this bool is doing, because that implies our bitmap is being accessed from other objects as well? Perhaps something like mBitmapSnapshotted (I know that's not a very good name either. hrm.), as the way I see it this bool is keeping track of whether we need to do a readback from the DrawTarget and into the SourceSurface's bitmap.

Comment 8

[Matt Woodrow (:mattwoodrow)]

(In reply to George Wright (:gw280) from comment #7)
> Comment on attachment 8606830 [details] [diff] [review]
> skia-threadsafe
> 
> Review of attachment 8606830 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/SourceSurfaceSkia.cpp
> @@ +135,3 @@
> >      SkBitmap temp = mBitmap;
> >      mBitmap.reset();
> >      temp.copyTo(&mBitmap, temp.colorType());
> 
> We never reset mSharingData to false here. Is this expected? I think it's
> probably a non issue because MarkChanged() nullptrs mSnapshot in
> DrawTargetSkia and so multiple draw commands to the DrawTarget shouldn't
> result in the snapshot being erroneously updated, but it may be worth
> ensuring that can't be the case here as well.

Yeah should be safe, but I'll fix this to make it more obvious.

> 
> I'm also not sure "mSharingData" is a descriptive name for what this bool is
> doing, because that implies our bitmap is being accessed from other objects
> as well? Perhaps something like mBitmapSnapshotted (I know that's not a very
> good name either. hrm.), as the way I see it this bool is keeping track of
> whether we need to do a readback from the DrawTarget and into the
> SourceSurface's bitmap.

If the bool is true, then we're sharing the underlying data of the DrawTarget. mUsingSharedData maybe?

(In reply to Milan Sreckovic [:milan] from comment #6)
> Comment on attachment 8606830 [details] [diff] [review]
> skia-threadsafe
> 
> Review of attachment 8606830 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like this in 40, don't want to ship a security problem with the release
> that has Skia on by default.

This is only an issue with bug 857895 which has since been backed out, so I don't think we need worry about this right now.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Sec high, tracking.

Comment 10

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

Comment on attachment 8606830 [details] [diff] [review]
skia-threadsafe

Based on comment 8, retracting aurora approval request.

Comment 11

[Carsten Book [:Tomcat]]

sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=dc683817edec seems one of this changes caused possibly https://treeherder.mozilla.org/logviewer.html#?job_id=10140037&repo=mozilla-inbound or https://treeherder.mozilla.org/logviewer.html#?job_id=10141568&repo=mozilla-inbound

Comment 12

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Emailed Matt about this.

Comment 13

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Oops, I failed to notice that current branches are all unaffected.

Comment 14

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Fixed by backout.

Comment 15

[Vincent Liu[:vliu]]

Hi Matt,

I'd ever encounter a crash on working Bug 801176 with my patches and fixed it by the following WIP.

diff --git a/gfx/2d/DrawTargetSkia.h b/gfx/2d/DrawTargetSkia.h
index 8ed48a7..f2cd507 100644
--- a/gfx/2d/DrawTargetSkia.h
+++ b/gfx/2d/DrawTargetSkia.h
@@ -171,15 +171,15 @@ private:
   std::vector<PushedLayer> mPushedLayers;
 
 #ifdef USE_SKIA_GPU
   RefPtrSkia<GrContext> mGrContext;
 #endif
 
   IntSize mSize;
   RefPtrSkia<SkCanvas> mCanvas;
-  SourceSurfaceSkia* mSnapshot;
+  RefPtr<SourceSurfaceSkia> mSnapshot;
 };
 
 } // namespace gfx
 } // namespace mozilla
 
 #endif // _MOZILLA_GFX_SOURCESURFACESKIA_H

 I found you had ever try to declare mSnapshot as RefPtr in this bug but finally the patch has been backout. I am not sure the actually reason why the backout happens.
 Do you know more about this? Will you keep working on this bug to land this patch? Thanks

Comment 16

[Matt Woodrow (:mattwoodrow)]

I don't remember why it got backed out, sorry.

I wasn't planning on working on it at the moment.

Do you want to push it to try and see if any failures still happen? I can take a lot at the results and try figure it out again.

Comment 17

[Vincent Liu[:vliu]]

Attachment: WIP-1.patch

Hi Matt,
I tried to rebase your patch into my local desktop build. Please see the attached WIP-1.path for detail. 
After applying it, I tried to run |./mach run --debugger lldb|. The thing is browser breaks when I surfed
www.google.com.

Comment 18

[Vincent Liu[:vliu]]

Attachment: break-log.log

The lldb message about Comment 17 said was attached. Could you please have look and see if there is any idea for that? Thanks

Comment 19

[Vincent Liu[:vliu]]

I tried to remove |MarkChanged();| from WIP-1.patch and the issue happens on Comment 17 was disappear.

--- a/gfx/2d/DrawTargetSkia.cpp
+++ b/gfx/2d/DrawTargetSkia.cpp
@@ -121,27 +121,28 @@ GetBitmapForSurface(SourceSurface* aSurface)
 
 DrawTargetSkia::DrawTargetSkia()
   : mSnapshot(nullptr)
 {
 }
 
 DrawTargetSkia::~DrawTargetSkia()
 {
+  //MarkChanged();
 }

Is it reasonable to remove from patch?

Comment 20

[Matt Woodrow (:mattwoodrow)]

Your crash stack is the signal the parent process gets when the child process crashes. You need to attach lldb/gdb to the child process to get the actual crash.

I think removing that line is ok, since SkBitmap reference counts the underlying data itself.

It would be nice to know why it crashes though, since it seems fine.