sync: Attempt to recover from directory corruption

sync/syncable/on_disk_directory_backing_store.cc

Index: sync/syncable/on_disk_directory_backing_store.cc
diff --git a/sync/syncable/on_disk_directory_backing_store.cc b/sync/syncable/on_disk_directory_backing_store.cc
index 1f43da6ada6744be7eacba8293c0801d12b7c730..f844f38c4293d2552143b94d1aeba619a582f96f 100644
--- a/sync/syncable/on_disk_directory_backing_store.cc
+++ b/sync/syncable/on_disk_directory_backing_store.cc
@@ -5,19 +5,35 @@
 #include "sync/syncable/on_disk_directory_backing_store.h"
 
 #include "base/logging.h"
+#include "base/metrics/histogram.h"
+#include "sync/syncable/syncable-inl.h"
 
 namespace syncer {
 namespace syncable {
 
+namespace {
+
+enum HistogramResultEnum {
+  FIRST_TRY_SUCCESS,
+  SECOND_TRY_SUCCESS,
+  SECOND_TRY_FAILURE,
+  RESULT_COUNT
+};
+
+}  // namespace
+
 OnDiskDirectoryBackingStore::OnDiskDirectoryBackingStore(
     const std::string& dir_name, const FilePath& backing_filepath)
     : DirectoryBackingStore(dir_name),
+      allow_failure_for_test_(false),
       backing_filepath_(backing_filepath) {
   db_->set_exclusive_locking();
   db_->set_page_size(4096);
 }
 
-DirOpenResult OnDiskDirectoryBackingStore::Load(
+OnDiskDirectoryBackingStore::~OnDiskDirectoryBackingStore() { }
+
+DirOpenResult OnDiskDirectoryBackingStore::TryLoad(
     MetahandlesIndex* entry_bucket,
     Directory::KernelLoadInfo* kernel_load_info) {
   DCHECK(CalledOnValidThread());
@@ -35,8 +51,53 @@ DirOpenResult OnDiskDirectoryBackingStore::Load(
     return FAILED_DATABASE_CORRUPT;
   if (!LoadInfo(kernel_load_info))
     return FAILED_DATABASE_CORRUPT;
+  if (!VerifyReferenceIntegrity(*entry_bucket))
+    return FAILED_DATABASE_CORRUPT;
 
   return OPENED;
+
+}
+
+DirOpenResult OnDiskDirectoryBackingStore::Load(
+    MetahandlesIndex* entry_bucket,
+    Directory::KernelLoadInfo* kernel_load_info) {
+  DirOpenResult result = TryLoad(entry_bucket, kernel_load_info);
+  if (result == OPENED) {
+    HISTOGRAM_ENUMERATION(
+        "Sync.DirectoryOpenResult", FIRST_TRY_SUCCESS, RESULT_COUNT);
+    return OPENED;
+  }
+
+  ReportFirstTryOpenFailure();
+
+  // The fallback: delete the current database and return a fresh one.  We can
+  // fetch the user's data from the could.
+  entry_bucket->clear();
+  db_.reset(new sql::Connection);
+  file_util::Delete(backing_filepath_, false);

[tim (not reviewing), 2012/08/02 00:31:18]

How did you test this?  

We have integration tests now that simulate browser start, I believe (I forget
exactly how they work).  It would be SO COOL if we could corrupt the db in such
a way that we'd trigger this code path...  is that just a pipe dream?  

We have some that simulate a browser restart, though... apparently disabled :(
http://code.google.com/p/chromium/source/search?q=RestartSyncService

[rlarocque, 2012/08/02 00:47:20]

We have to instantiate a special version of the directory to avoid the DCHECK. 
I don't think there's any hope of plumbing it all the way down through the
integration tests.

The corruption itself was pretty simple.  One of the checks we perform on load
is to make sure all the IDs referred to actually exist.  All I had to do was run
some SQL to update a node's PARENT_ID to a non-existent ID.

On the plus side, the transparent re-create case is tested by a unit test I'm
adding in this commit.  The failure case (all the way back to the
ProfileSyncService) is tested in the unit test I landed in r149306
(http://codereview.chromium.org/10830100/).  We've got pretty good coverage of
this code without needing to write integration tests.  

+
+  result = TryLoad(entry_bucket, kernel_load_info);
+  if (result == OPENED) {
+    HISTOGRAM_ENUMERATION(
+        "Sync.DirectoryOpenResult", SECOND_TRY_SUCCESS, RESULT_COUNT);
+  } else {
+    HISTOGRAM_ENUMERATION(
+        "Sync.DirectoryOpenResult", SECOND_TRY_FAILURE, RESULT_COUNT);
+  }
+
+  return result;
+}
+
+void OnDiskDirectoryBackingStore::ReportFirstTryOpenFailure() {
+  // In debug builds, the last thing we want is to silently clear the database.
+  // It's full of evidence that might help us determine what went wrong.  It
+  // might be sqlite's fault, but it could also be a bug in sync.  We crash
+  // immediately so a developer can investigate.
+  //
+  // Developers: If you're not interested in debugging this right now, just move
+  // aside the 'Sync Data' directory in your profile.  This is similar to what
+  // the code would do if this DCHECK were disabled.
+  NOTREACHED() << "Crashing to preserve corrupt sync database";
 }
 
 }  // namespace syncable