From e6496d127941b51f348f4c880032547d4c440c37 Mon Sep 17 00:00:00 2001
From: Robert Phillips <[email protected]>
Date: Thu, 25 Jun 2026 13:21:59 +0000
Subject: [PATCH] Fix Ganesh stencil UMR

When, for a given OpsTask, the stencil ops are discard/store there was a possibility of uninitialized values to creep into the stencil buffer.

This CL reduces the cases in which discard will be used, mapping the problematic cases to clear/store.

Bug: b/502351526
Change-Id: Ic5d7e352d4c59426f103f2cfc39d5cf3fd879213
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1269136
Reviewed-by: Greg Daniel <[email protected]>
Commit-Queue: Robert Phillips <[email protected]>
---

diff --git a/gn/tests.gni b/gn/tests.gni
index ea33c23..2bb7714 100644
--- a/gn/tests.gni
+++ b/gn/tests.gni
@@ -513,6 +513,7 @@
   "$_tests/SlugTest.cpp",
   "$_tests/SmallPathRendererTest.cpp",
   "$_tests/SrcSrcOverBatchTest.cpp",
+  "$_tests/StencilClearTest.cpp",
   "$_tests/SurfaceDrawContextTest.cpp",
   "$_tests/SurfaceSemaphoreTest.cpp",
   "$_tests/TextBlobCacheTest.cpp",
