Bug 1414448: Protect the SourceSurfaceSkia destructor from racing with DrawTargetWillChange. r=dvander
authorBas Schouten <bschouten@mozilla.com>
Wed, 08 Nov 2017 20:49:40 +0100
changeset 444098 44e87c9281a433d7204c7dd8d24dc226edf8fc72
parent 444097 b592e6f5ac17d2868485016fd617d20fd834b50b
child 444099 76f2e2460674d6aef1804160b6a2cecd6a5a10ec
push id1618
push userCallek@gmail.com
push dateThu, 11 Jan 2018 17:45:48 +0000
treeherdermozilla-release@882ca853e05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdvander
bugs1414448
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1414448: Protect the SourceSurfaceSkia destructor from racing with DrawTargetWillChange. r=dvander MozReview-Commit-ID: 6UYDLUEoJZy
gfx/2d/DrawTargetSkia.cpp
gfx/2d/DrawTargetSkia.h
gfx/2d/SourceSurfaceSkia.cpp
gfx/2d/SourceSurfaceSkia.h
--- a/gfx/2d/DrawTargetSkia.cpp
+++ b/gfx/2d/DrawTargetSkia.cpp
@@ -45,16 +45,18 @@
 #include "ScaledFontMac.h"
 #include "CGTextDrawing.h"
 #endif
 
 #ifdef XP_WIN
 #include "ScaledFontDWrite.h"
 #endif
 
