From 1d97d5f6f4e2957f0060565be83ea46957108d30 Mon Sep 17 00:00:00 2001 From: Lee Salzman Date: Wed, 24 Sep 2025 06:02:23 +0000 Subject: [PATCH] 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 | 43 +++++++++++++++++++++++------ gfx/layers/ipc/CanvasTranslator.h | 10 +++++-- 2 files changed, 42 insertions(+), 11 deletions(-) diff --git a/gfx/layers/ipc/CanvasTranslator.cpp b/gfx/layers/ipc/CanvasTranslator.cpp index 491a5fda178b..68b8336bcf03 100644 --- a/gfx/layers/ipc/CanvasTranslator.cpp +++ b/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); - AddDrawTarget(info.mRefPtr, 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 thread = gfx::CanvasRenderThread::GetCanvasRenderThread(); RefPtr 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 CanvasTranslator::CreateDrawTarget( dt = CreateFallbackDrawTarget(aRefPtr, aTextureOwnerId, aSize, aFormat); } - AddDrawTarget(aRefPtr, dt); + 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()); diff --git a/gfx/layers/ipc/CanvasTranslator.h b/gfx/layers/ipc/CanvasTranslator.h index 6e15cc130801..c69bcd25b217 100644 --- a/gfx/layers/ipc/CanvasTranslator.h +++ b/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 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 mTextureInfo; + + void AddTextureKeepAlive(const RemoteTextureOwnerId& aId); + void RemoveTextureKeepAlive(const RemoteTextureOwnerId& aId); + nsRefPtrHashtable, gfx::DataSourceSurface> mDataSurfaces; gfx::ReferencePtr mMappedSurface; UniquePtr mPreparedMap;