From 731888c38ee41e5892488f1edb8b45160fa9863c Mon Sep 17 00:00:00 2001
From: Thomas Smith <[email protected]>
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: b/521491024
Change-Id: Idb2b5eaccda7f4ec388e3dc415ee41d329d28533
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1295737
Reviewed-by: Michael Ludwig <[email protected]>
Commit-Queue: Thomas Smith <[email protected]>
---

diff --git a/src/gpu/ganesh/GrDrawingManager.cpp b/src/gpu/ganesh/GrDrawingManager.cpp
index 398bc35..55b58d9 100644
--- a/src/gpu/ganesh/GrDrawingManager.cpp
+++ b/src/gpu/ganesh/GrDrawingManager.cpp
@@ -175,6 +175,7 @@
     }
 
     bool cachePurgeNeeded = false;
+    bool flushSuccessful = false;
 
     if (preFlushSuccessful) {
         bool usingReorderedDAG = false;
@@ -205,8 +206,10 @@
             resourceAllocator.assign();
         }
 
-        cachePurgeNeeded = !resourceAllocator.failedInstantiation() &&
-                           this->executeRenderTasks(&flushState);
+        if (!resourceAllocator.failedInstantiation()) {
+            cachePurgeNeeded = this->executeRenderTasks(&flushState);
+            flushSuccessful = true;
+        }
     }
     this->removeRenderTasks();
 
@@ -226,7 +229,7 @@
     }
     fFlushing = false;
 
-    return true;
+    return flushSuccessful;
 }
 
 bool GrDrawingManager::submitToGpu() {
diff --git a/src/gpu/ganesh/SurfaceContext.cpp b/src/gpu/ganesh/SurfaceContext.cpp
index ba6bb93..efc3f09 100644
--- a/src/gpu/ganesh/SurfaceContext.cpp
+++ b/src/gpu/ganesh/SurfaceContext.cpp
@@ -292,7 +292,12 @@
         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()),
diff --git a/tests/ReadWritePixelsGpuTest.cpp b/tests/ReadWritePixelsGpuTest.cpp
index 143ff72..c64b79e 100644
--- a/tests/ReadWritePixelsGpuTest.cpp
+++ b/tests/ReadWritePixelsGpuTest.cpp
@@ -43,7 +43,9 @@
 #include "src/gpu/ganesh/GrCaps.h"
 #include "src/gpu/ganesh/GrDataUtils.h"
 #include "src/gpu/ganesh/GrDirectContextPriv.h"
+#include "src/gpu/ganesh/GrDrawingManager.h"
 #include "src/gpu/ganesh/GrImageInfo.h"
+#include "src/gpu/ganesh/GrOnFlushResourceProvider.h"
 #include "src/gpu/ganesh/GrPixmap.h"
 #include "src/gpu/ganesh/GrSamplerState.h"
 #include "src/gpu/ganesh/GrSurfaceProxy.h"
@@ -243,6 +245,19 @@
 template <typename T>
 using GpuReadDstFn = SkAutoPixmapStorage(const T&);
 
+class FailingFlushCallback : public GrOnFlushCallbackObject {
+public:
+    bool preFlush(GrOnFlushResourceProvider*) override {
+        ++fCount;
+        return false;
+    }
+
+    int count() const { return fCount; }
+
+private:
+    int fCount = 0;
+};
+
 }  // anonymous namespace
 
 SkPixmap make_pixmap_have_valid_alpha_type(SkPixmap pm) {
@@ -1513,3 +1528,61 @@
 
     ComparePixels(syncResult.pixmap(), asyncResult, tol, error);
 }
+
+// readPixels() that uses an intermediate scratch surface should not return stale recycled scratch
+// contents if the implicit flush of the queued draw into that surface drops its render tasks.
+DEF_GANESH_TEST_FOR_RENDERING_CONTEXTS(ReadPixelsIntermediateFailedFlush,
+                                       reporter,
+                                       ctxInfo,
+                                       CtsEnforcement::kNextRelease) {
+    auto dContext = ctxInfo.directContext();
+    if (dContext->supportsProtectedContent()) {
+        return;
+    }
+
+    static constexpr int kSize = 100;
+    static constexpr SkColor kStaleColor = 0xFF112233;
+    static constexpr SkColor kSrcColor = 0xFF008800;
+
+    auto srcII = SkImageInfo::Make({kSize, kSize}, kRGBA_8888_SkColorType, kPremul_SkAlphaType);
+    auto dstII = srcII.makeAlphaType(kUnpremul_SkAlphaType);
+    auto surf = SkSurfaces::RenderTarget(dContext, skgpu::Budgeted::kYes, srcII);
+    if (!surf) {
+        return;
+    }
+
+    SkAutoPixmapStorage pixels;
+    pixels.alloc(dstII);
+
+    // Reading the unpremul destination from the premul source draws through an intermediate
+    // approx-fit surface. Do this once so a matching scratch surface lands in the resource cache
+    // with known stale contents.
+    surf->getCanvas()->clear(kStaleColor);
+    if (!surf->readPixels(pixels, 0, 0)) {
+        return;
+    }
+
+    surf->getCanvas()->clear(kSrcColor);
+    dContext->flushAndSubmit(GrSyncCpu::kYes);
+
+    // From here on the flush-time callback fails so any queued render tasks are discarded.
+    FailingFlushCallback failingCallback;
+    dContext->priv().addOnFlushCallbackObject(&failingCallback);
+
+    pixels.erase(SkColors::kTransparent);
+    bool ok = surf->readPixels(pixels, 0, 0);
+
+    dContext->priv().drawingManager()->testingOnly_removeOnFlushCallbackObject(&failingCallback);
+
+    if (ok) {
+        SkColor result = pixels.getColor(0, 0);
+        REPORTER_ASSERT(reporter,
+                        result == kSrcColor,
+                        "readPixels reported success but returned 0x%08x, expected 0x%08x",
+                        result,
+                        kSrcColor);
+    } else {
+        REPORTER_ASSERT(reporter, failingCallback.count() > 0);
+    }
+}
+