Closed Bug 1165710 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: Canvas: 2D, defect)

Unspecified
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox40 + unaffected
firefox41 + unaffected
firefox-esr38 --- unaffected

People

(Reporter: arai, Assigned: mattwoodrow)

References

Details

(Keywords: csectype-race, sec-high, Whiteboard: [b2g-adv-main2.5-])

Crash Data

Attachments

(4 files)

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.
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)
Attached patch skia-threadsafeSplinter Review
Assignee: nobody → matt.woodrow
Attachment #8606830 - Flags: review?(gwright)
[Tracking Requested - why for this release]:
Duplicate of this bug: 1165799
Duplicate of this bug: 1166081
Crash Signature: mozilla::gfx::DrawTargetCaptureImpl::DrawSurface(mozilla::gfx::SourceSurface*, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::DrawSurfaceOptions const&, mozilla::gfx... → [@ SkPixelRef::unlockPixels()] mozilla::gfx::DrawTargetCaptureImpl::DrawSurface(mozilla::gfx::SourceSurface*, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::DrawSurfac…
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.
Attachment #8606830 - Flags: approval-mozilla-aurora?
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.
Attachment #8606830 - Flags: review?(gwright) → review+
(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.
Sec high, tracking.
Comment on attachment 8606830 [details] [diff] [review]
skia-threadsafe

Based on comment 8, retracting aurora approval request.
Attachment #8606830 - Flags: approval-mozilla-aurora?
OS: Unspecified → Mac OS X
Emailed Matt about this.
Oops, I failed to notice that current branches are all unaffected.
Fixed by backout.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Group: core-security → core-security-release
Flags: needinfo?(matt.woodrow)
Whiteboard: [b2g-adv-main2.5-]
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
Flags: needinfo?(matt.woodrow)
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.
Flags: needinfo?(matt.woodrow)
Attached patch WIP-1.patchSplinter Review
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.
Attached file 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
Flags: needinfo?(matt.woodrow)
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?
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(matt.woodrow)
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.
Flags: needinfo?(matt.woodrow)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.