Revert of [Sync] Erase sync DB when corrupted (patchset #4 id:60001 of https://codereview.chromium.org/1082893003/)

Reason for revert:
Test fails on Win7 bots. see crbug.com/481895

Original issue's description:
> [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}

TBR=zea@chromium.org,pvalenzuela@chromium.org,maniscalco@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=470993

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

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

12 files changed, 31 insertions, 185 deletions

diff --git a/sync/BUILD.gn b/sync/BUILD.gn
index bfb3566..d6cec92 100644
--- a/sync/BUILD.gn
+++ b/sync/BUILD.gn
@@ -444,8 +444,6 @@ 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",
@@ -490,8 +488,6 @@ 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 69804e1..33594a1 100644
--- a/sync/sync_tests.gypi
+++ b/sync/sync_tests.gypi
@@ -35,8 +35,6 @@
         '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',
@@ -81,8 +79,6 @@
         '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 0f680d22..767b807 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),
-      weak_ptr_factory_(this) {
+      invariant_check_level_(VERIFY_CHANGES) {
 }
 
 Directory::~Directory() {
@@ -208,12 +208,6 @@ 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;
 }
 
@@ -1560,12 +1554,6 @@ 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 4f45cdf..7f6ae0e 100644
--- a/sync/syncable/directory.h
+++ b/sync/syncable/directory.h
@@ -533,7 +533,6 @@ 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
@@ -623,10 +622,6 @@ 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);
 
@@ -658,8 +653,6 @@ 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 0cc723f..435533c 100644
--- a/sync/syncable/directory_backing_store_unittest.cc
+++ b/sync/syncable/directory_backing_store_unittest.cc
@@ -7,6 +7,8 @@
 #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"
@@ -25,7 +27,6 @@
 #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"
@@ -4094,6 +4095,7 @@ 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()));
@@ -4102,19 +4104,32 @@ TEST_F(DirectoryBackingStoreTest,
   ASSERT_TRUE(LoadAndIgnoreReturnedData(dbs.get()));
   ASSERT_FALSE(dbs->DidFailFirstOpenAttempt());
   Directory::SaveChangesSnapshot snapshot;
-  for (int i = 0; i < corruption_testing::kNumEntriesRequiredForCorruption;
-       ++i) {
+  const int num_entries = 4000;
+  for (int i = 0; i < num_entries; ++i) {
     snapshot.dirty_metas.insert(CreateEntry(i).release());
   }
   ASSERT_TRUE(dbs->SaveChanges(snapshot));
-  // Corrupt it.
-  ASSERT_TRUE(corruption_testing::CorruptDatabase(GetDatabasePath()));
+
+  // 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()));
+  }
+
   // 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 you need
-  // to increase the number of entries written to the DB. See also
-  // |kNumEntriesRequiredForCorruption|.
+  // 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.
   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 690af4f..d802d4f 100644
--- a/sync/syncable/directory_unittest.cc
+++ b/sync/syncable/directory_unittest.cc
@@ -12,7 +12,6 @@
 #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;
@@ -2031,25 +2030,6 @@ 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 3a37847..918dc07 100644
--- a/sync/syncable/on_disk_directory_backing_store.cc
+++ b/sync/syncable/on_disk_directory_backing_store.cc
@@ -26,8 +26,9 @@ enum HistogramResultEnum {
 OnDiskDirectoryBackingStore::OnDiskDirectoryBackingStore(
     const std::string& dir_name,
     const base::FilePath& backing_file_path)
-    : DirectoryBackingStore(dir_name), backing_file_path_(backing_file_path) {
-  DCHECK(backing_file_path_.IsAbsolute());
+    : DirectoryBackingStore(dir_name),
+      allow_failure_for_test_(false),
+      backing_file_path_(backing_file_path) {
 }
 
 OnDiskDirectoryBackingStore::~OnDiskDirectoryBackingStore() {
diff --git a/sync/syncable/on_disk_directory_backing_store.h b/sync/syncable/on_disk_directory_backing_store.h
index 05f4156..8c604d0 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);
 
-  // The path to the sync DB.
-  const base::FilePath backing_file_path_;
+  bool allow_failure_for_test_;
+  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
deleted file mode 100644
index 9767e56..0000000
--- a/sync/test/directory_backing_store_corruption_testing.cc
+++ /dev/null
@@ -1,35 +0,0 @@
-// 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
deleted file mode 100644
index 5897ec0..0000000
--- a/sync/test/directory_backing_store_corruption_testing.h
+++ /dev/null
@@ -1,27 +0,0 @@
-// 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
deleted file mode 100644
index bfed1f1..0000000
--- a/sync/util/mock_unrecoverable_error_handler.cc
+++ /dev/null
@@ -1,26 +0,0 @@
-// 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
deleted file mode 100644
index aacde89..0000000
--- a/sync/util/mock_unrecoverable_error_handler.h
+++ /dev/null
@@ -1,35 +0,0 @@
-// 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_