Fix leaks found by sync unit tests.

1. Fix EntryKernel leak if sync database load fails.
2. DirectoryBackingStore: always close the load handle.
3. DirectoryBackingStore unittests: free when .clear()ing test index
4. Fix real bad leak in Directory::PurgeEntriesWithTypeIn.

BUG=50334,50335,50336,50347,50348,50349
TEST=sync_unit_tests

Review URL: http://codereview.chromium.org/3026029

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

4 files changed, 47 insertions, 27 deletions

diff --git a/chrome/browser/sync/syncable/directory_backing_store.cc b/chrome/browser/sync/syncable/directory_backing_store.cc
index b4de3f6..c5bfdb0 100644
--- a/chrome/browser/sync/syncable/directory_backing_store.cc
+++ b/chrome/browser/sync/syncable/directory_backing_store.cc
@@ -15,6 +15,7 @@
 #include "base/file_util.h"
 #include "base/hash_tables.h"
 #include "base/logging.h"
+#include "base/stl_util-inl.h"
 #include "chrome/browser/sync/protocol/bookmark_specifics.pb.h"
 #include "chrome/browser/sync/protocol/service_constants.h"
 #include "chrome/browser/sync/protocol/sync.pb.h"
@@ -214,23 +215,42 @@ bool DirectoryBackingStore::OpenAndConfigureHandleHelper(
   return false;
 }
 
