Sync: Improve handling of database load failures

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 ed4242fd943695da86cd5285984c2f83cacfd4b2..c037ff09a9b01bdc3defe2d9582ad921ad9a4833 100644
--- a/chrome/browser/sync/syncable/directory_backing_store.cc
+++ b/chrome/browser/sync/syncable/directory_backing_store.cc
@@ -236,7 +236,11 @@ bool DirectoryBackingStore::OpenAndConfigureHandleHelper(
 bool DirectoryBackingStore::CheckIntegrity(sqlite3* handle, string* error)
     const {
   sqlite_utils::SQLStatement statement;
-  statement.prepare(handle, "PRAGMA integrity_check(1)");
+  if (SQLITE_OK !=
+      statement.prepare(handle, "PRAGMA integrity_check(1)")) {

[lipalani1, 2011/11/14 21:34:24]

We can add a UMA metric here.

[rlarocque, 2011/11/14 22:56:26]

We could, but we'd probably end up taking it out in a few weeks.  

This change is a band-aid solution.  This entire file's use of the sqlite API
assumes that prepare() can't fail, which is wrong.  Furthermore, every other
component of Chromium has migrated to a newer API that does a better job of
handling these errors.  See crbug.com/77634.

I think we should come back to this when we've converted to the new API.  We
should have more options for tracking those errors at that point.  

Until then, we can rely on the histogram I'm adding to ProfileSyncService to
tell us if we're disabling sync too much.  

+    *error = sqlite3_errmsg(handle);
+    return false;
+  }
   if (SQLITE_ROW != statement.step()) {
     *error =  sqlite3_errmsg(handle);
     return false;
@@ -289,37 +293,7 @@ DirOpenResult DirectoryBackingStore::Load(MetahandlesIndex* entry_bucket,
 
 bool DirectoryBackingStore::BeginLoad() {
   DCHECK(load_dbhandle_ == NULL);
-  bool ret = OpenAndConfigureHandleHelper(&load_dbhandle_);
-  if (ret)
-    return true;
-  // Something's gone wrong. Nuke the database and try again.
-  using ::operator<<;  // For string16.
-  LOG(ERROR) << "Sync database " << backing_filepath_.value()
-             << " corrupt. Deleting and recreating.";
-  file_util::Delete(backing_filepath_, false);
-  bool failed_again = !OpenAndConfigureHandleHelper(&load_dbhandle_);
-
-  // Using failed_again here lets us distinguish from cases where corruption
-  // occurred even when re-opening a fresh directory (they'll go in a separate
-  // double weight histogram bucket).  Failing twice in a row means we disable
-  // sync, so it's useful to see this number separately.
-  int bucket = failed_again ? 2 : 1;
-#if defined(OS_WIN)
-  UMA_HISTOGRAM_COUNTS_100("Sync.DirectoryOpenFailedWin", bucket);

[Nicolas Zea, 2011/11/14 19:29:21]

Since I suppose these histograms don't make sense when we're not attempting to
re-open the directory, we should remove them from the internal histogram file as
well.

[rlarocque, 2011/11/14 22:56:26]

Done in a separate CL that also adds replacement histograms.

-#elif defined(OS_MACOSX)
-  UMA_HISTOGRAM_COUNTS_100("Sync.DirectoryOpenFailedMac", bucket);
-#else
-  UMA_HISTOGRAM_COUNTS_100("Sync.DirectoryOpenFailedNotWinMac", bucket);
-
-#if defined(OS_CHROMEOS)
-  UMA_HISTOGRAM_COUNTS_100("Sync.DirectoryOpenFailedCros", bucket);
-#elif defined(OS_LINUX)
-  UMA_HISTOGRAM_COUNTS_100("Sync.DirectoryOpenFailedLinux", bucket);
-#else
-  UMA_HISTOGRAM_COUNTS_100("Sync.DirectoryOpenFailedOther", bucket);
-#endif  // OS_LINUX && !OS_CHROMEOS
-#endif  // OS_WIN
-  return !failed_again;
+  return OpenAndConfigureHandleHelper(&load_dbhandle_);
 }
 
 void DirectoryBackingStore::EndLoad() {