Fix memory leak in SkImageFilter

In our current implementation of SkImageFilterCache, when the
removeInternal() function is called, the Value is removed, but their
corresponding keys are not always removed in SkImageFilter. That could
result in memory leak.

In this CL, we made changes such that the Value structure now keeps
a pointer to the SkImageFilter. Each time when the removeInternal()
is called, we ask the SkImageFilter to remove the associated keys.

Bug: 689740
Change-Id: I0807fa3581881ad1530536df5289e3976792281f
Reviewed-on: https://skia-review.googlesource.com/20960
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Mike Reed <reed@google.com>
Reviewed-by: Stephen White <senorblanco@chromium.org>
Reviewed-by: Mike Klein <mtklein@chromium.org>

3 files changed, 34 insertions, 6 deletions

diff --git a/src/core/SkImageFilter.cpp b/src/core/SkImageFilter.cpp
index 3c3aa18056..cc0f1e9f81 100644
--- a/src/core/SkImageFilter.cpp
+++ b/src/core/SkImageFilter.cpp
@@ -221,7 +221,7 @@ sk_sp<SkSpecialImage> SkImageFilter::filterImage(SkSpecialImage* src, const Cont
 #endif
 
     if (result && context.cache()) {
-        context.cache()->set(key, result.get(), *offset);
+        context.cache()->set(key, result.get(), *offset, this);
         SkAutoMutexAcquire mutex(fMutex);
         fCacheKeys.push_back(key);
     }
@@ -229,6 +229,23 @@ sk_sp<SkSpecialImage> SkImageFilter::filterImage(SkSpecialImage* src, const Cont
     return result;
 }
 
+void SkImageFilter::removeKey(const SkImageFilterCacheKey& key) const {
+    SkAutoMutexAcquire mutex(fMutex);
+    for (int i = 0; i < fCacheKeys.count(); i++) {
+        if (fCacheKeys[i] == key) {
+            fCacheKeys.removeShuffle(i);
+            break;
+        }
+    }
+#ifdef SK_DEBUG
+    for (int i = 0; i < fCacheKeys.count(); i++) {
+        if (fCacheKeys[i] == key) {
+            SkASSERT(false);
+        }
+    }
+#endif
+}
+
 SkIRect SkImageFilter::filterBounds(const SkIRect& src, const SkMatrix& ctm,
                                  MapDirection direction) const {
     if (kReverse_MapDirection == direction) {
diff --git a/src/core/SkImageFilterCache.cpp b/src/core/SkImageFilterCache.cpp
index 0d9e367790..51249ef962 100644
--- a/src/core/SkImageFilterCache.cpp
+++ b/src/core/SkImageFilterCache.cpp
@@ -7,6 +7,7 @@
 
 #include "SkImageFilterCache.h"
 
+#include "SkImageFilter.h"
 #include "SkMutex.h"
 #include "SkOnce.h"
 #include "SkOpts.h"
@@ -37,12 +38,13 @@ public:
         }
     }
     struct Value {
-        Value(const Key& key, SkSpecialImage* image, const SkIPoint& offset)
-            : fKey(key), fImage(SkRef(image)), fOffset(offset) {}
+        Value(const Key& key, SkSpecialImage* image, const SkIPoint& offset, const SkImageFilter* filter)
+            : fKey(key), fImage(SkRef(image)), fOffset(offset), fFilter(filter) {}
 
         Key fKey;
         sk_sp<SkSpecialImage> fImage;
         SkIPoint fOffset;
+        const SkImageFilter* fFilter;
         static const Key& GetKey(const Value& v) {
             return v.fKey;
         }
@@ -65,12 +67,12 @@ public:
         return nullptr;
     }
 
-    void set(const Key& key, SkSpecialImage* image, const SkIPoint& offset) override {
+    void set(const Key& key, SkSpecialImage* image, const SkIPoint& offset, const SkImageFilter* filter) override {
         SkAutoMutexAcquire mutex(fMutex);
         if (Value* v = fLookup.find(key)) {
             this->removeInternal(v);
         }
-        Value* v = new Value(key, image, offset);
+        Value* v = new Value(key, image, offset, filter);
         fLookup.add(v);
         fLRU.addToHead(v);
         fCurrentBytes += image->getSize();
@@ -95,8 +97,13 @@ public:
 
     void purgeByKeys(const Key keys[], int count) override {
         SkAutoMutexAcquire mutex(fMutex);
+        // This function is only called in the destructor of SkImageFilter.
+        // Because the destructor will destroy the fCacheKeys anyway, we set the
+        // filter to be null so that removeInternal() won't call the
+        // SkImageFilter::removeKey() function.
         for (int i = 0; i < count; i++) {
             if (Value* v = fLookup.find(keys[i])) {
+                v->fFilter = nullptr;
                 this->removeInternal(v);
             }
         }
@@ -106,6 +113,9 @@ public:
 private:
     void removeInternal(Value* v) {
         SkASSERT(v->fImage);
+        if (v->fFilter) {
+            v->fFilter->removeKey(v->fKey);
+        }
         fCurrentBytes -= v->fImage->getSize();
         fLRU.remove(v);
         fLookup.remove(v->fKey);
diff --git a/src/core/SkImageFilterCache.h b/src/core/SkImageFilterCache.h
index 8f0d9f10fc..115b41370e 100644
--- a/src/core/SkImageFilterCache.h
+++ b/src/core/SkImageFilterCache.h
@@ -12,6 +12,7 @@
 #include "SkRefCnt.h"
 
 struct SkIPoint;
+class SkImageFilter;
 class SkSpecialImage;
 
 struct SkImageFilterCacheKey {
@@ -55,7 +56,7 @@ public:
     static SkImageFilterCache* Get();
     virtual sk_sp<SkSpecialImage> get(const SkImageFilterCacheKey& key, SkIPoint* offset) const = 0;
     virtual void set(const SkImageFilterCacheKey& key, SkSpecialImage* image,
-                     const SkIPoint& offset) = 0;
+                     const SkIPoint& offset, const SkImageFilter* filter) = 0;
     virtual void purge() = 0;
     virtual void purgeByKeys(const SkImageFilterCacheKey[], int) = 0;
     SkDEBUGCODE(virtual int count() const = 0;)