Fix EntryKernel leak if sync database load fails

chrome/browser/sync/syncable/directory_backing_store.cc

Index: chrome/browser/sync/syncable/directory_backing_store.cc
diff --git a/chrome/browser/sync/syncable/directory_backing_store.cc b/chrome/browser/sync/syncable/directory_backing_store.cc
index b4de3f6a06a40e973fc65e52558a55878ad88409..c5bfdb04d1d98b99bfbc10d814d4fe06e934627c 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))

[tim (not reviewing), 2010/07/28 19:00:54]

I don't think this is what you were suggesting before as another option (sorry
if I misunderstood), but looking at this, shouldn't we either do the
error-check-and-cleanup in LoadEntries, or do the following in here (DoLoad) to
nip it in the bud?

MetahandlesIndex rv;
STLElementDeleter d(&rv);
{
...
if (!LoadEntries(&rv))
  return FAILED
}

entry_bucket->swap(rv);
return OPENED;

[ncarter (slow), 2010/07/28 19:17:19]

It's a clever technique, but we need to handle the failure higher up: there
could be a failure later (e.g. in LoadInfo).  My preference is to leave things
as they are.

+    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() {