Bug 1871613 - Ensure CanvasDrawEventRecorder releases external surface dependencies. r=gfx-reviewers,lsalzman

This patch makes CanvasDrawEventRecorder track what eventCount we
recorded an external surface reference. When the reader has increment
its processedCount above that, we will release our reference as it
should have acquired a strong reference to the data. This was previously
done when we forwarded the texture, but with remote textures, we no
longer have this event. Now we check when we start a new recording, or
attempt to clear cached resources.

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

dom/canvas/DrawTargetWebgl.cpp

@@ -297,10 +297,10 @@ void SharedContextWebgl::ExitTlsScope() {
 // Remove any SourceSurface user data associated with this TextureHandle.
 inline void SharedContextWebgl::UnlinkSurfaceTexture(
     const RefPtr<TextureHandle>& aHandle) {
-  if (SourceSurface* surface = aHandle->GetSurface()) {
+  if (RefPtr<SourceSurface> surface = aHandle->GetSurface()) {
     // Ensure any WebGL snapshot textures get unlinked.
     if (surface->GetType() == SurfaceType::WEBGL) {
-      static_cast<SourceSurfaceWebgl*>(surface)->OnUnlinkTexture(this);
+      static_cast<SourceSurfaceWebgl*>(surface.get())->OnUnlinkTexture(this);
     }
     surface->RemoveUserData(aHandle->IsShadow() ? &mShadowTextureKey
                                                 : &mTextureHandleKey);
@@ -1701,12 +1701,6 @@ static inline bool SupportsLayering(const DrawOptions& aOptions) {
   }
 }
 
-// When a texture handle is no longer referenced, it must mark itself unused
-// by unlinking its owning surface.
-static void ReleaseTextureHandle(void* aPtr) {
-  static_cast<TextureHandle*>(aPtr)->SetSurface(nullptr);
-}
-
 bool DrawTargetWebgl::DrawRect(const Rect& aRect, const Pattern& aPattern,
                                const DrawOptions& aOptions,
                                Maybe<DeviceColor> aMaskColor,
@@ -2369,7 +2363,7 @@ bool SharedContextWebgl::DrawRectAccel(
         } else {
           handle->SetSurface(surfacePattern.mSurface);
           surfacePattern.mSurface->AddUserData(&mTextureHandleKey, handle.get(),
-                                               ReleaseTextureHandle);
+                                               nullptr);
         }
       }
 

dom/canvas/DrawTargetWebglInternal.h

@@ -202,8 +202,14 @@ class TextureHandle : public RefCounted<TextureHandle>,
   bool IsValid() const { return mValid; }
   void Invalidate() { mValid = false; }
 
-  void SetSurface(SourceSurface* aSurface) { mSurface = aSurface; }
-  SourceSurface* GetSurface() const { return mSurface; }
+  void ClearSurface() { mSurface = nullptr; }
+  void SetSurface(const RefPtr<SourceSurface>& aSurface) {
+    mSurface = aSurface;
+  }
+  already_AddRefed<SourceSurface> GetSurface() const {
+    RefPtr<SourceSurface> surface(mSurface);
+    return surface.forget();
+  }
 
   float GetSigma() const { return mSigma; }
   void SetSigma(float aSigma) { mSigma = aSigma; }
@@ -222,14 +228,14 @@ class TextureHandle : public RefCounted<TextureHandle>,
 
   // Note as used if there is corresponding surface or cache entry.
   bool IsUsed() const {
-    return mSurface || (mCacheEntry && mCacheEntry->IsValid());
+    return !mSurface.IsDead() || (mCacheEntry && mCacheEntry->IsValid());
   }
 
  private:
   bool mValid = true;
   // If applicable, weak pointer to the SourceSurface that is linked to this
   // TextureHandle.
-  SourceSurface* mSurface = nullptr;
+  ThreadSafeWeakPtr<SourceSurface> mSurface;
   // If this TextureHandle stores a cached shadow, then we need to remember the
   // blur sigma used to produce the shadow.
   float mSigma = -1.0f;

dom/canvas/SourceSurfaceWebgl.cpp

@@ -28,7 +28,7 @@ SourceSurfaceWebgl::SourceSurfaceWebgl(
 SourceSurfaceWebgl::~SourceSurfaceWebgl() {
   if (mHandle) {
     // Signal that the texture handle is not being used now.
-    mHandle->SetSurface(nullptr);
+    mHandle->ClearSurface();
   }
 }
 

gfx/2d/DrawEventRecorder.cpp

@@ -25,7 +25,7 @@ void DrawEventRecorderPrivate::StoreExternalSurfaceRecording(
   NS_ASSERT_OWNINGTHREAD(DrawEventRecorderPrivate);
 
   RecordEvent(RecordedExternalSurfaceCreation(aSurface, aKey));
-  mExternalSurfaces.push_back(aSurface);
+  mExternalSurfaces.push_back({aSurface});
 }
 
 void DrawEventRecorderPrivate::StoreSourceSurfaceRecording(

gfx/2d/DrawEventRecorder.h

@@ -11,6 +11,7 @@
 #include "RecordedEvent.h"
 #include "RecordingTypes.h"
 
+#include <deque>
 #include <functional>
 #include <vector>
 
@@ -159,11 +160,6 @@ class DrawEventRecorderPrivate : public DrawEventRecorder {
 
   bool WantsExternalFonts() const { return mExternalFonts; }
 
-  void TakeExternalSurfaces(std::vector<RefPtr<SourceSurface>>& aSurfaces) {
-    NS_ASSERT_OWNINGTHREAD(DrawEventRecorderPrivate);
-    aSurfaces = std::move(mExternalSurfaces);
-  }
-
   virtual void StoreSourceSurfaceRecording(SourceSurface* aSurface,
                                            const char* aReason);
 
@@ -179,6 +175,18 @@ class DrawEventRecorderPrivate : public DrawEventRecorder {
     MOZ_CRASH("GFX: AddDependentSurface");
   }
 
+  struct ExternalSurfaceEntry {
+    RefPtr<SourceSurface> mSurface;
+    int64_t mEventCount = -1;
+  };
+
+  using ExternalSurfacesHolder = std::deque<ExternalSurfaceEntry>;
+
+  void TakeExternalSurfaces(ExternalSurfacesHolder& aSurfaces) {
+    NS_ASSERT_OWNINGTHREAD(DrawEventRecorderPrivate);
+    aSurfaces = std::move(mExternalSurfaces);
+  }
+
  protected:
   NS_DECL_OWNINGTHREAD
 
@@ -220,7 +228,8 @@ class DrawEventRecorderPrivate : public DrawEventRecorder {
   // SourceSurfaces can get deleted off the main thread, so we hold a map of the
   // raw pointer to a ThreadSafeWeakPtr to protect against this.
   nsTHashMap<void*, ThreadSafeWeakPtr<SourceSurface>> mStoredSurfaces;
-  std::vector<RefPtr<SourceSurface>> mExternalSurfaces;
+
+  ExternalSurfacesHolder mExternalSurfaces;
   bool mExternalFonts;
 };
 

gfx/layers/CanvasDrawEventRecorder.cpp

@@ -116,6 +116,7 @@ int64_t CanvasDrawEventRecorder::CreateCheckpoint() {
   NS_ASSERT_OWNINGTHREAD(CanvasDrawEventRecorder);
   int64_t checkpoint = mHeader->eventCount;
   RecordEvent(RecordedCheckpoint());
+  ClearProcessedExternalSurfaces();
   return checkpoint;
 }
 
@@ -247,6 +248,8 @@ void CanvasDrawEventRecorder::DropFreeBuffers() {
     mCurrentBuffer = CanvasBuffer(std::move(mRecycledBuffers.front().shmem));
     mRecycledBuffers.pop();
   }
+
+  ClearProcessedExternalSurfaces();
 }
 
 void CanvasDrawEventRecorder::IncrementEventCount() {
@@ -307,6 +310,7 @@ void CanvasDrawEventRecorder::StoreSourceSurfaceRecording(
     nsresult rv = layers::SharedSurfacesChild::Share(aSurface, extId);
     if (NS_SUCCEEDED(rv)) {
       StoreExternalSurfaceRecording(aSurface, wr::AsUint64(extId));
+      mExternalSurfaces.back().mEventCount = mHeader->eventCount;
       return;
     }
   }
@@ -314,5 +318,14 @@ void CanvasDrawEventRecorder::StoreSourceSurfaceRecording(
   DrawEventRecorderPrivate::StoreSourceSurfaceRecording(aSurface, aReason);
 }
 
+void CanvasDrawEventRecorder::ClearProcessedExternalSurfaces() {
+  while (!mExternalSurfaces.empty()) {
+    if (mExternalSurfaces.front().mEventCount > mHeader->processedCount) {
+      break;
+    }
+    mExternalSurfaces.pop_front();
+  }
+}
+
 }  // namespace layers
 }  // namespace mozilla

gfx/layers/CanvasDrawEventRecorder.h

@@ -116,6 +116,8 @@ class CanvasDrawEventRecorder final : public gfx::DrawEventRecorderPrivate,
 
   void DropFreeBuffers();
 
+  void ClearProcessedExternalSurfaces();
+
  protected:
   gfx::ContiguousBuffer& GetContiguousBuffer(size_t aSize) final;
 

gfx/layers/client/TextureRecorded.cpp

@@ -78,16 +78,12 @@ bool RecordedTextureData::Lock(OpenMode aMode) {
     }
 
     // We lock the TextureData when we create it to get the remote DrawTarget.
-    mCanvasChild->OnTextureWriteLock();
     mLockedMode = aMode;
     return true;
   }
 
   mCanvasChild->RecordEvent(RecordedTextureLock(
       mTextureId, aMode, mLastRemoteTextureId, obsoleteRemoteTextureId));
-  if (aMode & OpenMode::OPEN_WRITE) {
-    mCanvasChild->OnTextureWriteLock();
-  }
   mLockedMode = aMode;
   return true;
 }
@@ -165,7 +161,8 @@ bool RecordedTextureData::Serialize(SurfaceDescriptor& aDescriptor) {
 }
 
 void RecordedTextureData::OnForwardedToHost() {
-  mCanvasChild->OnTextureForwarded();
+  // Compositing with RecordedTextureData requires RemoteTextureMap.
+  MOZ_CRASH("OnForwardedToHost not supported!");
 }
 
 TextureFlags RecordedTextureData::GetTextureFlags() const {

gfx/layers/ipc/CanvasChild.cpp

@@ -238,33 +238,6 @@ void CanvasChild::Destroy() {
   }
 }
 
-void CanvasChild::OnTextureWriteLock() {
-  NS_ASSERT_OWNINGTHREAD(CanvasChild);
-
-  mHasOutstandingWriteLock = true;
-  mLastWriteLockCheckpoint = mRecorder->CreateCheckpoint();
-}
-
-void CanvasChild::OnTextureForwarded() {
-  NS_ASSERT_OWNINGTHREAD(CanvasChild);
-
-  if (mHasOutstandingWriteLock) {
-    mRecorder->RecordEvent(RecordedCanvasFlush());
-    if (!mRecorder->WaitForCheckpoint(mLastWriteLockCheckpoint)) {
-      gfxWarning() << "Timed out waiting for last write lock to be processed.";
-    }
-
-    mHasOutstandingWriteLock = false;
-  }
-
-  // We hold onto the last transaction's external surfaces until we have waited
-  // for the write locks in this transaction. This means we know that the
-  // surfaces have been picked up in the canvas threads and there is no race
-  // with them being removed from SharedSurfacesParent. Note this releases the
-  // current contents of mLastTransactionExternalSurfaces.
-  mRecorder->TakeExternalSurfaces(mLastTransactionExternalSurfaces);
-}
-
 bool CanvasChild::EnsureBeginTransaction() {
   NS_ASSERT_OWNINGTHREAD(CanvasChild);
 

gfx/layers/ipc/CanvasChild.h

@@ -63,16 +63,6 @@ class CanvasChild final : public PCanvasChild, public SupportsWeakPtr {
    */
   void Destroy();
 
-  /**
-   * Called when a RecordedTextureData is write locked.
-   */
-  void OnTextureWriteLock();
-
-  /**
-   * Called when a RecordedTextureData is forwarded to the compositor.
-   */
-  void OnTextureForwarded();
-
   /**
    * @returns true if we should be caching data surfaces in the GPU process.
    */
@@ -173,14 +163,12 @@ class CanvasChild final : public PCanvasChild, public SupportsWeakPtr {
   bool mDataSurfaceShmemAvailable = false;
   int64_t mLastWriteLockCheckpoint = 0;
   uint32_t mTransactionsSinceGetDataSurface = kCacheDataSurfaceThreshold;
-  std::vector<RefPtr<gfx::SourceSurface>> mLastTransactionExternalSurfaces;
   struct TextureInfo {
     RefPtr<mozilla::ipc::SharedMemoryBasic> mSnapshotShmem;
     bool mRequiresRefresh = false;
   };
   std::unordered_map<int64_t, TextureInfo> mTextureInfo;
   bool mIsInTransaction = false;
-  bool mHasOutstandingWriteLock = false;
   bool mDormant = false;
 };
 

gfx/layers/wr/WebRenderCommandBuilder.cpp

@@ -108,7 +108,7 @@ struct BlobItemData {
   // We need to keep a list of all the external surfaces used by the blob image.
   // We do this on a per-display item basis so that the lists remains correct
   // during invalidations.
-  std::vector<RefPtr<SourceSurface>> mExternalSurfaces;
+  DrawEventRecorderPrivate::ExternalSurfacesHolder mExternalSurfaces;
 
   BlobItemData(DIGroup* aGroup, nsDisplayItem* aItem)
       : mInvisible(false), mUsed(false), mGroup(aGroup) {
@@ -183,18 +183,18 @@ static void DestroyBlobGroupDataProperty(nsTArray<BlobItemData*>* aArray) {
 
 static void TakeExternalSurfaces(
     WebRenderDrawEventRecorder* aRecorder,
-    std::vector<RefPtr<SourceSurface>>& aExternalSurfaces,
+    DrawEventRecorderPrivate::ExternalSurfacesHolder& aExternalSurfaces,
     RenderRootStateManager* aManager, wr::IpcResourceUpdateQueue& aResources) {
   aRecorder->TakeExternalSurfaces(aExternalSurfaces);
 
-  for (auto& surface : aExternalSurfaces) {
+  for (auto& entry : aExternalSurfaces) {
     // While we don't use the image key with the surface, because the blob image
     // renderer doesn't have easy access to the resource set, we still want to
     // ensure one is generated. That will ensure the surface remains alive until
     // at least the last epoch which the blob image could be used in.
     wr::ImageKey key;
     DebugOnly<nsresult> rv =
-        SharedSurfacesChild::Share(surface, aManager, aResources, key);
+        SharedSurfacesChild::Share(entry.mSurface, aManager, aResources, key);
     MOZ_ASSERT(rv.value != NS_ERROR_NOT_IMPLEMENTED);
   }
 }

gfx/layers/wr/WebRenderUserData.h

@@ -8,6 +8,7 @@
 #define GFX_WEBRENDERUSERDATA_H
 
 #include <vector>
+#include "mozilla/gfx/DrawEventRecorder.h"
 #include "mozilla/webrender/WebRenderAPI.h"
 #include "mozilla/image/WebRenderImageProvider.h"
 #include "mozilla/layers/AnimationInfo.h"
@@ -236,7 +237,7 @@ class WebRenderFallbackData : public WebRenderUserData {
   /// into.
   WebRenderImageData* PaintIntoImage();
 
-  std::vector<RefPtr<gfx::SourceSurface>> mExternalSurfaces;
+  gfx::DrawEventRecorderPrivate::ExternalSurfacesHolder mExternalSurfaces;
   UniquePtr<nsDisplayItemGeometry> mGeometry;
   DisplayItemClip mClip;
   nsRect mBounds;
@@ -344,7 +345,7 @@ class WebRenderMaskData : public WebRenderUserData {
 
   Maybe<wr::BlobImageKey> mBlobKey;
   std::vector<RefPtr<gfx::ScaledFont>> mFonts;
-  std::vector<RefPtr<gfx::SourceSurface>> mExternalSurfaces;
+  gfx::DrawEventRecorderPrivate::ExternalSurfacesHolder mExternalSurfaces;
   LayerIntRect mItemRect;
   nsPoint mMaskOffset;
   nsStyleImageLayers mMaskStyle;

image/BlobSurfaceProvider.cpp

@@ -289,17 +289,17 @@ Maybe<BlobImageKeyData> BlobSurfaceProvider::RecordDrawing(
     return Nothing();
   }
 
-  std::vector<RefPtr<SourceSurface>> externalSurfaces;
+  DrawEventRecorderPrivate::ExternalSurfacesHolder externalSurfaces;
   recorder->TakeExternalSurfaces(externalSurfaces);
 
-  for (auto& surface : externalSurfaces) {
+  for (auto& entry : externalSurfaces) {
     // While we don't use the image key with the surface, because the blob image
     // renderer doesn't have easy access to the resource set, we still want to
     // ensure one is generated. That will ensure the surface remains alive until
     // at least the last epoch which the blob image could be used in.
     wr::ImageKey key = {};
-    DebugOnly<nsresult> rv =
-        SharedSurfacesChild::Share(surface, rootManager, aResources, key);
+    DebugOnly<nsresult> rv = SharedSurfacesChild::Share(
+        entry.mSurface, rootManager, aResources, key);
     MOZ_ASSERT(rv.value != NS_ERROR_NOT_IMPLEMENTED);
   }
 

image/BlobSurfaceProvider.h

@@ -10,6 +10,7 @@
 #include "mozilla/Maybe.h"
 #include "mozilla/SVGImageContext.h"
 #include "mozilla/gfx/2D.h"
+#include "mozilla/gfx/DrawEventRecorder.h"
 #include "mozilla/layers/WebRenderLayerManager.h"
 #include "ImageRegion.h"
 #include "ISurfaceProvider.h"
@@ -21,10 +22,10 @@ namespace image {
 
 class BlobImageKeyData final {
  public:
-  BlobImageKeyData(layers::WebRenderLayerManager* aManager,
-                   const wr::BlobImageKey& aBlobKey,
-                   std::vector<RefPtr<gfx::ScaledFont>>&& aScaledFonts,
-                   std::vector<RefPtr<gfx::SourceSurface>>&& aExternalSurfaces)
+  BlobImageKeyData(
+      layers::WebRenderLayerManager* aManager, const wr::BlobImageKey& aBlobKey,
+      std::vector<RefPtr<gfx::ScaledFont>>&& aScaledFonts,
+      gfx::DrawEventRecorderPrivate::ExternalSurfacesHolder&& aExternalSurfaces)
       : mManager(aManager),
         mBlobKey(aBlobKey),
         mScaledFonts(std::move(aScaledFonts)),
@@ -53,7 +54,7 @@ class BlobImageKeyData final {
   RefPtr<layers::WebRenderLayerManager> mManager;
   wr::BlobImageKey mBlobKey;
   std::vector<RefPtr<gfx::ScaledFont>> mScaledFonts;
-  std::vector<RefPtr<gfx::SourceSurface>> mExternalSurfaces;
+  gfx::DrawEventRecorderPrivate::ExternalSurfacesHolder mExternalSurfaces;
   bool mDirty;
 };