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..10e694fccc996d9309e759fd6f3a1894737f9b6e 100644
--- a/sync/syncable/on_disk_directory_backing_store.cc
+++ b/sync/syncable/on_disk_directory_backing_store.cc
@@ -5,19 +5,33 @@
 #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(
+DirOpenResult OnDiskDirectoryBackingStore::TryLoad(
     MetahandlesIndex* entry_bucket,
     Directory::KernelLoadInfo* kernel_load_info) {
   DCHECK(CalledOnValidThread());
@@ -35,8 +49,50 @@ 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;
+  }
+
+  // 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.

[tim (not reviewing), 2012/08/01 21:31:59]

Let's keep an eye on this.  I've seen similar dchecks, particularly around
sqlite, in other parts of chrome code really annoy people, especially when
debugging (or inspecting the db, ...).  sqlite can and will fail.

[rlarocque, 2012/08/01 22:37:51]

I've updated the comment with instructions on how to bypass the DCHECK.  I'm
hoping that will reduce the annoyance somewhat.

+  if (!allow_failure_for_test_)
+    NOTREACHED() << "Crashing to preserve corrupt sync database";
+
+  // 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);

[tim (not reviewing), 2012/08/01 21:31:59]

A bit nervous to swap this out since we set it in the constructor, but okay.

+  file_util::Delete(backing_filepath_, false);
+
+  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::AllowFailureForTest() {

[tim (not reviewing), 2012/08/01 21:31:59]

I think it would be clearer / preferable to have a ReportTryLoadFailure()
virtual that could call NOTREACHED() << "try load failed!" in this case, and
perhaps just log a warning in tests.

[rlarocque, 2012/08/01 21:53:50]

Yet another implementation of DirectoryBackingStore.  That would bring the total
up to five.

You're right, though.  That is the right thing to do.

+  allow_failure_for_test_ = true;
 }
 
 }  // namespace syncable