Bug 1425257: Introduce a global lock to protect the dependency graph between DrawTargets. r=lsalzman
authorBas Schouten <bschouten@mozilla.com>
Tue, 06 Feb 2018 03:08:04 +0100
changeset 751786 e30390f2f53a505a1dc514dad36c181d75e4c83c
parent 751785 0104a3c366e71354eb34fecf1e280c75f2aa276c
child 751787 0790ec12200d5f663dd000a6ef3c72c795ca4ead
child 751839 637dffdef3fb7ae1794f14fabcb95647e0566cc5
push id98047
push userbmo:continuation@gmail.com
push dateTue, 06 Feb 2018 22:02:37 +0000
reviewerslsalzman
bugs1425257
milestone60.0a1
Bug 1425257: Introduce a global lock to protect the dependency graph between DrawTargets. r=lsalzman This patch takes the safest route for securing modifications to the dependency graph for D2D DrawTargets. It's possible a slightly optimal approach is possible, however lock contention should be rare and I believe this is the safest and most upliftable approach. MozReview-Commit-ID: HGfSdEp2U5N
gfx/2d/2D.h
gfx/2d/DrawTargetD2D1.cpp
gfx/2d/DrawTargetD2D1.h
gfx/2d/Factory.cpp
--- a/gfx/2d/2D.h
+++ b/gfx/2d/2D.h
@@ -1750,16 +1750,19 @@ private:
   static StaticRefPtr<ID3D11Device> mD3D11Device;
   static StaticRefPtr<IDWriteFactory> mDWriteFactory;
   static bool mDWriteFactoryInitialized;
 
 protected:
   // This guards access to the singleton devices above, as well as the
   // singleton devices in DrawTargetD2D1.
   static StaticMutex mDeviceLock;
+  // This synchronizes access between different D2D drawtargets and their
+  // implied dependency graph.
+  static StaticMutex mDTDependencyLock;
 
   friend class DrawTargetD2D1;
 #endif
 
 private:
   static DrawEventRecorder *mRecorder;
 };
 
--- a/gfx/2d/DrawTargetD2D1.cpp
+++ b/gfx/2d/DrawTargetD2D1.cpp
@@ -71,26 +71,31 @@ DrawTargetD2D1::~DrawTargetD2D1()
 
   if (mDC && IsDeviceContextValid()) {
     // The only way mDC can be null is if Init failed, but it can happen and the
     // destructor is the only place where we need to check for it since the
     // DrawTarget will destroyed right after Init fails.
     mDC->EndDraw();
   }
 