+using namespace std;
+
 namespace mozilla {
 namespace gfx {
 
 class GradientStopsSkia : public GradientStops
 {
 public:
   MOZ_DECLARE_REFCOUNTED_VIRTUAL_TYPENAME(GradientStopsSkia)
   GradientStopsSkia(const std::vector<GradientStop>& aStops, uint32_t aNumStops, ExtendMode aExtendMode)
@@ -271,16 +273,17 @@ GetSkImageForSurface(SourceSurface* aSur
   MOZ_ASSERT(VerifyRGBXCorners(surf->GetData(), surf->GetSize(),
                                surf->Stride(), surf->GetFormat(),
                                aBounds, aMatrix));
   return image;
 }
 
 DrawTargetSkia::DrawTargetSkia()
   : mSnapshot(nullptr)
+  , mSnapshotLock{make_shared<Mutex>("DrawTargetSkia::mSnapshotLock")}
 #ifdef MOZ_WIDGET_COCOA
   , mCG(nullptr)
   , mColorSpace(nullptr)
   , mCanvasData(nullptr)
   , mCGSize(0, 0)
   , mNeedLayer(false)
 #endif
 {
@@ -311,17 +314,17 @@ DrawTargetSkia::Snapshot()
     // If the surface is raster, making a snapshot may trigger a pixel copy.
     // Instead, try to directly make a raster image referencing the surface pixels.
     SkPixmap pixmap;
     if (mSurface->peekPixels(&pixmap)) {
       image = SkImage::MakeFromRaster(pixmap, nullptr, nullptr);
     } else {
       image = mSurface->makeImageSnapshot();
     }
-    if (!snapshot->InitFromImage(image, mFormat, this)) {
+    if (!snapshot->InitFromImage(image, mFormat, this, mSnapshotLock)) {
       return nullptr;
     }
     mSnapshot = snapshot;
   }
 
   return snapshot.forget();
 }
 
@@ -2209,16 +2212,17 @@ already_AddRefed<FilterNode>
 DrawTargetSkia::CreateFilter(FilterType aType)
 {
   return FilterNodeSoftware::Create(aType);
 }
 
 void
 DrawTargetSkia::MarkChanged()
 {
+  MutexAutoLock lock(*mSnapshotLock);
   if (mSnapshot) {
     mSnapshot->DrawTargetWillChange();
     mSnapshot = nullptr;
 
     // Handle copying of any image snapshots bound to the surface.
     if (mSurface) {
       mSurface->notifyContentWillChange(SkSurface::kRetain_ContentChangeMode);
     }
--- a/gfx/2d/DrawTargetSkia.h
+++ b/gfx/2d/DrawTargetSkia.h
@@ -201,16 +201,17 @@ private:
 #ifdef USE_SKIA_GPU
   sk_sp<GrContext> mGrContext;
 #endif
 
   IntSize mSize;
   sk_sp<SkSurface> mSurface;
   SkCanvas* mCanvas;
   SourceSurfaceSkia* mSnapshot;
+  std::shared_ptr<Mutex> mSnapshotLock;
 
 #ifdef MOZ_WIDGET_COCOA
   friend class BorrowedCGContext;
 
   CGContextRef BorrowCGContext(const DrawOptions &aOptions);
   void ReturnCGContext(CGContextRef);
   bool FillGlyphsWithCG(ScaledFont* aFont,
                         const GlyphBuffer& aBuffer,
--- a/gfx/2d/SourceSurfaceSkia.cpp
+++ b/gfx/2d/SourceSurfaceSkia.cpp
@@ -8,30 +8,35 @@
 #include "Logging.h"
 #include "SourceSurfaceSkia.h"
 #include "HelpersSkia.h"
 #include "DrawTargetSkia.h"
 #include "DataSurfaceHelpers.h"
 #include "skia/include/core/SkData.h"
 #include "mozilla/CheckedInt.h"
 
+using namespace std;
+
 namespace mozilla {
 namespace gfx {
 
 SourceSurfaceSkia::SourceSurfaceSkia()
   : mDrawTarget(nullptr)
   , mChangeMutex("SourceSurfaceSkia::mChangeMutex")
 {
 }
 
 SourceSurfaceSkia::~SourceSurfaceSkia()
 {
-  if (mDrawTarget) {
-    mDrawTarget->SnapshotDestroyed();
-    mDrawTarget = nullptr;
+  if (mSnapshotLock) {
+    MutexAutoLock lock{*mSnapshotLock};
+    if (mDrawTarget) {
+      mDrawTarget->SnapshotDestroyed();
+      mDrawTarget = nullptr;
+    }
   }
 }
 
 IntSize
 SourceSurfaceSkia::GetSize() const
 {
   return mSize;
 }
@@ -99,17 +104,18 @@ SourceSurfaceSkia::InitFromData(unsigned
   mFormat = aFormat;
   mStride = aStride;
   return true;
 }
 
 bool
 SourceSurfaceSkia::InitFromImage(const sk_sp<SkImage>& aImage,
                                  SurfaceFormat aFormat,
-                                 DrawTargetSkia* aOwner)
+                                 DrawTargetSkia* aOwner,
+                                 shared_ptr<Mutex> aSnapshotLock)
 {
   if (!aImage) {
     return false;
   }
 
   mSize = IntSize(aImage->width(), aImage->height());
 
   // For the raster image case, we want to use the format and stride
@@ -132,16 +138,18 @@ SourceSurfaceSkia::InitFromImage(const s
     mStride = SkAlign4(info.minRowBytes());
   } else {
     return false;
   }
 
   mImage = aImage;
 
   if (aOwner) {
+    MOZ_ASSERT(aSnapshotLock);
+    mSnapshotLock = move(aSnapshotLock);
     mDrawTarget = aOwner;
   }
 
   return true;
 }
 
 uint8_t*
 SourceSurfaceSkia::GetData()
@@ -181,16 +189,20 @@ SourceSurfaceSkia::Unmap()
   mChangeMutex.Unlock();
   MOZ_ASSERT(mIsMapped);
   mIsMapped = false;
 }
 
 void
 SourceSurfaceSkia::DrawTargetWillChange()
 {
+  // In this case synchronisation on destroy should be guaranteed!
+  MOZ_ASSERT(mSnapshotLock);
+  mSnapshotLock->AssertCurrentThreadOwns();
+
   MutexAutoLock lock(mChangeMutex);
   if (mDrawTarget) {
     // Raster snapshots do not use Skia's internal copy-on-write mechanism,
     // so we need to do an explicit copy here.
     // GPU snapshots, for which peekPixels is false, will already be dealt
     // with automatically via the internal copy-on-write mechanism, so we
     // don't need to do anything for them here.
     SkPixmap pixmap;
--- a/gfx/2d/SourceSurfaceSkia.h
+++ b/gfx/2d/SourceSurfaceSkia.h
@@ -13,16 +13,17 @@
 #include "skia/include/core/SkCanvas.h"
 #include "skia/include/core/SkImage.h"
 
 namespace mozilla {
 
 namespace gfx {
 
 class DrawTargetSkia;
+class SnapshotLock;
 
 class SourceSurfaceSkia : public DataSourceSurface
 {
 public:
   MOZ_DECLARE_REFCOUNTED_VIRTUAL_TYPENAME(DataSourceSurfaceSkia)
   SourceSurfaceSkia();
   ~SourceSurfaceSkia();
 
@@ -34,17 +35,18 @@ public:
 
   bool InitFromData(unsigned char* aData,
                     const IntSize &aSize,
                     int32_t aStride,
                     SurfaceFormat aFormat);
 
   bool InitFromImage(const sk_sp<SkImage>& aImage,
                      SurfaceFormat aFormat = SurfaceFormat::UNKNOWN,
-                     DrawTargetSkia* aOwner = nullptr);
+                     DrawTargetSkia* aOwner = nullptr,
+                     std::shared_ptr<Mutex> aSnapshotLock = std::shared_ptr<Mutex>{});
 
   virtual uint8_t* GetData();
 
   /**
    * The caller is responsible for ensuring aMappedSurface is not null.
    */
   virtual bool Map(MapType, MappedSurface *aMappedSurface);
 
@@ -57,15 +59,16 @@ private:
 
   void DrawTargetWillChange();
 
   sk_sp<SkImage> mImage;
   SurfaceFormat mFormat;
   IntSize mSize;
   int32_t mStride;
   RefPtr<DrawTargetSkia> mDrawTarget;
+  std::shared_ptr<Mutex> mSnapshotLock;
   Mutex mChangeMutex;
 };
 
 } // namespace gfx
 } // namespace mozilla
 
 #endif /* MOZILLA_GFX_SOURCESURFACESKIA_H_ */