Revert 110177 - Sync: Improve handling of database load failures

   
This failure path has not received a lot of testing until now.  Here are
the issues addressed by this patch:
    
- We usually check the return value of step() calls.  However, we do not
check the return value of prepare() calls, which are more likely to fail.
If they do fail, we will DCHECK() or go on to dereference an invalid
pointer in step().  This patch checks the return value of one particular
prepare statement, the one in CheckIntegrity().
    
- Disable DCHECKs on sqlite errors, DirectoryManager open failure,
and SyncManager initialization failure.  This will allow us to test these
error paths.
    
- Be careful in ShutdownOnSyncThread().  The directory will not be fully
intialized during shutdown if the database load failed.

- Add a ProfileSyncService unit test that simulates a load from an
unreadable database.  The harness had to be modified slightly to make
this possible.

- Remove a setup_for_test_mode_ flag in SyncManager::SyncInternal::Init.
I don't know what the original intent of this flag was.  However, I do
know that it prevents me from properly simulating a database load failure
and removing it seems to have no ill effects.

- Do not delete the database from DirectoryBackingStore.  If this code
were to get executed it would put us into an inconsistent state.  See
issue 103824.  However, it's unlikely this code would get executed.  If
the database were actually corrupt, we would DCHECK or de-reference an
invalid pointer on our way to this code because we don't check the
return value of the attempt to prepare an SQL statement in
DirectoryBackingStore::CheckIntegrity().
    
- Modify the DirectoryBackingStoreTest.Corruption unit test to expect the
new behaviour.

- Disable sync when backend initialize fails.  Such a failure could
be due to bad local state.  We don't know the actual cause because the
information is not available from the ProfileSyncService callback.  The
safe course of action is to clear our local sync state and try again
later.  It's the easiest way to get back to the most well travelled sync
initialization path.

BUG=103307, 103824
TEST=DirectoryBackingStoreTest.Corruption, ProfileSyncServiceTest.CorruptDatabase

Review URL: http://codereview.chromium.org/8496002

TBR=rlarocque@chromium.org
Review URL: http://codereview.chromium.org/8570016

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@110182 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 62 insertions, 63 deletions

diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc
index c68682e..dd4ced4 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -420,11 +420,8 @@ void SyncBackendHost::Core::OnInitializationComplete(
       base::Bind(&Core::HandleInitializationCompletedOnFrontendLoop, this,
                  js_backend, success));
 
