Bug 1989517 - Ensure DrawTarget removed from RecordedTextureDestruction playback. r=aosmond, a=RyanVM

CanvasTranslator::RemoveTexture delayed the call to RemoveDrawTarget till all keepalives are
gone. However, this may take place long after RecordedTextureDestruction actually called
RemoveTexture. This gives time for subsequent DrawTargets with the same ReferencePtr to be
created, causing the wrong DrawTargets to be removed.

This patch ensures that when RemoveTexture is called from a RecordedTextureDestruction event
that the DrawTarget is always removed then and there, rather than waiting.

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

gfx/layers/ipc/CanvasTranslator.cpp

@@ -533,7 +533,9 @@ bool CanvasTranslator::TryDrawTargetWebglFallback(
           CreateFallbackDrawTarget(info.mRefPtr, aTextureOwnerId,
                                    aWebgl->GetSize(), aWebgl->GetFormat())) {
     bool success = aWebgl->CopyToFallback(dt);
+    if (info.mRefPtr) {
       AddDrawTarget(info.mRefPtr, dt);
+    }
     return success;
   }
   return false;
@@ -1084,16 +1086,18 @@ void CanvasTranslator::CacheSnapshotShmem(
   if (gfx::DrawTargetWebgl* webgl = GetDrawTargetWebgl(aTextureOwnerId)) {
     if (auto shmemHandle = webgl->TakeShmemHandle()) {
       // Lock the DT so that it doesn't get removed while shmem is in transit.
-      mTextureInfo[aTextureOwnerId].mLocked++;
+      AddTextureKeepAlive(aTextureOwnerId);
       nsCOMPtr<nsIThread> thread =
           gfx::CanvasRenderThread::GetCanvasRenderThread();
       RefPtr<CanvasTranslator> translator = this;
       SendSnapshotShmem(aTextureOwnerId, std::move(shmemHandle))
           ->Then(
               thread, __func__,
-              [=](bool) { translator->RemoveTexture(aTextureOwnerId); },
+              [=](bool) {
+                translator->RemoveTextureKeepAlive(aTextureOwnerId);
+              },
               [=](ipc::ResponseRejectReason) {
-                translator->RemoveTexture(aTextureOwnerId);
+                translator->RemoveTextureKeepAlive(aTextureOwnerId);
               });
     }
   }
@@ -1267,7 +1271,9 @@ already_AddRefed<gfx::DrawTarget> CanvasTranslator::CreateDrawTarget(
     dt = CreateFallbackDrawTarget(aRefPtr, aTextureOwnerId, aSize, aFormat);
   }
 
+  if (dt && aRefPtr) {
     AddDrawTarget(aRefPtr, dt);
+  }
   return dt.forget();
 }
 
@@ -1290,9 +1296,23 @@ void CanvasTranslator::NotifyTextureDestruction(
   Unused << SendNotifyTextureDestruction(aTextureOwnerId);
 }
 
+void CanvasTranslator::AddTextureKeepAlive(const RemoteTextureOwnerId& aId) {
+  auto result = mTextureInfo.find(aId);
+  if (result == mTextureInfo.end()) {
+    return;
+  }
+  auto& info = result->second;
+  ++info.mKeepAlive;
+}
+
+void CanvasTranslator::RemoveTextureKeepAlive(const RemoteTextureOwnerId& aId) {
+  RemoveTexture(aId, 0, 0, false);
+}
+
 void CanvasTranslator::RemoveTexture(const RemoteTextureOwnerId aTextureOwnerId,
                                      RemoteTextureTxnType aTxnType,
-                                     RemoteTextureTxnId aTxnId) {
+                                     RemoteTextureTxnId aTxnId,
+                                     bool aFinalize) {
   // Don't erase the texture if still in use
   auto result = mTextureInfo.find(aTextureOwnerId);
   if (result == mTextureInfo.end()) {
@@ -1302,10 +1322,17 @@ void CanvasTranslator::RemoveTexture(const RemoteTextureOwnerId aTextureOwnerId,
   if (mRemoteTextureOwner && aTxnType && aTxnId) {
     mRemoteTextureOwner->WaitForTxn(aTextureOwnerId, aTxnType, aTxnId);
   }
-  if (--info.mLocked > 0) {
+  // Remove the DrawTarget only if this is being called from a recorded event
+  // or if there are no remaining keepalives. If this is being called only to
+  // remove a keepalive without forcing removal, then the DrawTarget is still
+  // being used by the recording.
+  if ((aFinalize || info.mKeepAlive <= 1) && info.mRefPtr) {
+    RemoveDrawTarget(info.mRefPtr);
+    info.mRefPtr = ReferencePtr();
+  }
+  if (--info.mKeepAlive > 0) {
     return;
   }
-  RemoveDrawTarget(info.mRefPtr);
   if (info.mTextureData) {
     if (info.mFallbackDrawTarget) {
       info.mTextureData->ReturnDrawTarget(info.mFallbackDrawTarget.forget());

gfx/layers/ipc/CanvasTranslator.h

@@ -210,8 +210,8 @@ class CanvasTranslator final : public gfx::InlineTranslator,
    * @param aTextureOwnerId the texture ID to remove
    */
   void RemoveTexture(const RemoteTextureOwnerId aTextureOwnerId,
-                     RemoteTextureTxnType aTxnType = 0,
-                     RemoteTextureTxnId aTxnId = 0);
+                     RemoteTextureTxnType aTxnType, RemoteTextureTxnId aTxnId,
+                     bool aFinalize = true);
 
   bool LockTexture(const RemoteTextureOwnerId aTextureOwnerId, OpenMode aMode,
                    bool aInvalidContents = false);
@@ -563,7 +563,7 @@ class CanvasTranslator final : public gfx::InlineTranslator,
     RefPtr<gfx::DrawTarget> mFallbackDrawTarget;
     bool mNotifiedRequiresRefresh = false;
     // Ref-count of how active uses of the DT. Avoids deletion when locked.
-    int32_t mLocked = 1;
+    int32_t mKeepAlive = 1;
     OpenMode mTextureLockMode = OpenMode::OPEN_NONE;
 
     gfx::DrawTargetWebgl* GetDrawTargetWebgl(
@@ -572,6 +572,10 @@ class CanvasTranslator final : public gfx::InlineTranslator,
   std::unordered_map<RemoteTextureOwnerId, TextureInfo,
                      RemoteTextureOwnerId::HashFn>
       mTextureInfo;
+
+  void AddTextureKeepAlive(const RemoteTextureOwnerId& aId);
+  void RemoveTextureKeepAlive(const RemoteTextureOwnerId& aId);
+
   nsRefPtrHashtable<nsPtrHashKey<void>, gfx::DataSourceSurface> mDataSurfaces;
   gfx::ReferencePtr mMappedSurface;
   UniquePtr<gfx::DataSourceSurface::ScopedMap> mPreparedMap;