-  // Targets depending on us can break that dependency, since we're obviously not going to
-  // be modified in the future.
-  for (auto iter = mDependentTargets.begin();
-       iter != mDependentTargets.end(); iter++) {
-    (*iter)->mDependingOnTargets.erase(this);
-  }
-  // Our dependencies on other targets no longer matter.
-  for (TargetSet::iterator iter = mDependingOnTargets.begin();
-       iter != mDependingOnTargets.end(); iter++) {
-    (*iter)->mDependentTargets.erase(this);
+  {
+    // Until this point in the destructor it -must- still be valid for FlushInternal
+    // to be called on this.
+    StaticMutexAutoLock lock(Factory::mDTDependencyLock);
+    // Targets depending on us can break that dependency, since we're obviously not going to
+    // be modified in the future.
+    for (auto iter = mDependentTargets.begin();
+      iter != mDependentTargets.end(); iter++) {
+      (*iter)->mDependingOnTargets.erase(this);
+    }
+    // Our dependencies on other targets no longer matter.
+    for (TargetSet::iterator iter = mDependingOnTargets.begin();
+      iter != mDependingOnTargets.end(); iter++) {
+      (*iter)->mDependentTargets.erase(this);
+    }
   }
 }
 
 already_AddRefed<SourceSurface>
 DrawTargetD2D1::Snapshot()
 {
   MutexAutoLock lock(*mSnapshotLock);
   if (mSnapshot) {
@@ -151,38 +156,17 @@ DrawTargetD2D1::IntoLuminanceSource(Lumi
 // this can cause issues with memory usage (see bug 1238328). EndDraw/BeginDraw
 // are expensive though, especially relatively when little work is done, so
 // we try to reduce the amount of times we execute these purges.
 static const uint32_t kPushedLayersBeforePurge = 25;
 
 void
 DrawTargetD2D1::Flush()
 {
-  if (IsDeviceContextValid()) {
-    if ((mUsedCommandListsSincePurge >= kPushedLayersBeforePurge) &&
-        mPushedLayers.size() == 1) {
-      // It's important to pop all clips as otherwise layers can forget about
-      // their clip when doing an EndDraw. When we have layers pushed we cannot
-      // easily pop all underlying clips to delay the purge until we have no
-      // layers pushed.
-      PopAllClips();
-      mUsedCommandListsSincePurge = 0;
-      mDC->EndDraw();
-      mDC->BeginDraw();
-    } else {
-      mDC->Flush();
-    }
-  }
-
-  // We no longer depend on any target.
-  for (TargetSet::iterator iter = mDependingOnTargets.begin();
-       iter != mDependingOnTargets.end(); iter++) {
-    (*iter)->mDependentTargets.erase(this);
-  }
-  mDependingOnTargets.clear();
+  FlushInternal();
 }
 
 void
 DrawTargetD2D1::DrawSurface(SourceSurface *aSurface,
                             const Rect &aDest,
                             const Rect &aSource,
                             const DrawSurfaceOptions &aSurfOptions,
                             const DrawOptions &aOptions)
@@ -1267,38 +1251,77 @@ DrawTargetD2D1::CleanupD2D()
   if (mFactory) {
     RadialGradientEffectD2D1::Unregister(mFactory);
     ExtendInputEffectD2D1::Unregister(mFactory);
     mFactory = nullptr;
   }
 }
 
 void
+DrawTargetD2D1::FlushInternal(bool aHasDependencyMutex /* = false */)
+{
+  if (IsDeviceContextValid()) {
+    if ((mUsedCommandListsSincePurge >= kPushedLayersBeforePurge) &&
+      mPushedLayers.size() == 1) {
+      // It's important to pop all clips as otherwise layers can forget about
+      // their clip when doing an EndDraw. When we have layers pushed we cannot
+      // easily pop all underlying clips to delay the purge until we have no
+      // layers pushed.
+      PopAllClips();
+      mUsedCommandListsSincePurge = 0;
+      mDC->EndDraw();
+      mDC->BeginDraw();
+    }
+    else {
+      mDC->Flush();
+    }
+  }
+
+  Maybe<StaticMutexAutoLock> lock;
+
+  if (!aHasDependencyMutex) {
+    lock.emplace(Factory::mDTDependencyLock);
+  }
+
+  Factory::mDTDependencyLock.AssertCurrentThreadOwns();
+  // We no longer depend on any target.
+  for (TargetSet::iterator iter = mDependingOnTargets.begin();
+    iter != mDependingOnTargets.end(); iter++) {
+    (*iter)->mDependentTargets.erase(this);
+  }
+  mDependingOnTargets.clear();
+}
+
+void
 DrawTargetD2D1::MarkChanged()
 {
   if (mSnapshot) {
     MutexAutoLock lock(*mSnapshotLock);
     if (mSnapshot->hasOneRef()) {
       // Just destroy it, since no-one else knows about it.
       mSnapshot = nullptr;
     } else {
       mSnapshot->DrawTargetWillChange();
       // The snapshot will no longer depend on this target.
       MOZ_ASSERT(!mSnapshot);
     }
   }
-  if (mDependentTargets.size()) {
-    // Copy mDependentTargets since the Flush()es below will modify it.
-    TargetSet tmpTargets = mDependentTargets;
-    for (TargetSet::iterator iter = tmpTargets.begin();
-         iter != tmpTargets.end(); iter++) {
-      (*iter)->Flush();
+
+  {
+    StaticMutexAutoLock lock(Factory::mDTDependencyLock);
+    if (mDependentTargets.size()) {
+      // Copy mDependentTargets since the Flush()es below will modify it.
+      TargetSet tmpTargets = mDependentTargets;
+      for (TargetSet::iterator iter = tmpTargets.begin();
+        iter != tmpTargets.end(); iter++) {
+        (*iter)->FlushInternal(true);
+      }
+      // The Flush() should have broken all dependencies on this target.
+      MOZ_ASSERT(!mDependentTargets.size());
     }
-    // The Flush() should have broken all dependencies on this target.
-    MOZ_ASSERT(!mDependentTargets.size());
   }
 }
 
 bool
 DrawTargetD2D1::ShouldClipTemporarySurfaceDrawing(CompositionOp aOp,
                                                   const Pattern& aPattern,
                                                   bool aClipIsComplex)
 {
@@ -1459,19 +1482,28 @@ DrawTargetD2D1::FinalizeDrawing(Composit
   radialGradientEffect->SetInput(0, source);
 
   mDC->DrawImage(radialGradientEffect, D2D1_INTERPOLATION_MODE_NEAREST_NEIGHBOR, D2DCompositionMode(aOp));
 }
 
 void
 DrawTargetD2D1::AddDependencyOnSource(SourceSurfaceD2D1* aSource)
 {
-  if (aSource->mDrawTarget && !mDependingOnTargets.count(aSource->mDrawTarget)) {
-    aSource->mDrawTarget->mDependentTargets.insert(this);
-    mDependingOnTargets.insert(aSource->mDrawTarget);
+  Maybe<MutexAutoLock> snapshotLock;
+  // We grab the SnapshotLock as well, this guaranteeds aSource->mDrawTarget
+  // cannot be cleared in between the if statement and the dereference.
+  if (aSource->mSnapshotLock) {
+    snapshotLock.emplace(*aSource->mSnapshotLock);
+  }
+  {
+    StaticMutexAutoLock lock(Factory::mDTDependencyLock);
+    if (aSource->mDrawTarget && !mDependingOnTargets.count(aSource->mDrawTarget)) {
+      aSource->mDrawTarget->mDependentTargets.insert(this);
+      mDependingOnTargets.insert(aSource->mDrawTarget);
+    }
   }
 }
 
 static D2D1_RECT_F
 IntersectRect(const D2D1_RECT_F& aRect1, const D2D1_RECT_F& aRect2)
 {
   D2D1_RECT_F result;
   result.left = max(aRect1.left, aRect2.left);
--- a/gfx/2d/DrawTargetD2D1.h
+++ b/gfx/2d/DrawTargetD2D1.h
@@ -168,16 +168,18 @@ public:
   }
 
   static uint64_t mVRAMUsageDT;
   static uint64_t mVRAMUsageSS;
 
 private:
   friend class SourceSurfaceD2D1;
 
+  void FlushInternal(bool aHasDependencyMutex = false);
+
   typedef std::unordered_set<DrawTargetD2D1*> TargetSet;
 
   // This function will mark the surface as changing, and make sure any
   // copy-on-write snapshots are notified.
   void MarkChanged();
   bool ShouldClipTemporarySurfaceDrawing(CompositionOp aOp, const Pattern& aPattern, bool aClipIsComplex);
   void PrepareForDrawing(CompositionOp aOp, const Pattern &aPattern);
   void FinalizeDrawing(CompositionOp aOp, const Pattern &aPattern);
--- a/gfx/2d/Factory.cpp
+++ b/gfx/2d/Factory.cpp
@@ -217,16 +217,17 @@ Mutex* Factory::mFTLock = nullptr;
 #ifdef WIN32
 // Note: mDeviceLock must be held when mutating these values.
 static uint32_t mDeviceSeq = 0;
 StaticRefPtr<ID3D11Device> Factory::mD3D11Device;
 StaticRefPtr<ID2D1Device> Factory::mD2D1Device;
 StaticRefPtr<IDWriteFactory> Factory::mDWriteFactory;
 bool Factory::mDWriteFactoryInitialized = false;
 StaticMutex Factory::mDeviceLock;
+StaticMutex Factory::mDTDependencyLock;
 #endif
 
 DrawEventRecorder *Factory::mRecorder;
 
 mozilla::gfx::Config* Factory::sConfig = nullptr;
 
 void
 Factory::Init(const Config& aConfig)