-  if (success) {
-    // Initialization is complete, so we can schedule recurring SaveChanges.
-    sync_loop_->PostTask(FROM_HERE,
-                         base::Bind(&Core::StartSavingChanges, this));
-  }
+  // Initialization is complete, so we can schedule recurring SaveChanges.
+  sync_loop_->PostTask(FROM_HERE, base::Bind(&Core::StartSavingChanges, this));
 }
 
 void SyncBackendHost::Core::OnAuthError(const AuthError& auth_error) {
@@ -611,7 +608,7 @@ void SyncBackendHost::Core::DoInitialize(const DoInitializeOptions& options) {
       host_->sync_notifier_factory_.CreateSyncNotifier(),
       options.restored_key_for_bootstrapping,
       options.setup_for_test_mode);
-  LOG_IF(ERROR, !success) << "Syncapi initialization failed!";
+  DCHECK(success) << "Syncapi initialization failed!";
 }
 
 void SyncBackendHost::Core::DoCheckServerReachable() {
diff --git a/chrome/browser/sync/internal_api/sync_manager.cc b/chrome/browser/sync/internal_api/sync_manager.cc
index 7f133ed..8359255a 100644
--- a/chrome/browser/sync/internal_api/sync_manager.cc
+++ b/chrome/browser/sync/internal_api/sync_manager.cc
@@ -802,7 +802,7 @@ bool SyncManager::SyncInternal::Init(
 
   bool signed_in = SignIn(credentials);
 
-  if (signed_in) {
+  if (signed_in || setup_for_test_mode_) {
     if (scheduler()) {
       scheduler()->Start(
           browser_sync::SyncScheduler::CONFIGURATION_MODE, base::Closure());
@@ -894,6 +894,7 @@ bool SyncManager::SyncInternal::OpenDirectory() {
   DCHECK(!initialized_) << "Should only happen once";
 
   bool share_opened = dir_manager()->Open(username_for_share(), this);
+  DCHECK(share_opened);
   if (!share_opened) {
     LOG(ERROR) << "Could not open share for:" << username_for_share();
     return false;
@@ -1290,7 +1291,7 @@ void SyncManager::SyncInternal::ShutdownOnSyncThread() {
   net::NetworkChangeNotifier::RemoveIPAddressObserver(this);
   observing_ip_address_changes_ = false;
 
-  if (initialized_ && dir_manager()) {
+  if (dir_manager()) {
     {
       // Cryptographer should only be accessed while holding a
       // transaction.
diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc
index 84af84f..26f089f8 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -555,16 +555,11 @@ void ProfileSyncService::OnUnrecoverableError(
 
 void ProfileSyncService::OnBackendInitialized(
     const WeakHandle<JsBackend>& js_backend, bool success) {
-  if (HasSyncSetupCompleted()) {
-    UMA_HISTOGRAM_BOOLEAN("Sync.FirstBackendInitializeSuccess", success);
-  } else {
-    UMA_HISTOGRAM_BOOLEAN("Sync.RestoreBackendInitializeSuccess", success);
-  }
-
   if (!success) {
-    // Something went unexpectedly wrong.  Play it safe: nuke our current state
-    // and prepare ourselves to try again later.
-    DisableForUser();
+    // If backend initialization failed, abort.  We only want to blow away
+    // state (DBs, etc) if this was a first-time scenario that failed.
+    wizard_.Step(SyncSetupWizard::FATAL_ERROR);
+    Shutdown(!HasSyncSetupCompleted());
     return;
   }
 
diff --git a/chrome/browser/sync/profile_sync_service_unittest.cc b/chrome/browser/sync/profile_sync_service_unittest.cc
index b2e7636..470ba48 100644
--- a/chrome/browser/sync/profile_sync_service_unittest.cc
+++ b/chrome/browser/sync/profile_sync_service_unittest.cc
@@ -77,15 +77,14 @@ class ProfileSyncServiceTest : public testing::Test {
   // TODO(akalin): Refactor the StartSyncService*() functions below.
 
   void StartSyncService() {
-    StartSyncServiceAndSetInitialSyncEnded(true, true, false, true, true);
+    StartSyncServiceAndSetInitialSyncEnded(true, true, false, true);
   }
 
   void StartSyncServiceAndSetInitialSyncEnded(
       bool set_initial_sync_ended,
       bool issue_auth_token,
       bool synchronous_sync_configuration,
-      bool sync_setup_completed,
-      bool expect_create_dtm) {
+      bool sync_setup_completed) {
     if (!service_.get()) {
       // Set bootstrap to true and it will provide a logged in user for test
       service_.reset(new TestProfileSyncService(&factory_,
@@ -98,13 +97,9 @@ class ProfileSyncServiceTest : public testing::Test {
       if (!sync_setup_completed)
         profile_->GetPrefs()->SetBoolean(prefs::kSyncHasSetupCompleted, false);
 
-      if (expect_create_dtm) {
-        // Register the bookmark data type.
-        EXPECT_CALL(factory_, CreateDataTypeManager(_, _)).
-            WillOnce(ReturnNewDataTypeManager());
-      } else {
-        EXPECT_CALL(factory_, CreateDataTypeManager(_, _)).Times(0);
-      }
+      // Register the bookmark data type.
+      EXPECT_CALL(factory_, CreateDataTypeManager(_, _)).
+          WillOnce(ReturnNewDataTypeManager());
 
       if (issue_auth_token) {
         profile_->GetTokenService()->IssueAuthTokenForTest(
@@ -197,7 +192,7 @@ TEST_F(ProfileSyncServiceTest, JsControllerHandlersBasic) {
 
 TEST_F(ProfileSyncServiceTest,
        JsControllerHandlersDelayedBackendInitialization) {
-  StartSyncServiceAndSetInitialSyncEnded(true, false, false, true, true);
+  StartSyncServiceAndSetInitialSyncEnded(true, false, false, true);
 
   StrictMock<MockJsEventHandler> event_handler;
   EXPECT_CALL(event_handler, HandleJsEvent(_, _)).Times(AtLeast(1));
@@ -239,7 +234,7 @@ TEST_F(ProfileSyncServiceTest, JsControllerProcessJsMessageBasic) {
 
 TEST_F(ProfileSyncServiceTest,
        JsControllerProcessJsMessageBasicDelayedBackendInitialization) {
-  StartSyncServiceAndSetInitialSyncEnded(true, false, false, true, true);
+  StartSyncServiceAndSetInitialSyncEnded(true, false, false, true);
 
   StrictMock<MockJsReplyHandler> reply_handler;
 
@@ -282,9 +277,8 @@ TEST_F(ProfileSyncServiceTest, TestStartupWithOldSyncData) {
   ASSERT_NE(-1,
             file_util::WriteFile(sync_file3, nonsense3, strlen(nonsense3)));
 
-  StartSyncServiceAndSetInitialSyncEnded(false, false, true, false, true);
+  StartSyncServiceAndSetInitialSyncEnded(false, false, true, false);
   EXPECT_FALSE(service_->HasSyncSetupCompleted());
-  EXPECT_FALSE(service_->sync_initialized());
 
   // Since we're doing synchronous initialization, backend should be
   // initialized by this call.
@@ -306,28 +300,6 @@ TEST_F(ProfileSyncServiceTest, TestStartupWithOldSyncData) {
   ASSERT_NE(file2text.compare(nonsense2), 0);
 }
 
-TEST_F(ProfileSyncServiceTest, CorruptDatabase) {
-  const char* nonesense = "not a database";
-
-  FilePath temp_directory = profile_->GetPath().AppendASCII("Sync Data");
-  FilePath sync_db_file = temp_directory.AppendASCII("SyncData.sqlite3");
-
-  ASSERT_TRUE(file_util::CreateDirectory(temp_directory));
-  ASSERT_NE(-1,
-            file_util::WriteFile(sync_db_file, nonesense, strlen(nonesense)));
-
-  // Initialize with HasSyncSetupCompleted() set to true and InitialSyncEnded
-  // false.  This is to model the scenario that would result when opening the
-  // sync database fails.
-  StartSyncServiceAndSetInitialSyncEnded(false, true, true, true, false);
-
-  // The backend is not ready.  Ensure the PSS knows this.
-  EXPECT_FALSE(service_->sync_initialized());
-
-  // Ensure we will be prepared to initialize a fresh DB next time.
-  EXPECT_FALSE(service_->HasSyncSetupCompleted());
-}
-
 }  // namespace
 
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/syncable/directory_backing_store.cc b/chrome/browser/sync/syncable/directory_backing_store.cc
index c037ff0..ed4242f 100644
--- a/chrome/browser/sync/syncable/directory_backing_store.cc
+++ b/chrome/browser/sync/syncable/directory_backing_store.cc
@@ -236,11 +236,7 @@ bool DirectoryBackingStore::OpenAndConfigureHandleHelper(
 bool DirectoryBackingStore::CheckIntegrity(sqlite3* handle, string* error)
     const {
   sqlite_utils::SQLStatement statement;
-  if (SQLITE_OK !=
-      statement.prepare(handle, "PRAGMA integrity_check(1)")) {
-    *error = sqlite3_errmsg(handle);
-    return false;
-  }
+  statement.prepare(handle, "PRAGMA integrity_check(1)");
   if (SQLITE_ROW != statement.step()) {
     *error =  sqlite3_errmsg(handle);
     return false;
@@ -293,7 +289,37 @@ DirOpenResult DirectoryBackingStore::Load(MetahandlesIndex* entry_bucket,
 
 bool DirectoryBackingStore::BeginLoad() {
   DCHECK(load_dbhandle_ == NULL);
-  return OpenAndConfigureHandleHelper(&load_dbhandle_);
+  bool ret = OpenAndConfigureHandleHelper(&load_dbhandle_);
+  if (ret)
+    return true;
+  // Something's gone wrong. Nuke the database and try again.
+  using ::operator<<;  // For string16.
+  LOG(ERROR) << "Sync database " << backing_filepath_.value()
+             << " corrupt. Deleting and recreating.";
+  file_util::Delete(backing_filepath_, false);
+  bool failed_again = !OpenAndConfigureHandleHelper(&load_dbhandle_);
+
+  // Using failed_again here lets us distinguish from cases where corruption
+  // occurred even when re-opening a fresh directory (they'll go in a separate
+  // double weight histogram bucket).  Failing twice in a row means we disable
+  // sync, so it's useful to see this number separately.
+  int bucket = failed_again ? 2 : 1;
+#if defined(OS_WIN)
+  UMA_HISTOGRAM_COUNTS_100("Sync.DirectoryOpenFailedWin", bucket);
+#elif defined(OS_MACOSX)
+  UMA_HISTOGRAM_COUNTS_100("Sync.DirectoryOpenFailedMac", bucket);
+#else
+  UMA_HISTOGRAM_COUNTS_100("Sync.DirectoryOpenFailedNotWinMac", bucket);
+
+#if defined(OS_CHROMEOS)
+  UMA_HISTOGRAM_COUNTS_100("Sync.DirectoryOpenFailedCros", bucket);
+#elif defined(OS_LINUX)
+  UMA_HISTOGRAM_COUNTS_100("Sync.DirectoryOpenFailedLinux", bucket);
+#else
+  UMA_HISTOGRAM_COUNTS_100("Sync.DirectoryOpenFailedOther", bucket);
+#endif  // OS_LINUX && !OS_CHROMEOS
+#endif  // OS_WIN
+  return !failed_again;
 }
 
 void DirectoryBackingStore::EndLoad() {
diff --git a/chrome/browser/sync/syncable/directory_backing_store_unittest.cc b/chrome/browser/sync/syncable/directory_backing_store_unittest.cc
index 461fac2..88536cf 100644
--- a/chrome/browser/sync/syncable/directory_backing_store_unittest.cc
+++ b/chrome/browser/sync/syncable/directory_backing_store_unittest.cc
@@ -2082,7 +2082,15 @@ TEST_F(DirectoryBackingStoreTest, Corruption) {
     scoped_ptr<DirectoryBackingStore> dbs(
         new DirectoryBackingStore(GetUsername(), GetDatabasePath()));
 
-    EXPECT_FALSE(dbs->BeginLoad());
+    // In release mode, we expect the sync database to nuke itself and start
+    // over if it detects invalid/corrupted data.
+#if defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON)
+    EXPECT_TRUE(dbs->BeginLoad());
+#elif defined(NDEBUG) && defined(DCHECK_ALWAYS_ON)
+    EXPECT_DEATH(dbs->BeginLoad(), "stmt_");
+#else
+    EXPECT_DEATH(dbs->BeginLoad(), "sqlite error");
+#endif
   }
 }
 
diff --git a/chrome/browser/sync/util/sqlite_utils.cc b/chrome/browser/sync/util/sqlite_utils.cc
index 7b75f0e..4cb6c4e 100644
--- a/chrome/browser/sync/util/sqlite_utils.cc
+++ b/chrome/browser/sync/util/sqlite_utils.cc
@@ -33,9 +33,9 @@ class DebugSQLErrorHandler: public VanillaSQLErrorHandler {
  public:
   virtual int HandleError(int error, sqlite3* db) {
     error_ = error;
-    LOG(ERROR) << "sqlite error " << error
-               << " " << sqlite3_errmsg(db)
-               << " db " << static_cast<void*>(db);
+    NOTREACHED() << "sqlite error " << error
+                 << " " << sqlite3_errmsg(db)
+                 << " db " << static_cast<void*>(db);
     return error;
   }
 };