+DirOpenResult DirectoryBackingStore::DoLoad(MetahandlesIndex* entry_bucket,
+    Directory::KernelLoadInfo* kernel_load_info) {
+  {
+    DirOpenResult result = InitializeTables();
+    if (result != OPENED)
+      return result;
+  }
+
+  if (!DropDeletedEntries())
+    return FAILED_DATABASE_CORRUPT;
+  if (!LoadEntries(entry_bucket))
+    return FAILED_DATABASE_CORRUPT;
+  if (!LoadInfo(kernel_load_info))
+    return FAILED_DATABASE_CORRUPT;
+
+  return OPENED;
+}
+
 DirOpenResult DirectoryBackingStore::Load(MetahandlesIndex* entry_bucket,
     Directory::KernelLoadInfo* kernel_load_info) {
+
+  // Open database handle.
   if (!BeginLoad())
     return FAILED_OPEN_DATABASE;
 
-  DirOpenResult result = InitializeTables();
-  if (OPENED != result)
-    return result;
+  // Load data from the database.
+  DirOpenResult result = DoLoad(entry_bucket, kernel_load_info);
 
-  if (!DropDeletedEntries() ||
-      !LoadEntries(entry_bucket) ||
-      !LoadInfo(kernel_load_info)) {
-    return FAILED_DATABASE_CORRUPT;
-  }
+  // Clean up partial results after failure.
+  if (result != OPENED)
+    STLDeleteElements(entry_bucket);
 
+  // Close database handle.
   EndLoad();
-  return OPENED;
+
+  return result;
 }
 
 bool DirectoryBackingStore::BeginLoad() {
diff --git a/chrome/browser/sync/syncable/directory_backing_store.h b/chrome/browser/sync/syncable/directory_backing_store.h
index 3739b39..e1b0cca 100644
--- a/chrome/browser/sync/syncable/directory_backing_store.h
+++ b/chrome/browser/sync/syncable/directory_backing_store.h
@@ -123,6 +123,8 @@ class DirectoryBackingStore {
   // Initialize and destroy load_dbhandle_.  Broken out for testing.
   bool BeginLoad();
   void EndLoad();
+  DirOpenResult DoLoad(MetahandlesIndex* entry_bucket,
+      Directory::KernelLoadInfo* kernel_load_info);
 
   // Close save_dbhandle_.  Broken out for testing.
   void EndSave();
diff --git a/chrome/browser/sync/syncable/directory_backing_store_unittest.cc b/chrome/browser/sync/syncable/directory_backing_store_unittest.cc
index b1f5dc98..365612c 100644
--- a/chrome/browser/sync/syncable/directory_backing_store_unittest.cc
+++ b/chrome/browser/sync/syncable/directory_backing_store_unittest.cc
@@ -879,6 +879,7 @@ TEST_P(MigrationTest, ToCurrentVersion) {
   }
 
   MetahandlesIndex index;
+  STLElementDeleter<MetahandlesIndex> index_deleter(&index);
   dbs->LoadEntries(&index);
   dbs->EndLoad();
 
@@ -1004,8 +1005,6 @@ TEST_P(MigrationTest, ToCurrentVersion) {
   ASSERT_EQ(14, (*it)->ref(META_HANDLE));
 
   ASSERT_TRUE(++it == index.end());
-
-  STLDeleteElements(&index);
 }
 
 INSTANTIATE_TEST_CASE_P(DirectoryBackingStore, MigrationTest,
@@ -1048,8 +1047,8 @@ TEST_F(DirectoryBackingStoreTest, DeleteEntries) {
   scoped_ptr<DirectoryBackingStore> dbs(
       new DirectoryBackingStore(GetUsername(), GetDatabasePath()));
   dbs->BeginLoad();
-
   MetahandlesIndex index;
+  STLElementDeleter<MetahandlesIndex> index_deleter(&index);
   dbs->LoadEntries(&index);
   size_t initial_size = index.size();
   ASSERT_LT(0U, initial_size) << "Test requires entries to delete.";
@@ -1058,7 +1057,7 @@ TEST_F(DirectoryBackingStoreTest, DeleteEntries) {
   to_delete.insert(first_to_die);
   EXPECT_TRUE(dbs->DeleteEntries(to_delete));
 
-  index.clear();
+  STLDeleteElements(&index);
   dbs->LoadEntries(&index);
 
   EXPECT_EQ(initial_size - 1, index.size());
@@ -1080,7 +1079,7 @@ TEST_F(DirectoryBackingStoreTest, DeleteEntries) {
 
   EXPECT_TRUE(dbs->DeleteEntries(to_delete));
 
-  index.clear();
+  STLDeleteElements(&index);
   dbs->LoadEntries(&index);
   EXPECT_EQ(0U, index.size());
 
diff --git a/chrome/browser/sync/syncable/syncable.cc b/chrome/browser/sync/syncable/syncable.cc
index a2a9a98..cf19379 100644
--- a/chrome/browser/sync/syncable/syncable.cc
+++ b/chrome/browser/sync/syncable/syncable.cc
@@ -32,6 +32,7 @@
 #include "base/perftimer.h"
 #include "base/scoped_ptr.h"
 #include "base/string_util.h"
+#include "base/stl_util-inl.h"
 #include "base/time.h"
 #include "chrome/browser/sync/engine/syncer.h"
 #include "chrome/browser/sync/engine/syncer_util.h"
@@ -183,10 +184,6 @@ Directory::Kernel::Kernel(const FilePath& db_path,
       next_metahandle(info.max_metahandle + 1) {
 }
 
-inline void DeleteEntry(EntryKernel* kernel) {
-  delete kernel;
-}
-
 void Directory::Kernel::AddRef() {
   base::subtle::NoBarrier_AtomicIncrement(&refcount, 1);
 }
@@ -207,7 +204,7 @@ Directory::Kernel::~Kernel() {
   delete parent_id_child_index;
   delete client_tag_index;
   delete ids_index;
-  for_each(metahandles_index->begin(), metahandles_index->end(), DeleteEntry);
+  STLDeleteElements(metahandles_index);
   delete metahandles_index;
 }
 
@@ -339,7 +336,7 @@ EntryKernel* Directory::GetEntryByHandle(const int64 metahandle,
   // Look up in memory
   kernel_->needle.put(META_HANDLE, metahandle);
   MetahandlesIndex::iterator found =
-    kernel_->metahandles_index->find(&kernel_->needle);
+      kernel_->metahandles_index->find(&kernel_->needle);
   if (found != kernel_->metahandles_index->end()) {
     // Found it in memory.  Easy.
     return *found;
@@ -630,17 +627,19 @@ void Directory::PurgeEntriesWithTypeIn(const std::set<ModelType>& types) {
           kernel_->metahandles_to_purge->insert(handle);
 
           size_t num_erased = 0;
-          num_erased = kernel_->ids_index->erase(*it);
+          EntryKernel* entry = *it;
+          num_erased = kernel_->ids_index->erase(entry);
           DCHECK_EQ(1u, num_erased);
-          num_erased = kernel_->client_tag_index->erase(*it);
-          DCHECK_EQ((*it)->ref(UNIQUE_CLIENT_TAG).empty(), !num_erased);
+          num_erased = kernel_->client_tag_index->erase(entry);
+          DCHECK_EQ(entry->ref(UNIQUE_CLIENT_TAG).empty(), !num_erased);
           num_erased = kernel_->unsynced_metahandles->erase(handle);
-          DCHECK_EQ((*it)->ref(IS_UNSYNCED), num_erased > 0);
+          DCHECK_EQ(entry->ref(IS_UNSYNCED), num_erased > 0);
           num_erased = kernel_->unapplied_update_metahandles->erase(handle);
-          DCHECK_EQ((*it)->ref(IS_UNAPPLIED_UPDATE), num_erased > 0);
-          num_erased = kernel_->parent_id_child_index->erase(*it);
-          DCHECK_EQ((*it)->ref(IS_DEL), !num_erased);
+          DCHECK_EQ(entry->ref(IS_UNAPPLIED_UPDATE), num_erased > 0);
+          num_erased = kernel_->parent_id_child_index->erase(entry);
+          DCHECK_EQ(entry->ref(IS_DEL), !num_erased);
           kernel_->metahandles_index->erase(it++);
+          delete entry;
         } else {
           ++it;
         }