From b7170fe3fe0d8933de57968222bab95ef6615a5a Mon Sep 17 00:00:00 2001
From: Sergio Giro <sgiro@google.com>
Date: Tue, 17 Nov 2015 11:52:05 +0000
Subject: [PATCH] libutils: fix cache removal when callback invalidates the key

Bug: 24785286
Change-Id: I9d17d2247258a56ef7776b3a701389e825a9c533
---
 include/utils/LruCache.h         |  2 +-
 libutils/tests/LruCache_test.cpp | 38 ++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/include/utils/LruCache.h b/include/utils/LruCache.h
index 1741fb2e4..ed96fe4ff 100644
--- a/include/utils/LruCache.h
+++ b/include/utils/LruCache.h
@@ -200,11 +200,11 @@ bool LruCache<TKey, TValue>::remove(const TKey& key) {
         return false;
     }
     Entry* entry = *find_result;
+    mSet->erase(entry);
     if (mListener) {
         (*mListener)(entry->key, entry->value);
     }
     detachFromCache(*entry);
-    mSet->erase(entry);
     delete entry;
     return true;
 }
diff --git a/libutils/tests/LruCache_test.cpp b/libutils/tests/LruCache_test.cpp
index 580b98058..dd95c576d 100644
--- a/libutils/tests/LruCache_test.cpp
+++ b/libutils/tests/LruCache_test.cpp
@@ -73,6 +73,13 @@ struct ComplexValue {
 
 ssize_t ComplexValue::instanceCount = 0;
 
+struct KeyWithPointer {
+    int *ptr;
+    bool operator ==(const KeyWithPointer& other) const {
+        return *ptr == *other.ptr;
+    }
+};
+
 } // namespace
 
 
@@ -84,6 +91,10 @@ template<> inline android::hash_t hash_type(const ComplexKey& value) {
     return hash_type(value.k);
 }
 
+template<> inline android::hash_t hash_type(const KeyWithPointer& value) {
+    return hash_type(*value.ptr);
+}
+
 class EntryRemovedCallback : public OnEntryRemoved<SimpleKey, StringValue> {
 public:
     EntryRemovedCallback() : callbackCount(0), lastKey(-1), lastValue(NULL) { }
@@ -98,6 +109,14 @@ public:
     StringValue lastValue;
 };
 
+class InvalidateKeyCallback : public OnEntryRemoved<KeyWithPointer, StringValue> {
+public:
+    void operator()(KeyWithPointer& k, StringValue&) {
+        delete k.ptr;
+        k.ptr = nullptr;
+    }
+};
+
 class LruCacheTest : public testing::Test {
 protected:
     virtual void SetUp() {
@@ -293,6 +312,25 @@ TEST_F(LruCacheTest, CallbackOnClear) {
     EXPECT_EQ(3, callback.callbackCount);
 }
 
+TEST_F(LruCacheTest, CallbackRemovesKeyWorksOK) {
+    LruCache<KeyWithPointer, StringValue> cache(1);
+    InvalidateKeyCallback callback;
+    cache.setOnEntryRemovedListener(&callback);
+    KeyWithPointer key1;
+    key1.ptr = new int(1);
+    KeyWithPointer key2;
+    key2.ptr = new int(2);
+
+    cache.put(key1, "one");
+    // As the size of the cache is 1, the put will call the callback.
+    // Make sure everything goes smoothly even if the callback invalidates
+    // the key (b/24785286)
+    cache.put(key2, "two");
+    EXPECT_EQ(1U, cache.size());
+    EXPECT_STREQ("two", cache.get(key2));
+    cache.clear();
+}
+
 TEST_F(LruCacheTest, IteratorCheck) {
     LruCache<int, int> cache(100);