ccpr: Unregister path listeners when their cache entries are evicted

Bug: skia:8452
Change-Id: Ibd49d8f0ed15c568156c09db358eba0415df48f5
Reviewed-on: https://skia-review.googlesource.com/c/165120
Reviewed-by: Brian Salomon <[email protected]>
Commit-Queue: Chris Dalton <[email protected]>

include/private/SkPathRef.h

@@ -309,9 +309,24 @@ class SK_API SkPathRef final : public SkNVRefCnt<SkPathRef> {
      */
     uint32_t genID() const;
 
-    struct GenIDChangeListener : SkRefCnt {
+    class GenIDChangeListener : public SkRefCnt {
+    public:
+        GenIDChangeListener() : fShouldUnregisterFromPath(false) {}
         virtual ~GenIDChangeListener() {}
+
         virtual void onChange() = 0;
+
+        // The caller can use this method to notify the path that it no longer needs to listen. Once
+        // called, the path will remove this listener from the list at some future point.
+        void markShouldUnregisterFromPath() {
+            fShouldUnregisterFromPath.store(true, std::memory_order_relaxed);
+        }
+        bool shouldUnregisterFromPath() {
+            return fShouldUnregisterFromPath.load(std::memory_order_acquire);
+        }
+
+    private:
+        std::atomic<bool> fShouldUnregisterFromPath;
     };
 
     void addGenIDChangeListener(sk_sp<GenIDChangeListener>);  // Threadsafe.

src/core/SkPathRef.cpp

@@ -93,6 +93,7 @@ SkPathRef::~SkPathRef() {
     // Deliberately don't validate() this path ref, otherwise there's no way
     // to read one that's not valid and then free its memory without asserting.
     this->callGenIDChangeListeners();
+    SkASSERT(fGenIDChangeListeners.empty());  // These are raw ptrs.
     sk_free(fPoints);
 
     SkDEBUGCODE(fPoints = nullptr;)
@@ -707,19 +708,33 @@ void SkPathRef::addGenIDChangeListener(sk_sp<GenIDChangeListener> listener) {
     if (nullptr == listener || this == gEmpty) {
         return;
     }
+
     SkAutoMutexAcquire lock(fGenIDChangeListenersMutex);
+
+    // Clean out any stale listeners before we append the new one.
+    for (int i = 0; i < fGenIDChangeListeners.count(); ++i) {
+        if (fGenIDChangeListeners[i]->shouldUnregisterFromPath()) {
+            fGenIDChangeListeners[i]->unref();
+            fGenIDChangeListeners.removeShuffle(i--);  // No need to preserve the order after i.
+        }
+    }
+
+    SkASSERT(!listener->shouldUnregisterFromPath());
     *fGenIDChangeListeners.append() = listener.release();
 }
 
 // we need to be called *before* the genID gets changed or zerod
 void SkPathRef::callGenIDChangeListeners() {
     SkAutoMutexAcquire lock(fGenIDChangeListenersMutex);
-    for (int i = 0; i < fGenIDChangeListeners.count(); i++) {
-        fGenIDChangeListeners[i]->onChange();
+    for (GenIDChangeListener* listener : fGenIDChangeListeners) {
+        if (!listener->shouldUnregisterFromPath()) {
+            listener->onChange();
+        }
+        // Listeners get at most one shot, so whether these triggered or not, blow them away.
+        listener->unref();
     }
 
-    // Listeners get at most one shot, so whether these triggered or not, blow them away.
-    fGenIDChangeListeners.unrefAll();
+    fGenIDChangeListeners.reset();
 }
 
 SkRRect SkPathRef::getRRect() const {

src/gpu/ccpr/GrCCPathCache.cpp

@@ -163,6 +163,7 @@ inline void GrCCPathCache::HashNode::willExitHashTable() {
     SkASSERT(fPathCache->fLRU.isInList(fEntry.get()));
 
     fEntry->invalidateAtlas();  // Must happen on graphics thread.
+    fEntry->markShouldUnregisterFromPath();
     fPathCache->fLRU.remove(fEntry.get());
 }
 
@@ -237,13 +238,13 @@ sk_sp<GrCCPathCacheEntry> GrCCPathCache::find(const GrShape& shape, const MaskTr
 }
 
 void GrCCPathCache::evict(GrCCPathCacheEntry* entry) {
-    SkASSERT(SkGetThreadID() == fGraphicsThreadID);
+    // Has the entry already been evicted? (We mark "shouldUnregisterFromPath" upon eviction.)
+    bool isInCache = !entry->shouldUnregisterFromPath();
+    SkDEBUGCODE(HashNode* entryKeyNode = fHashTable.find(HashNode::GetKey(entry)));
+    SkASSERT((entryKeyNode && entryKeyNode->entry() == entry) == isInCache);
+    SkASSERT(fLRU.isInList(entry) == isInCache);
 
-    bool isInCache = entry->fNext || (fLRU.tail() == entry);
-    SkASSERT(isInCache == fLRU.isInList(entry));
     if (isInCache) {
-        SkDEBUGCODE(HashNode* node = fHashTable.find(HashNode::GetKey(entry)));
-        SkASSERT(node && node->entry() == entry);
         fHashTable.remove(HashNode::GetKey(entry));
         // HashNode::willExitHashTable() takes care of the rest.
     }