[Sync] Erase sync DB when corrupted

Directory now registers to be notified of catastrophic sync DB errors.
Upon notification, Directory triggers an unrecoverable sync error which
causes ProfileSyncService to delete the sync DB.

Add integration test that verifies the sync DB is deleted when DB
corruption is detected.

Remove unused member variable allow_failure_for_test_ from
DirectoryBackingStore.

Add MockUnrecoverableErrorHandler to assist in writing tests for
Directory.

BUG=470993

Committed: https://crrev.com/90a7f8d3e3b3a2aac43e71a93eac2731f084945c
Cr-Commit-Position: refs/heads/master@{#327120}

Review URL: https://codereview.chromium.org/1082893003

Cr-Commit-Position: refs/heads/master@{#327766}

6 files changed, 54 insertions, 31 deletions

diff --git a/sync/syncable/directory.cc b/sync/syncable/directory.cc
index 767b807..0f680d22 100644
--- a/sync/syncable/directory.cc
+++ b/sync/syncable/directory.cc
@@ -111,12 +111,12 @@ Directory::Directory(
     : kernel_(NULL),
       store_(store),
       unrecoverable_error_handler_(unrecoverable_error_handler),
-      report_unrecoverable_error_function_(
-          report_unrecoverable_error_function),
+      report_unrecoverable_error_function_(report_unrecoverable_error_function),
       unrecoverable_error_set_(false),
       nigori_handler_(nigori_handler),
       cryptographer_(cryptographer),
-      invariant_check_level_(VERIFY_CHANGES) {
+      invariant_check_level_(VERIFY_CHANGES),
+      weak_ptr_factory_(this) {
 }
 
 Directory::~Directory() {
@@ -208,6 +208,12 @@ DirOpenResult Directory::OpenImpl(
   if (!SaveChanges())
     return FAILED_INITIAL_WRITE;
 
+  // Now that we've successfully opened the store, install an error handler to
+  // deal with catastrophic errors that may occur later on. Use a weak pointer
+  // because we cannot guarantee that this Directory will outlive the Closure.
+  store_->SetCatastrophicErrorHandler(base::Bind(
+      &Directory::OnCatastrophicError, weak_ptr_factory_.GetWeakPtr()));
+
   return OPENED;
 }
 
@@ -1554,6 +1560,12 @@ void Directory::GetAttachmentIdsToUpload(BaseTransaction* trans,
                       std::back_inserter(*ids));
 }
 
+void Directory::OnCatastrophicError() {
+  ReadTransaction trans(FROM_HERE, this);
+  OnUnrecoverableError(&trans, FROM_HERE,
+                       "Catastrophic error detected, Sync DB is unrecoverable");
+}
+
 Directory::Kernel* Directory::kernel() {
   return kernel_;
 }
diff --git a/sync/syncable/directory.h b/sync/syncable/directory.h
index 7f6ae0e..4f45cdf 100644
--- a/sync/syncable/directory.h
+++ b/sync/syncable/directory.h
@@ -533,6 +533,7 @@ class SYNC_EXPORT Directory {
                            TakeSnapshotGetsOnlyDirtyHandlesTest);
   FRIEND_TEST_ALL_PREFIXES(SyncableDirectoryTest,
                            TakeSnapshotGetsMetahandlesToPurge);
+  FRIEND_TEST_ALL_PREFIXES(SyncableDirectoryTest, CatastrophicError);
 
   // You'll notice that some of the methods below are private overloads of the
   // public ones declared above. The general pattern is that the public overload
@@ -622,6 +623,10 @@ class SYNC_EXPORT Directory {
                             ModelType type,
                             std::vector<int64>* result);
 
+  // Invoked by DirectoryBackingStore when a catastrophic database error is
+  // detected.
+  void OnCatastrophicError();
+
   // Returns true if the initial sync for |type| has completed.
   bool InitialSyncEndedForType(BaseTransaction* trans, ModelType type);
 
@@ -653,6 +658,8 @@ class SYNC_EXPORT Directory {
   // are deleted in native models as well.
   scoped_ptr<DeleteJournal> delete_journal_;
 
+  base::WeakPtrFactory<Directory> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(Directory);
 };
 
diff --git a/sync/syncable/directory_backing_store_unittest.cc b/sync/syncable/directory_backing_store_unittest.cc
index 435533c..0cc723f 100644
--- a/sync/syncable/directory_backing_store_unittest.cc
+++ b/sync/syncable/directory_backing_store_unittest.cc
@@ -7,8 +7,6 @@
 #include <string>
 
 #include "base/files/file_path.h"
-#include "base/files/file_util.h"
-#include "base/files/scoped_file.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
@@ -27,6 +25,7 @@
 #include "sync/syncable/directory_backing_store.h"
 #include "sync/syncable/on_disk_directory_backing_store.h"
 #include "sync/syncable/syncable-inl.h"
+#include "sync/test/directory_backing_store_corruption_testing.h"
 #include "sync/test/test_directory_backing_store.h"
 #include "sync/util/time.h"
 #include "testing/gtest/include/gtest/gtest-param-test.h"
@@ -4095,7 +4094,6 @@ TEST_F(DirectoryBackingStoreTest,
   bool was_called = false;
   const base::Closure handler =
       base::Bind(&CatastrophicErrorHandler, &was_called);
-
   // Create a DB with many entries.
   scoped_ptr<OnDiskDirectoryBackingStoreForTest> dbs(
       new OnDiskDirectoryBackingStoreForTest(GetUsername(), GetDatabasePath()));
@@ -4104,32 +4102,19 @@ TEST_F(DirectoryBackingStoreTest,
   ASSERT_TRUE(LoadAndIgnoreReturnedData(dbs.get()));
   ASSERT_FALSE(dbs->DidFailFirstOpenAttempt());
   Directory::SaveChangesSnapshot snapshot;
-  const int num_entries = 4000;
-  for (int i = 0; i < num_entries; ++i) {
+  for (int i = 0; i < corruption_testing::kNumEntriesRequiredForCorruption;
+       ++i) {
     snapshot.dirty_metas.insert(CreateEntry(i).release());
   }
   ASSERT_TRUE(dbs->SaveChanges(snapshot));
-
-  // Corrupt the DB by write a bunch of zeros at the beginning.
-  {
-    // Because the file is already open for writing (see dbs above), it's
-    // important that we open it in a sharing compatible way for platforms that
-    // have the concept of shared/exclusive file access (e.g. Windows).
-    base::ScopedFILE db_file(base::OpenFile(GetDatabasePath(), "wb"));
-    ASSERT_TRUE(db_file.get());
-    const std::string zeros(4096, '\0');
-    ASSERT_EQ(1U, fwrite(zeros.data(), zeros.size(), 1, db_file.get()));
-  }
-
+  // Corrupt it.
+  ASSERT_TRUE(corruption_testing::CorruptDatabase(GetDatabasePath()));
   // Attempt to save all those entries again. See that it fails (because of the
   // corruption).
   //
-  // If this test fails because SaveChanges returned true, it may mean that you
-  // need to increase the number of entries written to the DB. Try increasing
-  // the value of num_entries above. The value needs to be large enough to force
-  // the underlying DB to be read from disk before writing. The value *may*
-  // depend on the underlying DB page size as well as the DB's cache_size
-  // PRAGMA.
+  // If this test fails because SaveChanges returned true, it may mean you need
+  // to increase the number of entries written to the DB. See also
+  // |kNumEntriesRequiredForCorruption|.
   ASSERT_FALSE(dbs->SaveChanges(snapshot));
   // At this point the handler has been posted but not executed.
   ASSERT_FALSE(was_called);
diff --git a/sync/syncable/directory_unittest.cc b/sync/syncable/directory_unittest.cc
index d802d4f..690af4f 100644
--- a/sync/syncable/directory_unittest.cc
+++ b/sync/syncable/directory_unittest.cc
@@ -12,6 +12,7 @@
 #include "sync/syncable/syncable_write_transaction.h"
 #include "sync/test/engine/test_syncable_utils.h"
 #include "sync/test/test_directory_backing_store.h"
+#include "sync/util/mock_unrecoverable_error_handler.h"
 
 using base::ExpectDictBooleanValue;
 using base::ExpectDictStringValue;
@@ -2030,6 +2031,25 @@ TEST_F(SyncableDirectoryTest, SaveChangesSnapshot_HasUnsavedMetahandleChanges) {
   snapshot.delete_journals_to_purge.clear();
 }
 
+// Verify that Directory triggers an unrecoverable error when a catastrophic
+// DirectoryBackingStore error is detected.
+TEST_F(SyncableDirectoryTest, CatastrophicError) {
+  MockUnrecoverableErrorHandler unrecoverable_error_handler;
+  Directory dir(new InMemoryDirectoryBackingStore("catastrophic_error"),
+                &unrecoverable_error_handler, nullptr, nullptr, nullptr);
+  ASSERT_EQ(OPENED, dir.Open(kDirectoryName, directory_change_delegate(),
+                             NullTransactionObserver()));
+  ASSERT_EQ(0, unrecoverable_error_handler.invocation_count());
+
+  // Fire off two catastrophic errors. Call it twice to ensure Directory is
+  // tolerant of multiple invocations since that may happen in the real world.
+  dir.OnCatastrophicError();
+  dir.OnCatastrophicError();
+
+  // See that the unrecoverable error handler has been invoked twice.
+  ASSERT_EQ(2, unrecoverable_error_handler.invocation_count());
+}
+
 }  // namespace syncable
 
 }  // namespace syncer
diff --git a/sync/syncable/on_disk_directory_backing_store.cc b/sync/syncable/on_disk_directory_backing_store.cc
index 918dc07..3a37847 100644
--- a/sync/syncable/on_disk_directory_backing_store.cc
+++ b/sync/syncable/on_disk_directory_backing_store.cc
@@ -26,9 +26,8 @@ enum HistogramResultEnum {
 OnDiskDirectoryBackingStore::OnDiskDirectoryBackingStore(
     const std::string& dir_name,
     const base::FilePath& backing_file_path)
-    : DirectoryBackingStore(dir_name),
-      allow_failure_for_test_(false),
-      backing_file_path_(backing_file_path) {
+    : DirectoryBackingStore(dir_name), backing_file_path_(backing_file_path) {
+  DCHECK(backing_file_path_.IsAbsolute());
 }
 
 OnDiskDirectoryBackingStore::~OnDiskDirectoryBackingStore() {
diff --git a/sync/syncable/on_disk_directory_backing_store.h b/sync/syncable/on_disk_directory_backing_store.h
index 8c604d0..05f4156 100644
--- a/sync/syncable/on_disk_directory_backing_store.h
+++ b/sync/syncable/on_disk_directory_backing_store.h
@@ -40,8 +40,8 @@ class SYNC_EXPORT_PRIVATE OnDiskDirectoryBackingStore
                         MetahandleSet* metahandles_to_purge,
                         Directory::KernelLoadInfo* kernel_load_info);
 
-  bool allow_failure_for_test_;
-  base::FilePath backing_file_path_;
+  // The path to the sync DB.
+  const base::FilePath backing_file_path_;
 
   DISALLOW_COPY_AND_ASSIGN(OnDiskDirectoryBackingStore);
 };