Fix EntryKernel leak if sync database load fails

chrome/browser/sync/syncable/directory_backing_store.cc

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/syncable/directory_backing_store.h"
 
 #include "build/build_config.h"
 
 #if defined(OS_MACOSX)
 #include <CoreFoundation/CoreFoundation.h>
 #endif
 
 #include <limits>
 
 #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"
 #include "chrome/browser/sync/syncable/syncable-inl.h"
 #include "chrome/browser/sync/syncable/syncable_columns.h"
 #include "chrome/browser/sync/util/crypto_helpers.h"
 #include "chrome/common/sqlite_utils.h"
 #include "third_party/sqlite/preprocessed/sqlite3.h"
 
 // Sometimes threads contend on the DB lock itself, especially when one thread
@@ skipping 179 matching lines @@
     const BOOL attrs_set =
       SetFileAttributes(backing_filepath_.value().c_str(),
                         attrs | FILE_ATTRIBUTE_NOT_CONTENT_INDEXED);
 #endif
 
     return true;
   }
   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() {
   DCHECK(load_dbhandle_ == NULL);
   bool ret = OpenAndConfigureHandleHelper(&load_dbhandle_);
   if (ret)
     return ret;
   // Something's gone wrong. Nuke the database and try again.
   LOG(ERROR) << "Sync database " << backing_filepath_.value()
              << " corrupt. Deleting and recreating.";
@@ skipping 696 matching lines @@
       "name TEXT, "
       "store_birthday TEXT, "
       "db_create_version TEXT, "
       "db_create_time INT, "
       "next_id INT default -2, "
       "cache_guid TEXT)");
   return ExecQuery(load_dbhandle_, query.c_str());
 }
 
 }  // namespace syncable