Fix dangling pointers when Ganesh culls CCPR Ops early

BUG=chromium:775868

Change-Id: I0066e34fd8ebe4b46ad72481f5bb955dc0dd5910
Reviewed-on: https://skia-review.googlesource.com/67682
Commit-Queue: Chris Dalton <csmartdalton@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>

src/gpu/ccpr/GrCoverageCountingPathRenderer.cpp

@@ -104,6 +104,7 @@ GrCoverageCountingPathRenderer::DrawPathsOp::DrawPathsOp(GrCoverageCountingPathR
         , fProcessors(std::move(args.fPaint))
         , fTailDraw(&fHeadDraw)
         , fOwningRTPendingOps(nullptr) {
+    SkDEBUGCODE(++fCCPR->fPendingDrawOpsCount);
     SkDEBUGCODE(fBaseInstance = -1);
     SkDEBUGCODE(fDebugInstanceCount = 1;)
     SkDEBUGCODE(fDebugSkippedInstances = 0;)
@@ -139,20 +140,34 @@ GrCoverageCountingPathRenderer::DrawPathsOp::DrawPathsOp(GrCoverageCountingPathR
     this->setBounds(devBounds, GrOp::HasAABloat::kYes, GrOp::IsZeroArea::kNo);
 }
 
+GrCoverageCountingPathRenderer::DrawPathsOp::~DrawPathsOp() {
+    if (fOwningRTPendingOps) {
+        // Remove CCPR's dangling pointer to this Op before deleting it.
+        SkASSERT(!fCCPR->fFlushing);
+        fOwningRTPendingOps->fOpList.remove(this);
+    }
+    SkDEBUGCODE(--fCCPR->fPendingDrawOpsCount);
+}
+
 GrDrawOp::RequiresDstTexture DrawPathsOp::finalize(const GrCaps& caps, const GrAppliedClip* clip,
                                                    GrPixelConfigIsClamped dstIsClamped) {
-    SingleDraw& onlyDraw = this->getOnlyPathDraw();
+    SkASSERT(!fCCPR->fFlushing);
+    // There should only be one single path draw in this Op right now.
+    SkASSERT(1 == fDebugInstanceCount);
+    SkASSERT(&fHeadDraw == fTailDraw);
     GrProcessorSet::Analysis analysis = fProcessors.finalize(
-            onlyDraw.fColor, GrProcessorAnalysisCoverage::kSingleChannel, clip, false, caps,
+            fHeadDraw.fColor, GrProcessorAnalysisCoverage::kSingleChannel, clip, false, caps,
-            dstIsClamped, &onlyDraw.fColor);
+            dstIsClamped, &fHeadDraw.fColor);
     return analysis.requiresDstTexture() ? RequiresDstTexture::kYes : RequiresDstTexture::kNo;
 }
 
 bool DrawPathsOp::onCombineIfPossible(GrOp* op, const GrCaps& caps) {
     DrawPathsOp* that = op->cast<DrawPathsOp>();
     SkASSERT(fCCPR == that->fCCPR);
+    SkASSERT(!fCCPR->fFlushing);
     SkASSERT(fOwningRTPendingOps);
     SkASSERT(fDebugInstanceCount);
+    SkASSERT(!that->fOwningRTPendingOps || that->fOwningRTPendingOps == fOwningRTPendingOps);
     SkASSERT(that->fDebugInstanceCount);
 
     if (this->getFillType() != that->getFillType() ||
@@ -161,20 +176,8 @@ bool DrawPathsOp::onCombineIfPossible(GrOp* op, const GrCaps& caps) {
         return false;
     }
 
-    if (RTPendingOps* owningRTPendingOps = that->fOwningRTPendingOps) {
-        SkASSERT(owningRTPendingOps == fOwningRTPendingOps);
-        owningRTPendingOps->fOpList.remove(that);
-    } else {
-        // The Op is being combined immediately after creation, before a call to wasRecorded. In
-        // this case wasRecorded will not be called. So we count its path here instead.
-        const SingleDraw& onlyDraw = that->getOnlyPathDraw();
-        ++fOwningRTPendingOps->fNumTotalPaths;
-        fOwningRTPendingOps->fNumSkPoints += onlyDraw.fPath.countPoints();
-        fOwningRTPendingOps->fNumSkVerbs += onlyDraw.fPath.countVerbs();
-    }
-
     fTailDraw->fNext = &fOwningRTPendingOps->fDrawsAllocator.push_back(that->fHeadDraw);
-    fTailDraw = that->fTailDraw == &that->fHeadDraw ? fTailDraw->fNext : that->fTailDraw;
+    fTailDraw = (that->fTailDraw == &that->fHeadDraw) ? fTailDraw->fNext : that->fTailDraw;
 
     this->joinBounds(*that);
 
@@ -184,12 +187,9 @@ bool DrawPathsOp::onCombineIfPossible(GrOp* op, const GrCaps& caps) {
 }
 
 void DrawPathsOp::wasRecorded(GrRenderTargetOpList* opList) {
+    SkASSERT(!fCCPR->fFlushing);
     SkASSERT(!fOwningRTPendingOps);
-    const SingleDraw& onlyDraw = this->getOnlyPathDraw();
     fOwningRTPendingOps = &fCCPR->fRTPendingOpsMap[opList->uniqueID()];
-    ++fOwningRTPendingOps->fNumTotalPaths;
-    fOwningRTPendingOps->fNumSkPoints += onlyDraw.fPath.countPoints();
-    fOwningRTPendingOps->fNumSkVerbs += onlyDraw.fPath.countVerbs();
     fOwningRTPendingOps->fOpList.addToTail(this);
 }
 
@@ -224,22 +224,29 @@ void GrCoverageCountingPathRenderer::setupPerFlushResources(GrOnFlushResourcePro
 
     fPerFlushResourcesAreValid = false;
 
-    SkTInternalLList<DrawPathsOp> flushingOps;
+    // Gather the Ops that are being flushed.
     int maxTotalPaths = 0, numSkPoints = 0, numSkVerbs = 0;
-
+    SkTInternalLList<DrawPathsOp> flushingOps;
     for (int i = 0; i < numOpListIDs; ++i) {
         auto it = fRTPendingOpsMap.find(opListIDs[i]);
-        if (fRTPendingOpsMap.end() != it) {
+        if (fRTPendingOpsMap.end() == it) {
-            RTPendingOps& rtPendingOps = it->second;
+            continue;
-            SkASSERT(!rtPendingOps.fOpList.isEmpty());
-            flushingOps.concat(std::move(rtPendingOps.fOpList));
-            maxTotalPaths += rtPendingOps.fNumTotalPaths;
-            numSkPoints += rtPendingOps.fNumSkPoints;
-            numSkVerbs += rtPendingOps.fNumSkVerbs;
         }
+        SkTInternalLList<DrawPathsOp>::Iter iter;
+        SkTInternalLList<DrawPathsOp>& rtFlushingOps = it->second.fOpList;
+        iter.init(rtFlushingOps, SkTInternalLList<DrawPathsOp>::Iter::kHead_IterStart);
+        while (DrawPathsOp* flushingOp = iter.get()) {
+            for (const auto* draw = &flushingOp->fHeadDraw; draw; draw = draw->fNext) {
+                ++maxTotalPaths;
+                numSkPoints += draw->fPath.countPoints();
+                numSkVerbs += draw->fPath.countVerbs();
+            }
+            flushingOp->fOwningRTPendingOps = nullptr; // Owner is about to change to 'flushingOps'.
+            iter.next();
+        }
+        flushingOps.concat(std::move(rtFlushingOps));
     }
 
-    SkASSERT(flushingOps.isEmpty() == !maxTotalPaths);
     if (flushingOps.isEmpty()) {
         return; // Nothing to draw.
     }
@@ -376,6 +383,8 @@ void DrawPathsOp::onExecute(GrOpFlushState* flushState) {
         return; // Setup failed.
     }
 
+    SkASSERT(fBaseInstance >= 0); // Make sure setupPerFlushResources has set us up.
+
     GrPipeline::InitArgs initArgs;
     initArgs.fFlags = fSRGBFlags;
     initArgs.fProxy = flushState->drawOpArgs().fProxy;

src/gpu/ccpr/GrCoverageCountingPathRenderer.h

@@ -35,6 +35,12 @@ public:
     static sk_sp<GrCoverageCountingPathRenderer> CreateIfSupported(const GrCaps&,
                                                                    bool drawCachablePaths);
 
+    ~GrCoverageCountingPathRenderer() override {
+        // Ensure nothing exists that could have a dangling pointer back into this class.
+        SkASSERT(fRTPendingOpsMap.empty());
+        SkASSERT(0 == fPendingDrawOpsCount);
+    }
+
     // GrPathRenderer overrides.
     StencilSupport onGetStencilSupport(const GrShape&) const override {
         return GrPathRenderer::kNoSupport_StencilSupport;
@@ -55,6 +61,7 @@ public:
         SK_DECLARE_INTERNAL_LLIST_INTERFACE(DrawPathsOp);
 
         DrawPathsOp(GrCoverageCountingPathRenderer*, const DrawPathArgs&, GrColor);
+        ~DrawPathsOp() override;
 
         const char* name() const override { return "GrCoverageCountingPathRenderer::DrawPathsOp"; }
 
@@ -87,12 +94,6 @@ public:
             SingleDraw*   fNext = nullptr;
         };
 
-        SingleDraw& getOnlyPathDraw() {
-            SkASSERT(&fHeadDraw == fTailDraw);
-            SkASSERT(1 == fDebugInstanceCount);
-            return fHeadDraw;
-        }
-
         struct AtlasBatch {
             const GrCCPRAtlas*   fAtlas;
             int                  fEndInstanceIdx;
@@ -130,14 +131,12 @@ private:
 
     struct RTPendingOps {
         SkTInternalLList<DrawPathsOp>                 fOpList;
-        int                                           fNumTotalPaths = 0;
-        int                                           fNumSkPoints = 0;
-        int                                           fNumSkVerbs = 0;
         GrSTAllocator<256, DrawPathsOp::SingleDraw>   fDrawsAllocator;
     };
 
     // Map from render target ID to the individual render target's pending path ops.
     std::map<uint32_t, RTPendingOps>   fRTPendingOpsMap;
+    SkDEBUGCODE(int                    fPendingDrawOpsCount = 0;)
 
     sk_sp<GrBuffer>                    fPerFlushIndexBuffer;
     sk_sp<GrBuffer>                    fPerFlushVertexBuffer;

tests/GrCCPRTest.cpp

@@ -154,6 +154,28 @@ class GrCCPRTest_cleanup : public CCPRTest {
 };
 DEF_CCPR_TEST(GrCCPRTest_cleanup)
 
+class GrCCPRTest_unregisterCulledOps : public CCPRTest {
+    void onRun(skiatest::Reporter* reporter, CCPRPathDrawer& ccpr) override {
+        REPORTER_ASSERT(reporter, SkPathPriv::TestingOnly_unique(fPath));
+
+        // Ensure Ops get unregistered from CCPR when culled early.
+        ccpr.drawPath(fPath);
+        REPORTER_ASSERT(reporter, !SkPathPriv::TestingOnly_unique(fPath));
+        ccpr.clear(); // Clear should delete the CCPR Op.
+        REPORTER_ASSERT(reporter, SkPathPriv::TestingOnly_unique(fPath));
+        ccpr.flush(); // Should not crash (DrawPathsOp should have unregistered itself).
+
+        // Ensure Op unregisters work when we delete the context without flushing.
+        ccpr.drawPath(fPath);
+        REPORTER_ASSERT(reporter, !SkPathPriv::TestingOnly_unique(fPath));
+        ccpr.clear(); // Clear should delete the CCPR DrawPathsOp.
+        REPORTER_ASSERT(reporter, SkPathPriv::TestingOnly_unique(fPath));
+        ccpr.abandonGrContext();
+        fMockContext.reset(); // Should not crash (DrawPathsOp should have unregistered itself).
+    }
+};
+DEF_CCPR_TEST(GrCCPRTest_unregisterCulledOps)
+
 class CCPRRenderingTest {
 public:
     void run(skiatest::Reporter* reporter, GrContext* ctx) const {