Sync: Improve handling of database load failures

chrome/browser/sync/syncable/directory_backing_store.cc

 // Copyright (c) 2011 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"
 
 #include <limits>
 
@@ skipping 218 matching lines @@
 #endif
 
     return true;
   }
   return false;
 }
 
 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;
   }
   string integrity_result = statement.column_text(0);
   if (integrity_result != "ok") {
     *error = integrity_result;
     return false;
   }
   return true;
@@ skipping 32 matching lines @@
     STLDeleteElements(entry_bucket);
 
   // Close database handle.
   EndLoad();
 
   return result;
 }
 
 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() {
   sqlite3_close(load_dbhandle_);
   load_dbhandle_ = NULL;  // No longer used.
 }
 
 void DirectoryBackingStore::EndSave() {
   sqlite3_close(save_dbhandle_);
   save_dbhandle_ = NULL;
@@ skipping 937 matching lines @@
       "id TEXT primary key, "
       "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