commit f30e49310fb8927d1acbfd39f6bdc5513aa7daa1
author Chris Dalton <csmartdalton@google.com> Thu Oct 18 18:56:59 2018 -0600
committer Skia Commit-Bot <skia-commit-bot@chromium.org> Fri Oct 19 01:59:27 2018 +0000
tree 11c7b25456d8363ca22d02d3d8423899164bd90a
parent 0f7e55a818f4661309871f2db2fe2c2196135bda

ccpr: Unregister path listeners when their cache entries are evicted

Bug: skia:8452
Change-Id: I5cf63c07481db38fc37e920e04ca140bad8966e4
Reviewed-on: https://skia-review.googlesource.com/c/163560
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Chris Dalton <csmartdalton@google.com>

include/private/SkPathRef.h

diff --git a/include/private/SkPathRef.h b/include/private/SkPathRef.h
index 92568d8..49cc28f 100644
--- a/include/private/SkPathRef.h
+++ b/include/private/SkPathRef.h
@@ -15,6 +15,7 @@
 #include "SkRRect.h"
 #include "SkRect.h"
 #include "SkRefCnt.h"
+#include "SkTArray.h"
 #include "SkTDArray.h"
 #include "SkTemplates.h"
 #include "SkTo.h"
@@ -309,9 +310,24 @@
      */
     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_relaxed);
+        }
+
+    private:
+        mutable std::atomic<bool> fShouldUnregisterFromPath;
     };
 
     void addGenIDChangeListener(sk_sp<GenIDChangeListener>);  // Threadsafe.
@@ -545,8 +561,8 @@
     mutable uint32_t    fGenerationID;
     SkDEBUGCODE(int32_t fEditorsAttached;) // assert that only one editor in use at any time.
 
-    SkMutex                         fGenIDChangeListenersMutex;
-    SkTDArray<GenIDChangeListener*> fGenIDChangeListeners;  // pointers are reffed
+    SkMutex fGenIDChangeListenersMutex;
+    SkTArray<sk_sp<GenIDChangeListener>> fGenIDChangeListeners;
 
     mutable uint8_t  fBoundsIsDirty;
     mutable bool     fIsFinite;    // only meaningful if bounds are valid

src/core/SkPathRef.cpp

diff --git a/src/core/SkPathRef.cpp b/src/core/SkPathRef.cpp
index f767323..85d1b9c 100644
--- a/src/core/SkPathRef.cpp
+++ b/src/core/SkPathRef.cpp
@@ -707,19 +707,31 @@
     if (nullptr == listener || this == gEmpty) {
         return;
     }
+
     SkAutoMutexAcquire lock(fGenIDChangeListenersMutex);
-    *fGenIDChangeListeners.append() = listener.release();
+
+    // Clean out any stale listeners before we append the new one.
+    for (int i = 0; i < fGenIDChangeListeners.count(); ++i) {
+        if (fGenIDChangeListeners[i]->shouldUnregisterFromPath()) {
+            fGenIDChangeListeners.removeShuffle(i--);  // No need to preserve the order after i.
+        }
+    }
+
+    SkASSERT(!listener->shouldUnregisterFromPath());
+    fGenIDChangeListeners.push_back(std::move(listener));
 }
 
 // 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 (const sk_sp<GenIDChangeListener>& listener : fGenIDChangeListeners) {
+        if (!listener->shouldUnregisterFromPath()) {
+            listener->onChange();
+        }
     }
 
     // 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

diff --git a/src/gpu/ccpr/GrCCPathCache.cpp b/src/gpu/ccpr/GrCCPathCache.cpp
index db85641..32008c2 100644
--- a/src/gpu/ccpr/GrCCPathCache.cpp
+++ b/src/gpu/ccpr/GrCCPathCache.cpp
@@ -187,12 +187,15 @@
 }
 
 void GrCCPathCache::evict(GrCCPathCacheEntry* entry) {
-    bool isInCache = entry->fNext || (fLRU.tail() == entry);
-    SkASSERT(isInCache == fLRU.isInList(entry));
-    if (isInCache) {
-        fLRU.remove(entry);
-        fHashTable.remove(HashNode::GetKey(entry));  // Do this last, as it might delete the entry.
+    // Has the entry already been evicted? (SkPaths can post eviction messages from any thread.)
+    if (entry->shouldUnregisterFromPath()) {
+        SkASSERT(!fLRU.isInList(entry));
+        return;
     }
+
+    entry->markShouldUnregisterFromPath();
+    fLRU.remove(entry);
+    fHashTable.remove(HashNode::GetKey(entry));  // Do this last, as it might delete the entry.
 }
 
 void GrCCPathCache::purgeAsNeeded() {