From 6d60180f3ca3f9289d37b27e77f90d97db488f7c Mon Sep 17 00:00:00 2001 From: vkuzkokov Date: Tue, 28 Apr 2015 04:27:33 -0700 Subject: 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} --- sync/BUILD.gn | 4 --- sync/sync_tests.gypi | 4 --- sync/syncable/directory.cc | 18 ++--------- sync/syncable/directory.h | 7 ----- sync/syncable/directory_backing_store_unittest.cc | 31 ++++++++++++++----- sync/syncable/directory_unittest.cc | 20 ------------- sync/syncable/on_disk_directory_backing_store.cc | 5 ++-- sync/syncable/on_disk_directory_backing_store.h | 4 +-- .../directory_backing_store_corruption_testing.cc | 35 ---------------------- .../directory_backing_store_corruption_testing.h | 27 ----------------- sync/util/mock_unrecoverable_error_handler.cc | 26 ---------------- sync/util/mock_unrecoverable_error_handler.h | 35 ---------------------- 12 files changed, 31 insertions(+), 185 deletions(-) delete mode 100644 sync/test/directory_backing_store_corruption_testing.cc delete mode 100644 sync/test/directory_backing_store_corruption_testing.h delete mode 100644 sync/util/mock_unrecoverable_error_handler.cc delete mode 100644 sync/util/mock_unrecoverable_error_handler.h (limited to 'sync') 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* 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 delete_journal_; - base::WeakPtrFactory 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 #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 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 - -#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_ -- cgit v1.1