[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

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

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

12 files changed, 185 insertions, 31 deletions

diff --git a/sync/BUILD.gn b/sync/BUILD.gn
index d6cec92..bfb3566 100644
--- a/sync/BUILD.gn
+++ b/sync/BUILD.gn
@@ -444,6 +444,8 @@ static_library("test_support_sync_core") {
     "sessions/test_util.cc",
     "sessions/test_util.h",
     "test/callback_counter.h",
+    "test/directory_backing_store_corruption_testing.cc",
+    "test/directory_backing_store_corruption_testing.h",
     "test/engine/fake_model_worker.cc",
     "test/engine/fake_model_worker.h",
     "test/engine/fake_sync_scheduler.cc",
@@ -488,6 +490,8 @@ static_library("test_support_sync_core") {
     "test/test_transaction_observer.h",
     "test/trackable_mock_invalidation.cc",
     "test/trackable_mock_invalidation.h",
+    "util/mock_unrecoverable_error_handler.cc",
+    "util/mock_unrecoverable_error_handler.h",
     "util/test_unrecoverable_error_handler.cc",
     "util/test_unrecoverable_error_handler.h",
   ]
diff --git a/sync/sync_tests.gypi b/sync/sync_tests.gypi
index 33594a1..69804e1 100644
--- a/sync/sync_tests.gypi
+++ b/sync/sync_tests.gypi
@@ -35,6 +35,8 @@
         'sessions/test_util.cc',
         'sessions/test_util.h',
         'test/callback_counter.h',
+        "test/directory_backing_store_corruption_testing.cc",
+        "test/directory_backing_store_corruption_testing.h",
         'test/engine/fake_model_worker.cc',
         'test/engine/fake_model_worker.h',
         'test/engine/fake_sync_scheduler.cc',
@@ -79,6 +81,8 @@
         'test/test_transaction_observer.h',
         'test/trackable_mock_invalidation.cc',
         'test/trackable_mock_invalidation.h',
+        'util/mock_unrecoverable_error_handler.cc',
+        'util/mock_unrecoverable_error_handler.h',
         'util/test_unrecoverable_error_handler.cc',
         'util/test_unrecoverable_error_handler.h',
       ],
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);
 };
diff --git a/sync/test/directory_backing_store_corruption_testing.cc b/sync/test/directory_backing_store_corruption_testing.cc
new file mode 100644
index 0000000..9767e56
--- /dev/null
+++ b/sync/test/directory_backing_store_corruption_testing.cc
@@ -0,0 +1,35 @@
+// Copyright 2015 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 "sync/test/directory_backing_store_corruption_testing.h"
+
+#include "base/files/file_util.h"
+#include "base/files/scoped_file.h"
+
+namespace syncer {
+namespace syncable {
+namespace corruption_testing {
+
+// This 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.
+const int kNumEntriesRequiredForCorruption = 4000;
+
+bool CorruptDatabase(const base::FilePath& backing_file_path) {
+  // Corrupt the DB by write a bunch of zeros at the beginning.
+  //
+  // Because the file may already open for writing, 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(backing_file_path, "wb"));
+  if (!db_file.get())
+    return false;
+  const std::string zeros(4096, '\0');
+  const int num_written = fwrite(zeros.data(), zeros.size(), 1, db_file.get());
+  return num_written == 1U;
+}
+
+}  // namespace corruption_util
+}  // namespace syncable
+}  // namespace syncer
diff --git a/sync/test/directory_backing_store_corruption_testing.h b/sync/test/directory_backing_store_corruption_testing.h
new file mode 100644
index 0000000..5897ec0
--- /dev/null
+++ b/sync/test/directory_backing_store_corruption_testing.h
@@ -0,0 +1,27 @@
+// Copyright 2015 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.
+
+#ifndef SYNC_TEST_DIRECTORY_BACKING_STORE_CORRUPTION_TESTING_H_
+#define SYNC_TEST_DIRECTORY_BACKING_STORE_CORRUPTION_TESTING_H_
+
+#include "base/files/file_path.h"
+
+namespace syncer {
+namespace syncable {
+namespace corruption_testing {
+
+// The number of DB entries required to reliably trigger detectable corruption
+// using |CorruptDatabase|.
+extern const int kNumEntriesRequiredForCorruption;
+
+// Corrupt the sync database at |backing_file_path|.
+//
+// Returns true if the database was successfully corrupted.
+bool CorruptDatabase(const base::FilePath& backing_file_path);
+
+}  // namespace corruption_util
+}  // namespace syncable
+}  // namespace syncer
+
+#endif  // SYNC_TEST_DIRECTORY_BACKING_STORE_CORRUPTION_TESTING_H_
diff --git a/sync/util/mock_unrecoverable_error_handler.cc b/sync/util/mock_unrecoverable_error_handler.cc
new file mode 100644
index 0000000..bfed1f1
--- /dev/null
+++ b/sync/util/mock_unrecoverable_error_handler.cc
@@ -0,0 +1,26 @@
+// Copyright 2015 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 "sync/util/mock_unrecoverable_error_handler.h"
+
+namespace syncer {
+
+MockUnrecoverableErrorHandler::MockUnrecoverableErrorHandler()
+    : invocation_count_(0) {
+}
+
+MockUnrecoverableErrorHandler::~MockUnrecoverableErrorHandler() {
+}
+
+void MockUnrecoverableErrorHandler::OnUnrecoverableError(
+    const tracked_objects::Location& from_here,
+    const std::string& message) {
+  ++invocation_count_;
+}
+
+int MockUnrecoverableErrorHandler::invocation_count() const {
+  return invocation_count_;
+}
+
+}  // namespace syncer
diff --git a/sync/util/mock_unrecoverable_error_handler.h b/sync/util/mock_unrecoverable_error_handler.h
new file mode 100644
index 0000000..aacde89
--- /dev/null
+++ b/sync/util/mock_unrecoverable_error_handler.h
@@ -0,0 +1,35 @@
+// Copyright 2015 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.
+
+#ifndef SYNC_UTIL_MOCK_UNRECOVERABLE_ERROR_HANDLER_H_
+#define SYNC_UTIL_MOCK_UNRECOVERABLE_ERROR_HANDLER_H_
+
+#include <string>
+
+#include "base/macros.h"
+#include "sync/internal_api/public/util/unrecoverable_error_handler.h"
+
+namespace syncer {
+
+// Mock implementation of UnrecoverableErrorHandler that counts how many times
+// it has been invoked.
+class MockUnrecoverableErrorHandler : public UnrecoverableErrorHandler {
+ public:
+  MockUnrecoverableErrorHandler();
+  ~MockUnrecoverableErrorHandler() override;
+  void OnUnrecoverableError(const tracked_objects::Location& from_here,
+                            const std::string& message) override;
+
+  // Returns the number of times this handler has been invoked.
+  int invocation_count() const;
+
+ private:
+  int invocation_count_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockUnrecoverableErrorHandler);
+};
+
+}  // namespace syncer
+
+#endif  // SYNC_UTIL_MOCK_UNRECOVERABLE_ERROR_HANDLER_H_