From 731888c38ee41e5892488f1edb8b45160fa9863c Mon Sep 17 00:00:00 2001
From: Thomas Smith <thomsmit@google.com>
Date: Thu, 16 Jul 2026 11:01:22 -0400
Subject: [PATCH] [ganesh] prevent stale readbacks

* Ganesh's SurfaceContext::readPixels did not consider whether the content it was attempting to readback was successfully rendered or not, leading to a scenario where stale texture data could potentially be readback.

* Add some state tracking so that a failed flush is propagated out of the drawing manager and to the surface context

Bug: https://issues.chromium.org/issues/521491024
Change-Id: Idb2b5eaccda7f4ec388e3dc415ee41d329d28533
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1295737
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Thomas Smith <thomsmit@google.com>
---
 src/gpu/ganesh/GrDrawingManager.cpp |  9 ++--
 src/gpu/ganesh/SurfaceContext.cpp   |  7 ++-
 tests/ReadWritePixelsGpuTest.cpp    | 73 +++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+), 4 deletions(-)

diff --git a/src/gpu/ganesh/GrDrawingManager.cpp b/src/gpu/ganesh/GrDrawingManager.cpp
index 398bc35c5e..55b58d958e 100644
--- a/src/gpu/ganesh/GrDrawingManager.cpp
+++ b/src/gpu/ganesh/GrDrawingManager.cpp
@@ -175,6 +175,7 @@ bool GrDrawingManager::flush(SkSpan<GrSurfaceProxy*> proxies,
     }
 
     bool cachePurgeNeeded = false;
+    bool flushSuccessful = false;
 
     if (preFlushSuccessful) {
         bool usingReorderedDAG = false;
@@ -205,8 +206,10 @@ bool GrDrawingManager::flush(SkSpan<GrSurfaceProxy*> proxies,
             resourceAllocator.assign();
         }
 
-        cachePurgeNeeded = !resourceAllocator.failedInstantiation() &&
-                           this->executeRenderTasks(&flushState);
+        if (!resourceAllocator.failedInstantiation()) {
+            cachePurgeNeeded = this->executeRenderTasks(&flushState);
+            flushSuccessful = true;
+        }
     }
     this->removeRenderTasks();
 
@@ -226,7 +229,7 @@ bool GrDrawingManager::flush(SkSpan<GrSurfaceProxy*> proxies,
     }
     fFlushing = false;
 
-    return true;
+    return flushSuccessful;
 }
 
 bool GrDrawingManager::submitToGpu() {
diff --git a/src/gpu/ganesh/SurfaceContext.cpp b/src/gpu/ganesh/SurfaceContext.cpp
index ba6bb936c4..efc3f0911a 100644
--- a/src/gpu/ganesh/SurfaceContext.cpp
+++ b/src/gpu/ganesh/SurfaceContext.cpp
@@ -292,7 +292,12 @@ bool SurfaceContext::readPixels(GrDirectContext* dContext, GrPixmap dst, SkIPoin
         pt.fY = flip ? srcSurface->height() - pt.fY - dst.height() : pt.fY;
     }
 
-    dContext->priv().flushSurface(srcProxy.get());
+    bool hasPendingTasks =
+            dContext->priv().drawingManager()->getLastRenderTask(srcProxy.get()) != nullptr;
+    GrSemaphoresSubmitted flushResult = dContext->priv().flushSurface(srcProxy.get());
+    if (flushResult == GrSemaphoresSubmitted::kNo && hasPendingTasks) {
+        return false;
+    }
     dContext->submit();
     if (!dContext->priv().getGpu()->readPixels(srcSurface,
                                                SkIRect::MakePtSize(pt, dst.dimensions()),
-- 
2.53.0

