Sever fOriginalPath connection whenever a GrShape becomes a simple type

When drawing a round-rect, for example, we may end up in drawPath with a
temporary path that was created with the rrect added. We ended up putting
a genID listener on that (stack allocated) path, so we would evict cache
entries immediately. This restores the old behavior, where cache entries
for paths derived from simple types are never invalidated.

Bug: skia:7087
Change-Id: I3eed9c3a289241bfe1e42036be3362f592256693
Reviewed-on: https://skia-review.googlesource.com/54460
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>

3 files changed, 60 insertions, 6 deletions

diff --git a/src/gpu/GrShape.cpp b/src/gpu/GrShape.cpp
index 9636427c8e..91cd3c8eb1 100644
--- a/src/gpu/GrShape.cpp
+++ b/src/gpu/GrShape.cpp
@@ -386,7 +386,6 @@ GrShape::GrShape(const GrShape& parent, GrStyle::Apply apply, SkScalar scale) {
                                                  scale)) {
             tmpParent.init(*srcForPathEffect, GrStyle(strokeRec, nullptr));
             *this = tmpParent.get()->applyStyle(apply, scale);
-            fOriginalPath = parent.fOriginalPath;
             return;
         }
         // A path effect has access to change the res scale but we aren't expecting it to and it
@@ -498,6 +497,10 @@ void GrShape::attemptToSimplifyPath() {
     }
     if (Type::kPath != fType) {
         fInheritedKey.reset(0);
+        // Whenever we simplify to a non-path, break the chain so we no longer refer to the
+        // original path. This prevents attaching genID listeners to temporary paths created when
+        // drawing simple shapes.
+        fOriginalPath.reset();
         if (Type::kRRect == fType) {
             this->attemptToSimplifyRRect();
         } else if (Type::kLine == fType) {
diff --git a/src/gpu/GrShape.h b/src/gpu/GrShape.h
index dd8aa850d6..032c4d5e57 100644
--- a/src/gpu/GrShape.h
+++ b/src/gpu/GrShape.h
@@ -373,9 +373,11 @@ public:
     void addGenIDChangeListener(SkPathRef::GenIDChangeListener* listener) const;
 
     /**
-     * Gets the generation ID of the *original* path. This is only exposed for unit tests.
+     * Helpers that are only exposed for unit tests, to determine if the shape is a path, and get
+     * the generation ID of the *original* path.
      */
     uint32_t testingOnly_getOriginalGenerationID() const;
+    bool testingOnly_isPath() const;
 
 private:
 
diff --git a/tests/GrShapeTest.cpp b/tests/GrShapeTest.cpp
index 2b2a169477..259026c8d0 100644
--- a/tests/GrShapeTest.cpp
+++ b/tests/GrShapeTest.cpp
@@ -21,6 +21,10 @@ uint32_t GrShape::testingOnly_getOriginalGenerationID() const {
     return fOriginalPath.getGenerationID();
 }
 
+bool GrShape::testingOnly_isPath() const {
+    return Type::kPath == fType;
+}
+
 using Key = SkTArray<uint32_t>;
 
 static bool make_key(Key* key, const GrShape& shape) {
@@ -213,6 +217,54 @@ static void check_equivalence(skiatest::Reporter* r, const GrShape& a, const GrS
     REPORTER_ASSERT(r, ignoreInversenessDifference || a.inverseFilled() == b.inverseFilled());
 }
 
+static void check_original_path_ids(skiatest::Reporter* r, const GrShape& base, const GrShape& pe,
+                                    const GrShape& peStroke, const GrShape& full) {
+    bool baseIsPath = base.testingOnly_isPath();
+    bool peIsPath = pe.testingOnly_isPath();
+    bool peStrokeIsPath = peStroke.testingOnly_isPath();
+    bool fullIsPath = full.testingOnly_isPath();
+
+    REPORTER_ASSERT(r, peStrokeIsPath == fullIsPath);
+
+    uint32_t baseID = base.testingOnly_getOriginalGenerationID();
+    uint32_t peID = pe.testingOnly_getOriginalGenerationID();
+    uint32_t peStrokeID = peStroke.testingOnly_getOriginalGenerationID();
+    uint32_t fullID = full.testingOnly_getOriginalGenerationID();
+
+    // All empty paths have the same gen ID
+    uint32_t emptyID = SkPath().getGenerationID();
+
+    // If we started with a real path, then our genID should match that path's gen ID (and not be
+    // empty). If we started with a simple shape, our original path should have been reset.
+    REPORTER_ASSERT(r, baseIsPath == (baseID != emptyID));
+
+    // For the derived shapes, if they're simple types, their original paths should have been reset
+    REPORTER_ASSERT(r, peIsPath || (peID == emptyID));
+    REPORTER_ASSERT(r, peStrokeIsPath || (peStrokeID == emptyID));
+    REPORTER_ASSERT(r, fullIsPath || (fullID == emptyID));
+
+    if (!peIsPath) {
+        // If the path effect produces a simple shape, then there are no unbroken chains to test
+        return;
+    }
+
+    // From here on, we know that the path effect produced a shape that was a "real" path
+
+    if (baseIsPath) {
+        REPORTER_ASSERT(r, baseID == peID);
+    }
+
+    if (peStrokeIsPath) {
+        REPORTER_ASSERT(r, peID == peStrokeID);
+        REPORTER_ASSERT(r, peStrokeID == fullID);
+    }
+
+    if (baseIsPath && peStrokeIsPath) {
+        REPORTER_ASSERT(r, baseID == peStrokeID);
+        REPORTER_ASSERT(r, baseID == fullID);
+    }
+}
+
 void test_inversions(skiatest::Reporter* r, const GrShape& shape, const Key& shapeKey) {
     GrShape preserve = GrShape::MakeFilled(shape, GrShape::FillInversion::kPreserve);
     Key preserveKey;
@@ -499,10 +551,7 @@ private:
 
         // All shapes should report the same "original" path, so that path renderers can get to it
         // if necessary.
-        uint32_t baseGenID = fBase.testingOnly_getOriginalGenerationID();
-        REPORTER_ASSERT(r, baseGenID == fAppliedPE.testingOnly_getOriginalGenerationID());
-        REPORTER_ASSERT(r, baseGenID == fAppliedPEThenStroke.testingOnly_getOriginalGenerationID());
-        REPORTER_ASSERT(r, baseGenID == fAppliedFull.testingOnly_getOriginalGenerationID());
+        check_original_path_ids(r, fBase, fAppliedPE, fAppliedPEThenStroke, fAppliedFull);
 
         // Applying the path effect and then the stroke should always be the same as applying
         // both in one go.