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

@@ -1755,6 +1755,9 @@ 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

gfx/2d/DrawTargetD2D1.cpp

@@ -76,16 +76,21 @@ DrawTargetD2D1::~DrawTargetD2D1()
     mDC->EndDraw();
   }
 
-  // Targets depending on us can break that dependency, since we're obviously not going to
+  {
-  // be modified in the future.
+    // Until this point in the destructor it -must- still be valid for FlushInternal
-  for (auto iter = mDependentTargets.begin();
+    // to be called on this.
-       iter != mDependentTargets.end(); iter++) {
+    StaticMutexAutoLock lock(Factory::mDTDependencyLock);
-    (*iter)->mDependingOnTargets.erase(this);
+    // Targets depending on us can break that dependency, since we're obviously not going to
-  }
+    // be modified in the future.
-  // Our dependencies on other targets no longer matter.
+    for (auto iter = mDependentTargets.begin();
-  for (TargetSet::iterator iter = mDependingOnTargets.begin();
+      iter != mDependentTargets.end(); iter++) {
-       iter != mDependingOnTargets.end(); iter++) {
+      (*iter)->mDependingOnTargets.erase(this);
-    (*iter)->mDependentTargets.erase(this);
+    }
+    // Our dependencies on other targets no longer matter.
+    for (TargetSet::iterator iter = mDependingOnTargets.begin();
+      iter != mDependingOnTargets.end(); iter++) {
+      (*iter)->mDependentTargets.erase(this);
+    }
   }
 }
 
@@ -156,28 +161,7 @@ static const uint32_t kPushedLayersBeforePurge = 25;
 void
 DrawTargetD2D1::Flush()
 {
-  if (IsDeviceContextValid()) {
+  FlushInternal();
-    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();
 }
 
 void
@@ -1271,6 +1255,41 @@ DrawTargetD2D1::CleanupD2D()
   }
 }
 
+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()
 {
@@ -1285,15 +1304,19 @@ DrawTargetD2D1::MarkChanged()
       MOZ_ASSERT(!mSnapshot);
     }
   }
-  if (mDependentTargets.size()) {
+
-    // Copy mDependentTargets since the Flush()es below will modify it.
+  {
-    TargetSet tmpTargets = mDependentTargets;
+    StaticMutexAutoLock lock(Factory::mDTDependencyLock);
-    for (TargetSet::iterator iter = tmpTargets.begin();
+    if (mDependentTargets.size()) {
-         iter != tmpTargets.end(); iter++) {
+      // Copy mDependentTargets since the Flush()es below will modify it.
-      (*iter)->Flush();
+      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());
   }
 }
 
@@ -1464,9 +1487,18 @@ DrawTargetD2D1::FinalizeDrawing(CompositionOp aOp, const Pattern &aPattern)
 void
 DrawTargetD2D1::AddDependencyOnSource(SourceSurfaceD2D1* aSource)
 {
-  if (aSource->mDrawTarget && !mDependingOnTargets.count(aSource->mDrawTarget)) {
+  Maybe<MutexAutoLock> snapshotLock;
-    aSource->mDrawTarget->mDependentTargets.insert(this);
+  // We grab the SnapshotLock as well, this guaranteeds aSource->mDrawTarget
-    mDependingOnTargets.insert(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);
+    }
   }
 }
 

gfx/2d/DrawTargetD2D1.h

@@ -173,6 +173,8 @@ public:
 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

gfx/2d/Factory.cpp

@@ -222,6 +222,7 @@ StaticRefPtr<ID2D1Device> Factory::mD2D1Device;
 StaticRefPtr<IDWriteFactory> Factory::mDWriteFactory;
 bool Factory::mDWriteFactoryInitialized = false;
 StaticMutex Factory::mDeviceLock;
+StaticMutex Factory::mDTDependencyLock;
 #endif
 
 DrawEventRecorder *Factory::mRecorder;