Fix memory leaks in ExtensionPrefStore and its unit test.

Clarify comments describing the memory ownership model.

BUG=48980
TEST=valgrind and heapcheck bots no longer show related leaks
Review URL: http://codereview.chromium.org/2805094

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@53623 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 37 insertions, 81 deletions

diff --git a/chrome/browser/extensions/extension_pref_store.cc b/chrome/browser/extensions/extension_pref_store.cc
index 437ca77..7635151 100644
--- a/chrome/browser/extensions/extension_pref_store.cc
+++ b/chrome/browser/extensions/extension_pref_store.cc
@@ -5,7 +5,6 @@
 #include "chrome/browser/extensions/extension_pref_store.h"
 
 #include "base/logging.h"
-#include "base/stl_util-inl.h"
 #include "base/values.h"
 #include "chrome/browser/pref_service.h"
 
@@ -49,6 +48,9 @@ void ExtensionPrefStore::UpdateOnePref(const wchar_t* path) {
 }
 
 void ExtensionPrefStore::UpdatePrefs(const PrefValueMap* pref_values) {
+  if (!pref_values)
+    return;
+
   for (PrefValueMap::const_iterator i = pref_values->begin();
         i != pref_values->end(); ++i) {
     UpdateOnePref(i->first);
@@ -70,14 +72,14 @@ void ExtensionPrefStore::InstallExtensionPref(std::string extension_id,
   PrefValueMap* pref_values;
   if (i != extension_stack_.end()) {
     pref_values = (*i)->pref_values;
+    delete (*pref_values)[pref_path];
     (*pref_values)[pref_path] = pref_value;
   } else {
     pref_values = new PrefValueMap();
     (*pref_values)[pref_path] = pref_value;
 
-    ExtensionPrefs* extension_prefs = new ExtensionPrefs();
-    extension_prefs->extension_id = extension_id;
-    extension_prefs->pref_values = pref_values;
+    ExtensionPrefs* extension_prefs = new ExtensionPrefs(extension_id,
+                                                         pref_values);
     extension_stack_.push_front(extension_prefs);
   }
 
@@ -87,20 +89,16 @@ void ExtensionPrefStore::InstallExtensionPref(std::string extension_id,
 
 void ExtensionPrefStore::UninstallExtension(std::string extension_id) {
   // Remove this extension from the stack.
-  scoped_ptr<PrefValueMap> pref_values;
   for (ExtensionStack::iterator i = extension_stack_.begin();
       i != extension_stack_.end(); ++i) {
     if ((*i)->extension_id == extension_id) {
-      pref_values.reset((*i)->pref_values);
-      delete *i;
+      ExtensionPrefs* to_be_deleted = *i;
       extension_stack_.erase(i);
-      break;
+      UpdatePrefs(to_be_deleted->pref_values);
+      delete to_be_deleted;
+      return;
     }
   }
-  if (!pref_values.get())
-    return;
-
-  UpdatePrefs(pref_values.get());
 }
 
 void ExtensionPrefStore::GetExtensionIDs(std::vector<std::string>* result) {
diff --git a/chrome/browser/extensions/extension_pref_store.h b/chrome/browser/extensions/extension_pref_store.h
index fc10b5b..26c6c13 100644
--- a/chrome/browser/extensions/extension_pref_store.h
+++ b/chrome/browser/extensions/extension_pref_store.h
@@ -12,6 +12,7 @@
 
 #include "base/basictypes.h"
 #include "base/scoped_ptr.h"
+#include "base/stl_util-inl.h"
 #include "chrome/common/pref_store.h"
 
 class DictionaryValue;
@@ -35,6 +36,7 @@ class ExtensionPrefStore : public PrefStore {
 
   // Begins tracking the preference and value an extension wishes to set. This
   // must be called each time an extension API tries to set a preference.
+  // The ExtensionPrefStore will take ownership of the |pref_value|.
   virtual void InstallExtensionPref(std::string extension_id,
                                     const wchar_t* pref_path,
                                     Value* pref_value);
@@ -61,26 +63,34 @@ class ExtensionPrefStore : public PrefStore {
   typedef std::map<const wchar_t*, Value*> PrefValueMap;
 
   // Applies the highest-priority extension's setting for the given preference
-  // path, or clears the setting in this PrefStore if no extensions wish to
-  // control it.
+  // path to the |prefs_| store, or clears the setting there if no extensions
+  // wish to control it.
   void UpdateOnePref(const wchar_t* path);
 
-  // Updates each preference in the |pref_values| list.
+  // Updates each preference in the key set of the |pref_values| map.
   void UpdatePrefs(const PrefValueMap* pref_values);
 
   // A cache of the highest-priority values for each preference that any
   // extension is controlling, for quick read access. Owns the stored values.
   scoped_ptr<DictionaryValue> prefs_;
 
-  // Associates an extension ID with the prefs it sets.
+  // Associates an extension ID with the prefs it sets. Owns the pref values.
   struct ExtensionPrefs {
+    ExtensionPrefs(std::string id, PrefValueMap* values)
+        : extension_id(id),
+          pref_values(values) {}
+
+    ~ExtensionPrefs() {
+      STLDeleteValues(pref_values);
+      delete pref_values;
+    }
+
     std::string extension_id;
     PrefValueMap* pref_values;
   };
 
   // A pseudo-stack of extensions and their preferences. Extensions are always
-  // added to the head, but may be removed from the middle. This stack owns
-  // the values in the extensions' PrefValueMaps.
+  // added to the head, but may be removed from the middle.
   typedef std::list<ExtensionPrefs*> ExtensionStack;
   ExtensionStack extension_stack_;
 
diff --git a/chrome/browser/extensions/extension_pref_store_unittest.cc b/chrome/browser/extensions/extension_pref_store_unittest.cc
index 166abaa..aa16441 100644
--- a/chrome/browser/extensions/extension_pref_store_unittest.cc
+++ b/chrome/browser/extensions/extension_pref_store_unittest.cc
@@ -153,7 +153,7 @@ TEST(ExtensionPrefStoreTest, UninstallOnlyExtension) {
   eps.InstallExtensionPref("id1", kPref1, Value::CreateStringValue("val1"));
   eps.InstallExtensionPref("id1", kPref2, Value::CreateStringValue("val2"));
 
-  // No need to check the state here; the InstallOneExtension already has.
+  // No need to check the state here; the Install* tests cover that.
   eps.UninstallExtension("id1");
 
   TestExtensionPrefStore::ExtensionIDs ids;
@@ -256,26 +256,26 @@ TEST(ExtensionPrefStoreTest, UninstallExtensionFromMiddle) {
 }
 
 TEST(ExtensionPrefStoreTest, NotifyWhenNeeded) {
-  TestExtensionPrefStore eps;
+  TestExtensionPrefStore* eps = new TestExtensionPrefStore;
 
   // The PrefValueStore takes ownership of the PrefStores; in this case, that's
-  // only an ExtensionPrefStore.
-  PrefValueStore* value_store = new PrefValueStore(NULL, &eps, NULL, NULL,
-      NULL);
-  MockPrefService* pref_service = new MockPrefService(value_store);
-  eps.SetPrefService(pref_service);
+  // only an ExtensionPrefStore. Likewise, the PrefService takes ownership of
+  // the PrefValueStore.
+  PrefValueStore* value_store = new PrefValueStore(NULL, eps, NULL, NULL, NULL);
+  scoped_ptr<MockPrefService> pref_service(new MockPrefService(value_store));
+  eps->SetPrefService(pref_service.get());
   pref_service->RegisterStringPref(kPref1, std::string());
 
-  eps.InstallExtensionPref("abc", kPref1,
+  eps->InstallExtensionPref("abc", kPref1,
       Value::CreateStringValue("https://www.chromium.org"));
   EXPECT_TRUE(pref_service->fired_observers_);
-  eps.InstallExtensionPref("abc", kPref1,
+  eps->InstallExtensionPref("abc", kPref1,
       Value::CreateStringValue("https://www.chromium.org"));
   EXPECT_FALSE(pref_service->fired_observers_);
-  eps.InstallExtensionPref("abc", kPref1,
+  eps->InstallExtensionPref("abc", kPref1,
       Value::CreateStringValue("chrome://newtab"));
   EXPECT_TRUE(pref_service->fired_observers_);
 
-  eps.UninstallExtension("abc");
+  eps->UninstallExtension("abc");
   EXPECT_TRUE(pref_service->fired_observers_);
 }
diff --git a/tools/heapcheck/suppressions.txt b/tools/heapcheck/suppressions.txt
index 8578f0c..ac07700 100644
--- a/tools/heapcheck/suppressions.txt
+++ b/tools/heapcheck/suppressions.txt
@@ -840,38 +840,6 @@
    fun:start_thread
 }
 {
-   48980_a
-   Heapcheck:Leak
-   ...
-   fun:ExtensionPrefStoreTest_NotifyWhenNeeded_Test::TestBody
-}
-{
-   48980_b
-   Heapcheck:Leak
-   ...
-   fun:PrefService::RegisterStringPref
-   fun:ExtensionPrefStoreTest_*_Test::TestBody
-}
-{
-   48980_c
-   Heapcheck:Leak
-   ...
-   fun:ExtensionPrefStore::InstallExtensionPref
-}
-{
-   48980_d
-   Heapcheck:Leak
-   ...
-   fun:basic_string
-   fun:ExtensionPrefStoreTest_*_Test::TestBody
-}
-{
-   48980_e
-   Heapcheck:Leak
-   fun:Value::CreateStringValue
-   fun:ExtensionPrefStoreTest_*_Test::TestBody
-}
-{
    bug_49300_a
    Heapcheck:Leak
    fun:disk_cache::StorageBlock::AllocateData
diff --git a/tools/valgrind/memcheck/suppressions.txt b/tools/valgrind/memcheck/suppressions.txt
index 34aa1a5..9649db2 100644
--- a/tools/valgrind/memcheck/suppressions.txt
+++ b/tools/valgrind/memcheck/suppressions.txt
@@ -3469,26 +3469,6 @@
    fun:_ZN51AudioRendererHostTest_CreateLowLatencyAndClose_Test8TestBodyEv
 }
 {
-   bug_48980_a
-   Memcheck:Leak
-   ...
-   fun:_ZN18ExtensionPrefStore20InstallExtensionPrefESsPKwP5Value
-   fun:_ZN*ExtensionPrefStoreTest_*_Test8TestBodyEv
-}
-{
-   bug_48980_b
-   Memcheck:Leak
-   fun:_Znw*
-   fun:_ZN5Value17CreateStringValueERKSs
-   fun:_ZN*ExtensionPrefStoreTest_*_Test8TestBodyEv
-}
-{
-   bug_48980_c
-   Memcheck:Leak
-   fun:_Znw*
-   fun:_ZN44ExtensionPrefStoreTest_NotifyWhenNeeded_Test8TestBodyEv
-}
-{
    bug_48998
    Memcheck:Leak
    fun:_Znw*