diff --git a/src/gpu/ganesh/ops/OpsTask.cpp b/src/gpu/ganesh/ops/OpsTask.cpp
index 24ce928..6dbc866 100644
--- a/src/gpu/ganesh/ops/OpsTask.cpp
+++ b/src/gpu/ganesh/ops/OpsTask.cpp
@@ -589,7 +589,17 @@
     GrLoadOp stencilLoadOp;
     switch (fInitialStencilContent) {
         case StencilContent::kDontCare:
-            stencilLoadOp = GrLoadOp::kDiscard;
+            if (stencil && !caps.performStencilClearsAsDraws()) {
+                // This OpTask has a stencil, doesn't care about its contents,
+                // isn't clearing it with draws, and is going to store the result.
+                // In that case, we proactively clear it so that uninitialized data won't
+                // creep into the stencil buffer.
+                stencilLoadOp = GrLoadOp::kClear;
+            } else {
+                // This should only intentionally happen for the AtlasRenderTask which
+                // immediately inserts a clear.
+                stencilLoadOp = GrLoadOp::kDiscard;
+            }
             break;
         case StencilContent::kUserBitsCleared:
             SkASSERT(!caps.performStencilClearsAsDraws());
@@ -624,6 +634,7 @@
     // their store op might be "discard", and we currently make the assumption that a discard will
     // not invalidate what's already in main memory. This is probably ok for now, but certainly
     // something we want to address soon.
+    // b/160958008 forces discardStencilValuesAfterRenderPass to always return false.
     GrStoreOp stencilStoreOp = (caps.discardStencilValuesAfterRenderPass() && !fMustPreserveStencil)
             ? GrStoreOp::kDiscard
             : GrStoreOp::kStore;
@@ -647,6 +658,16 @@
     if (markStencilCleared) {
         stencil->markHasPerformedInitialClear();
     }
+
+#if defined(SK_DEBUG)
+    if (stencilLoadOp == GrLoadOp::kDiscard) {
+        // The only time we should have a stencil discard load-op is when either:
+        //    there is no stencil buffer
+        //    or stencil clears are being performed by draws
+        SkASSERT(!stencil || caps.performStencilClearsAsDraws());
+    }
+#endif
+
     flushState->setOpsRenderPass(renderPass);
     renderPass->begin();
 
@@ -736,10 +757,10 @@
         fTotalBounds.join(toMerge->fTotalBounds);
         fRenderPassXferBarriers |= toMerge->fRenderPassXferBarriers;
         if (fInitialStencilContent == StencilContent::kDontCare) {
-            // Propogate the first stencil content that isn't kDontCare.
+            // Propagate the first stencil content that isn't kDontCare.
             //
             // Once the stencil has any kind of initial content that isn't kDontCare, then the
-            // inital contents of subsequent opsTasks that get merged in don't matter.
+            // initial contents of subsequent opsTasks that get merged in don't matter.
             //
             // (This works because the opsTask all target the same render target and are in
             // painter's order. kPreserved obviously happens automatically with a merge, and kClear
diff --git a/tests/StencilClearTest.cpp b/tests/StencilClearTest.cpp
new file mode 100644
index 0000000..6bb1155
--- /dev/null
+++ b/tests/StencilClearTest.cpp
@@ -0,0 +1,131 @@
+/*
+ * Copyright 2026 Google LLC
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "include/core/SkCanvas.h"
+#include "include/core/SkPaint.h"
+#include "include/core/SkPathBuilder.h"
+#include "include/core/SkSurface.h"
+#include "include/gpu/ganesh/GrDirectContext.h"
+#include "include/gpu/ganesh/SkSurfaceGanesh.h"
+#include "tests/CtsEnforcement.h"
+#include "tests/Test.h"
+
+namespace {
+
+SkPath make_star() {
+    SkPathBuilder starPath;
+    starPath.moveTo(0.0f, -33.3333f);
+    starPath.lineTo(9.62f, -16.6667f);
+    starPath.lineTo(28.867f, -16.6667f);
+    starPath.lineTo(19.24f, 0.0f);
+    starPath.lineTo(28.867f, 16.6667f);
+    starPath.lineTo(9.62f, 16.6667f);
+    starPath.lineTo(0.0f, 33.3333f);
+    starPath.lineTo(-9.62f, 16.6667f);
+    starPath.lineTo(-28.867f, 16.6667f);
+    starPath.lineTo(-19.24f, 0.0f);
+    starPath.lineTo(-28.867f, -16.6667f);
+    starPath.lineTo(-9.62f, -16.6667f);
+    starPath.close();
+
+    return starPath.detach();
+}
+
+sk_sp<SkSurface> gpu_surface(GrDirectContext* dContext) {
+    SkSurfaceProps props(SkSurfaceProps::kDynamicMSAA_Flag, kUnknown_SkPixelGeometry);
+
+    SkImageInfo ii = SkImageInfo::Make(256, 256, kRGBA_8888_SkColorType, kPremul_SkAlphaType);
+    return SkSurfaces::RenderTarget(dContext,
+                                    skgpu::Budgeted::kYes,
+                                    ii,
+                                    /* sampleCount= */ 4,
+                                    kTopLeft_GrSurfaceOrigin,
+                                    &props,
+                                    /*shouldCreateWithMips=*/true);
+}
+
+static void disable_split_reduction(GrContextOptions* options) {
+    options->fReduceOpsTaskSplitting = GrContextOptions::Enable::kNo;
+}
+
+} // anonymous namespace
+
+// This test exercises b/502351526.
+// The ultimate goal is to:
+//    create an OpsTask (O1) that uses a stencil buffer (S) and clears it
+//    create a new OpsTask (O2) that doesn't use a stencil
+//    later, get O2 to add a stencil (and, specifically, reuse S)
+// In the bug this will allow uninitialized values to creep into the
+// stencil buffer and, potentially, impact later stencil-based rendering.
+// When executed this test will trigger an assert if the bug reoccurs.
+DEF_GANESH_TEST_FOR_CONTEXTS(StencilClearTest,
+                             skgpu::IsRenderingContext,
+                             reporter,
+                             ctxInfo,
+                             disable_split_reduction,
+                             CtsEnforcement::kNextRelease) {
+    GrDirectContext* dContext = ctxInfo.directContext();
+
+    SkPath star = make_star();
+
+    // This first draw clears the stencil buffer and fills it with the first star.
+    // The OpsTask (O1) has clear/store stencilOps and marks the stencil buffer as cleared.
+    sk_sp<SkSurface> s1 = gpu_surface(dContext);
+    if (!s1) {
+        return;  // Dynamic MSAA isn't supported everywhere
+    }
+
+    {
+        SkCanvas* canvas = s1->getCanvas();
+
+        SkPaint paint;
+        paint.setColor(SK_ColorBLUE);
+        paint.setAntiAlias(true);
+
+        canvas->concat(SkMatrix::ScaleTranslate(4, 4, 128, 128));
+        canvas->drawPath(star, paint);
+    }
+
+    // This starts a new Task that doesn't (yet) need stencil.
+    // Previously, this OpsTask (O2) would have discard/store stencilOps. With
+    // the fix this will now have clear/store stencilOps.
+    sk_sp<SkSurface> s2 = gpu_surface(dContext);
+    {
+        SkCanvas* canvas = s2->getCanvas();
+
+        SkPaint paint;
+        paint.setColor(SK_ColorRED);
+        canvas->drawRect(SkRect::MakeWH(256, 256), paint);
+    }
+
+    // This block just serves to close the active Task from s2