Bug 1825169: Process off main thread deletions in DrawEventRecorderPrivate::AddStoredObject. r=aosmond

Differential Revision: https://phabricator.services.mozilla.com/D174952

gfx/2d/DrawEventRecorder.h

@@ -14,7 +14,9 @@
 #include <unordered_set>
 #include <unordered_map>
 #include <functional>
+#include <vector>
 
+#include "mozilla/DataMutex.h"
 #include "nsTHashSet.h"
 
 namespace mozilla {
@@ -63,12 +65,24 @@ class DrawEventRecorderPrivate : public DrawEventRecorder {
   }
 
   virtual void RecordEvent(const RecordedEvent& aEvent) = 0;
-  void WritePath(const PathRecording* aPath);
 
   void AddStoredObject(const ReferencePtr aObject) {
+    PendingDeletionsVector pendingDeletions;
+    {
+      auto lockedPendingDeletions = mPendingDeleltions.Lock();
+      pendingDeletions.swap(*lockedPendingDeletions);
+    }
+    for (const auto& pendingDeletion : pendingDeletions) {
+      pendingDeletion();
+    }
     mStoredObjects.insert(aObject);
   }
 
+  void AddPendingDeletion(std::function<void()>&& aPendingDeletion) {
+    auto lockedPendingDeletions = mPendingDeleltions.Lock();
+    lockedPendingDeletions->emplace_back(std::move(aPendingDeletion));
+  }
+
   void RemoveStoredObject(const ReferencePtr aObject) {
     mStoredObjects.erase(aObject);
   }
@@ -127,17 +141,12 @@ class DrawEventRecorderPrivate : public DrawEventRecorder {
                                            const char* aReason);
 
   /**
-   * This is virtual to allow subclasses to control the recording, if for
-   * example it needs to happen on a specific thread. aSurface is a void*
-   * instead of a SourceSurface* because this is called during the SourceSurface
-   * destructor, so it is partially destructed and should not be accessed. If we
-   * use a SourceSurface* we might, for example, accidentally AddRef/Release the
-   * object by passing it to NewRunnableMethod to submit to a different thread.
-   * We are only using the pointer as a lookup ID to our internal maps and
-   * ReferencePtr to be used on the translation side.
+   * Used when a source surface is destroyed, aSurface is a void* instead of a
+   * SourceSurface* because this is called during the SourceSurface destructor,
+   * so it is partially destructed and should not be accessed.
    * @param aSurface the surface whose destruction is being recorded
    */
-  virtual void RecordSourceSurfaceDestruction(void* aSurface);
+  void RecordSourceSurfaceDestruction(void* aSurface);
 
   virtual void AddDependentSurface(uint64_t aDependencyId) {
     MOZ_CRASH("GFX: AddDependentSurface");
@@ -150,6 +159,10 @@ class DrawEventRecorderPrivate : public DrawEventRecorder {
 
   std::unordered_set<const void*> mStoredObjects;
 
+  using PendingDeletionsVector = std::vector<std::function<void()>>;
+  DataMutex<PendingDeletionsVector> mPendingDeleltions{
+      "DrawEventRecorderPrivate::mPendingDeleltions"};
+
   // It's difficult to track the lifetimes of UnscaledFonts directly, so we
   // instead track the number of recorded ScaledFonts that hold a reference to
   // an Unscaled font and use that as a proxy to the real lifetime. An

gfx/2d/DrawTargetRecording.cpp

@@ -35,10 +35,16 @@ static void RecordingSourceSurfaceUserDataFunc(void* aUserData) {
   RecordingSourceSurfaceUserData* userData =
       static_cast<RecordingSourceSurfaceUserData*>(aUserData);
 
-  userData->recorder->RecordSourceSurfaceDestruction(
-      static_cast<SourceSurface*>(userData->refPtr));
+  if (NS_IsMainThread()) {
+    userData->recorder->RecordSourceSurfaceDestruction(userData->refPtr);
+    delete userData;
+    return;
+  }
 
-  delete userData;
+  userData->recorder->AddPendingDeletion([userData]() -> void {
+    userData->recorder->RecordSourceSurfaceDestruction(userData->refPtr);
+    delete userData;
+  });
 }
 
 static void EnsureSurfaceStoredRecording(DrawEventRecorderPrivate* aRecorder,
@@ -48,8 +54,11 @@ static void EnsureSurfaceStoredRecording(DrawEventRecorderPrivate* aRecorder,
     return;
   }
 
-  aRecorder->StoreSourceSurfaceRecording(aSurface, reason);
+  // It's important that AddStoredObject is called first because that will
+  // run any pending processing required by recorded objects that have been
+  // deleted off the main thread.
   aRecorder->AddStoredObject(aSurface);
+  aRecorder->StoreSourceSurfaceRecording(aSurface, reason);
   aRecorder->AddSourceSurface(aSurface);
 
   RecordingSourceSurfaceUserData* userData = new RecordingSourceSurfaceUserData;
@@ -696,8 +705,11 @@ already_AddRefed<PathRecording> DrawTargetRecording::EnsurePathStored(
     pathRecording = builderRecording->Finish().downcast<PathRecording>();
   }
 
-  mRecorder->RecordEvent(RecordedPathCreation(pathRecording.get()));
+  // It's important that AddStoredObject is called first because that will
+  // run any pending processing required by recorded objects that have been
+  // deleted off the main thread.
   mRecorder->AddStoredObject(pathRecording);
+  mRecorder->RecordEvent(RecordedPathCreation(pathRecording.get()));
   pathRecording->mStoredRecorders.push_back(mRecorder);
 
   return pathRecording.forget();

gfx/layers/CanvasDrawEventRecorder.cpp

@@ -514,18 +514,5 @@ void CanvasDrawEventRecorder::StoreSourceSurfaceRecording(
   StoreExternalSurfaceRecording(aSurface, wr::AsUint64(extId));
 }
 
-void CanvasDrawEventRecorder::RecordSourceSurfaceDestruction(void* aSurface) {
-  // We must only record things on the main thread and surfaces that have been
-  // recorded can sometimes be destroyed off the main thread.
-  if (NS_IsMainThread()) {
-    DrawEventRecorderPrivate::RecordSourceSurfaceDestruction(aSurface);
-    return;
-  }
-
-  NS_DispatchToMainThread(NewRunnableMethod<void*>(
-      "DrawEventRecorderPrivate::RecordSourceSurfaceDestruction", this,
-      &DrawEventRecorderPrivate::RecordSourceSurfaceDestruction, aSurface));
-}
-
 }  // namespace layers
 }  // namespace mozilla

gfx/layers/CanvasDrawEventRecorder.h

@@ -256,8 +256,6 @@ class CanvasDrawEventRecorder final : public gfx::DrawEventRecorderPrivate {
   void StoreSourceSurfaceRecording(gfx::SourceSurface* aSurface,
                                    const char* aReason) final;
 
-  void RecordSourceSurfaceDestruction(void* aSurface) final;
-
   void Flush() final {}
 
   void ReturnRead(char* aOut, size_t aSize) {

gfx/layers/ipc/CanvasChild.cpp

@@ -50,13 +50,29 @@ class SourceSurfaceCanvasRecording final : public gfx::SourceSurface {
       : mRecordedSurface(aRecordedSuface),
         mCanvasChild(aCanvasChild),
         mRecorder(aRecorder) {
-    mRecorder->RecordEvent(RecordedAddSurfaceAlias(this, aRecordedSuface));
+    // It's important that AddStoredObject is called first because that will
+    // run any pending processing required by recorded objects that have been
+    // deleted off the main thread.
     mRecorder->AddStoredObject(this);
+    mRecorder->RecordEvent(RecordedAddSurfaceAlias(this, aRecordedSuface));
   }
 
   ~SourceSurfaceCanvasRecording() {
-    ReleaseOnMainThread(std::move(mRecorder), this, std::move(mRecordedSurface),
-                        std::move(mCanvasChild));
+    ReferencePtr surfaceAlias = this;
+    if (NS_IsMainThread()) {
+      ReleaseOnMainThread(std::move(mRecorder), surfaceAlias,
+                          std::move(mRecordedSurface), std::move(mCanvasChild));
+      return;
+    }
+
+    mRecorder->AddPendingDeletion(
+        [recorder = std::move(mRecorder), surfaceAlias,
+         aliasedSurface = std::move(mRecordedSurface),
+         canvasChild = std::move(mCanvasChild)]() mutable -> void {
+          ReleaseOnMainThread(std::move(recorder), surfaceAlias,
+                              std::move(aliasedSurface),
+                              std::move(canvasChild));
+        });
   }
 
   gfx::SurfaceType GetType() const final { return mRecordedSurface->GetType(); }
@@ -85,14 +101,7 @@ class SourceSurfaceCanvasRecording final : public gfx::SourceSurface {
                                   ReferencePtr aSurfaceAlias,
                                   RefPtr<gfx::SourceSurface> aAliasedSurface,
                                   RefPtr<CanvasChild> aCanvasChild) {
-    if (!NS_IsMainThread()) {
-      NS_DispatchToMainThread(NewRunnableFunction(
-          "SourceSurfaceCanvasRecording::ReleaseOnMainThread",
-          SourceSurfaceCanvasRecording::ReleaseOnMainThread,
-          std::move(aRecorder), aSurfaceAlias, std::move(aAliasedSurface),
-          std::move(aCanvasChild)));
-      return;
-    }
+    MOZ_ASSERT(NS_IsMainThread());
 
     aRecorder->RemoveStoredObject(aSurfaceAlias);
     aRecorder->RecordEvent(RecordedRemoveSurfaceAlias(aSurfaceAlias));