Bug 1416862: Reverse DrawTargetSkia snapshot ownership model r=dvander

MozReview-Commit-ID: 3hpeYteEPlA

gfx/2d/DrawTargetSkia.cpp

@@ -278,7 +278,7 @@ GetSkImageForSurface(SourceSurface* aSurface, const Rect* aBounds = nullptr, con
 
 DrawTargetSkia::DrawTargetSkia()
   : mSnapshot(nullptr)
-  , mSnapshotLock{make_shared<Mutex>("DrawTargetSkia::mSnapshotLock")}
+  , mSnapshotLock{"DrawTargetSkia::mSnapshotLock"}
 #ifdef MOZ_WIDGET_COCOA
   , mCG(nullptr)
   , mColorSpace(nullptr)
@@ -291,6 +291,12 @@ DrawTargetSkia::DrawTargetSkia()
 
 DrawTargetSkia::~DrawTargetSkia()
 {
+  if (mSnapshot) {
+    MutexAutoLock lock(mSnapshotLock);
+    // We're going to go away, hand our SkSurface to the SourceSurface.
+    mSnapshot->GiveSurface(mSurface);
+  }
+
 #ifdef MOZ_WIDGET_COCOA
   if (mCG) {
     CGContextRelease(mCG);
@@ -309,7 +315,7 @@ DrawTargetSkia::Snapshot()
 {
   // Without this lock, this could cause us to get out a snapshot and race with
   // Snapshot::~Snapshot() actually destroying itself.
-  MutexAutoLock lock(*mSnapshotLock);
+  MutexAutoLock lock(mSnapshotLock);
   RefPtr<SourceSurfaceSkia> snapshot = mSnapshot;
   if (mSurface && !snapshot) {
     snapshot = new SourceSurfaceSkia();
@@ -322,7 +328,7 @@ DrawTargetSkia::Snapshot()
     } else {
       image = mSurface->makeImageSnapshot();
     }
-    if (!snapshot->InitFromImage(image, mFormat, this, mSnapshotLock)) {
+    if (!snapshot->InitFromImage(image, mFormat, this)) {
       return nullptr;
     }
     mSnapshot = snapshot;
@@ -2226,8 +2232,17 @@ DrawTargetSkia::CreateFilter(FilterType aType)
 void
 DrawTargetSkia::MarkChanged()
 {
-  MutexAutoLock lock(*mSnapshotLock);
+  // I'm not entirely certain whether this lock is needed, as multiple threads
+  // should never modify the DrawTarget at the same time anyway, but this seems
+  // like the safest.
+  MutexAutoLock lock(mSnapshotLock);
   if (mSnapshot) {
+    if (mSnapshot->hasOneRef()) {
+      // No owners outside of this DrawTarget's own reference. Just dump it.
+      mSnapshot = nullptr;
+      return;
+    }
+
     mSnapshot->DrawTargetWillChange();
     mSnapshot = nullptr;
 
@@ -2238,11 +2253,5 @@ DrawTargetSkia::MarkChanged()
   }
 }
 
-void
-DrawTargetSkia::SnapshotDestroyed()
-{
-  mSnapshot = nullptr;
-}
-
 } // namespace gfx
 } // namespace mozilla

gfx/2d/DrawTargetSkia.h

@@ -160,7 +160,6 @@ public:
 
 private:
   friend class SourceSurfaceSkia;
-  void SnapshotDestroyed();
 
   void MarkChanged();
 
@@ -205,8 +204,8 @@ private:
   IntSize mSize;
   sk_sp<SkSurface> mSurface;
   SkCanvas* mCanvas;
-  SourceSurfaceSkia* mSnapshot;
-  std::shared_ptr<Mutex> mSnapshotLock;
+  RefPtr<SourceSurfaceSkia> mSnapshot;
+  Mutex mSnapshotLock;
 
 #ifdef MOZ_WIDGET_COCOA
   friend class BorrowedCGContext;

gfx/2d/SourceSurfaceSkia.cpp

@@ -26,13 +26,6 @@ SourceSurfaceSkia::SourceSurfaceSkia()
 
 SourceSurfaceSkia::~SourceSurfaceSkia()
 {
-  if (mSnapshotLock) {
-    MutexAutoLock lock{*mSnapshotLock};
-    if (mDrawTarget) {
-      mDrawTarget->SnapshotDestroyed();
-      mDrawTarget = nullptr;
-    }
-  }
 }
 
 IntSize
@@ -109,8 +102,7 @@ SourceSurfaceSkia::InitFromData(unsigned char* aData,
 bool
 SourceSurfaceSkia::InitFromImage(const sk_sp<SkImage>& aImage,
                                  SurfaceFormat aFormat,
-                                 DrawTargetSkia* aOwner,
-                                 shared_ptr<Mutex> aSnapshotLock)
+                                 DrawTargetSkia* aOwner)
 {
   if (!aImage) {
     return false;
@@ -143,8 +135,6 @@ SourceSurfaceSkia::InitFromImage(const sk_sp<SkImage>& aImage,
   mImage = aImage;
 
   if (aOwner) {
-    MOZ_ASSERT(aSnapshotLock);
-    mSnapshotLock = move(aSnapshotLock);
     mDrawTarget = aOwner;
   }
 
@@ -194,10 +184,6 @@ SourceSurfaceSkia::Unmap()
 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,

gfx/2d/SourceSurfaceSkia.h

@@ -31,6 +31,12 @@ public:
   virtual IntSize GetSize() const;
   virtual SurfaceFormat GetFormat() const;
 
+  // This is only ever called by the DT destructor, which can only ever happen
+  // from one place at a time. Therefore it doesn't need to hold the ChangeMutex
+  // as mSurface is never read to directly and is just there to keep the object
+  // alive, which itself is refcounted in a thread-safe manner.
+  void GiveSurface(sk_sp<SkSurface> &aSurface) { mSurface = aSurface; mDrawTarget = nullptr; }
+
   sk_sp<SkImage> GetImage();
 
   bool InitFromData(unsigned char* aData,
@@ -40,8 +46,7 @@ public:
 
   bool InitFromImage(const sk_sp<SkImage>& aImage,
                      SurfaceFormat aFormat = SurfaceFormat::UNKNOWN,
-                     DrawTargetSkia* aOwner = nullptr,
-                     std::shared_ptr<Mutex> aSnapshotLock = std::shared_ptr<Mutex>{});
+                     DrawTargetSkia* aOwner = nullptr);
 
   virtual uint8_t* GetData();
 
@@ -60,11 +65,12 @@ private:
   void DrawTargetWillChange();
 
   sk_sp<SkImage> mImage;
+  // This keeps a surface alive if needed because its DrawTarget has gone away.
+  sk_sp<SkSurface> mSurface;
   SurfaceFormat mFormat;
   IntSize mSize;
   int32_t mStride;
-  RefPtr<DrawTargetSkia> mDrawTarget;
-  std::shared_ptr<Mutex> mSnapshotLock;
+  DrawTargetSkia* mDrawTarget;
   Mutex mChangeMutex;
 };