Fix SyncManager initialization failure crash.

This crash was introduced in r148926, which changes SyncManager init
behaviour.  Previously, the SyncManager would end up with a valid
SyncScheduler regardless of whether or not the initialization failed.
This SyncScheduler would later play an important role in shutting down
the syncer.  Without a scheduler, the shutdown process crashes.

The ProfileSyncServiceTest.CorruptDatabase test simulates the scenario
we fail to load a sync directory.  This will result in a SyncManager
initialization failure.  It is intended to test that this failure is
handled by the ProfileSyncService, SyncBackendHost and SyncManager.

BUG=139723


Review URL: https://chromiumcodereview.appspot.com/10830100

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

4 files changed, 46 insertions, 48 deletions

diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc
index 8c0d8c7..8a98cd5 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -846,6 +846,13 @@ void SyncBackendHost::Core::OnInitializationComplete(
     bool success,
     const syncer::ModelTypeSet restored_types) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
+
+  if (!success) {
+    sync_manager_->RemoveObserver(this);
+    sync_manager_->ShutdownOnSyncThread();
+    sync_manager_.reset();
+  }
+
   host_.Call(
       FROM_HERE,
       &SyncBackendHost::HandleSyncManagerInitializationOnFrontendLoop,
@@ -1053,19 +1060,22 @@ void SyncBackendHost::Core::DoRefreshNigori(
 
 void SyncBackendHost::Core::DoStopSyncManagerForShutdown(
     const base::Closure& closure) {
-  DCHECK(sync_manager_.get());
-  sync_manager_->StopSyncingForShutdown(closure);
+  if (sync_manager_.get()) {
+    sync_manager_->StopSyncingForShutdown(closure);
+  } else {
+    sync_loop_->PostTask(FROM_HERE, closure);
+  }
 }
 
 void SyncBackendHost::Core::DoShutdown(bool sync_disabled) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  if (!sync_manager_.get())
-    return;
+  if (sync_manager_.get()) {
+    save_changes_timer_.reset();
+    sync_manager_->ShutdownOnSyncThread();
+    sync_manager_->RemoveObserver(this);
+    sync_manager_.reset();
+  }
 
-  save_changes_timer_.reset();
-  sync_manager_->ShutdownOnSyncThread();
-  sync_manager_->RemoveObserver(this);
-  sync_manager_.reset();
   chrome_sync_notification_bridge_ = NULL;
   registrar_ = NULL;
 
diff --git a/chrome/browser/sync/profile_sync_service_unittest.cc b/chrome/browser/sync/profile_sync_service_unittest.cc
index a6f692f..1d589b3 100644
--- a/chrome/browser/sync/profile_sync_service_unittest.cc
+++ b/chrome/browser/sync/profile_sync_service_unittest.cc
@@ -82,7 +82,7 @@ class ProfileSyncServiceTest : public testing::Test {
 
   void StartSyncService() {
     StartSyncServiceAndSetInitialSyncEnded(
-        true, true, false, true, true, false);
+        true, true, false, true, true, syncer::STORAGE_IN_MEMORY);
   }
 
   void StartSyncServiceAndSetInitialSyncEnded(
@@ -91,7 +91,7 @@ class ProfileSyncServiceTest : public testing::Test {
       bool synchronous_sync_configuration,
       bool sync_setup_completed,
       bool expect_create_dtm,
-      bool use_real_database) {
+      syncer::StorageOption storage_option) {
     if (!service_.get()) {
       SigninManager* signin =
           SigninManagerFactory::GetForProfile(profile_.get());
@@ -109,8 +109,7 @@ class ProfileSyncServiceTest : public testing::Test {
         service_->dont_set_initial_sync_ended_on_init();
       if (synchronous_sync_configuration)
         service_->set_synchronous_sync_configuration();
-      if (use_real_database)
-        service_->set_use_real_database();
+      service_->set_storage_option(storage_option);
       if (!sync_setup_completed)
         profile_->GetPrefs()->SetBoolean(prefs::kSyncHasSetupCompleted, false);
 
@@ -253,7 +252,8 @@ TEST_F(ProfileSyncServiceTest, JsControllerHandlersBasic) {
 
 TEST_F(ProfileSyncServiceTest,
        JsControllerHandlersDelayedBackendInitialization) {
-  StartSyncServiceAndSetInitialSyncEnded(true, false, false, true, true, false);
+  StartSyncServiceAndSetInitialSyncEnded(true, false, false, true, true,
+                                         syncer::STORAGE_IN_MEMORY);
 
   StrictMock<syncer::MockJsEventHandler> event_handler;
   EXPECT_CALL(event_handler, HandleJsEvent(_, _)).Times(AtLeast(1));
@@ -294,7 +294,8 @@ TEST_F(ProfileSyncServiceTest, JsControllerProcessJsMessageBasic) {
 
 TEST_F(ProfileSyncServiceTest,
        JsControllerProcessJsMessageBasicDelayedBackendInitialization) {
-  StartSyncServiceAndSetInitialSyncEnded(true, false, false, true, true, false);
+  StartSyncServiceAndSetInitialSyncEnded(true, false, false, true, true,
+                                         syncer::STORAGE_IN_MEMORY);
 
   StrictMock<syncer::MockJsReplyHandler> reply_handler;
 
@@ -336,7 +337,8 @@ TEST_F(ProfileSyncServiceTest, TestStartupWithOldSyncData) {
   ASSERT_NE(-1,
             file_util::WriteFile(sync_file3, nonsense3, strlen(nonsense3)));
 
-  StartSyncServiceAndSetInitialSyncEnded(false, false, true, false, true, true);
+  StartSyncServiceAndSetInitialSyncEnded(false, false, true, false, true,
+                                         syncer::STORAGE_ON_DISK);
   EXPECT_FALSE(service_->HasSyncSetupCompleted());
   EXPECT_FALSE(service_->sync_initialized());
 
@@ -359,29 +361,16 @@ TEST_F(ProfileSyncServiceTest, TestStartupWithOldSyncData) {
   ASSERT_NE(file2text.compare(nonsense2), 0);
 }
 
-// Disabled because of crbug.com/109668.
-TEST_F(ProfileSyncServiceTest, DISABLED_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, true);
+// Simulates a scenario where a database is corrupted and it is impossible to
+// recreate it.  This test is useful mainly when it is run under valgrind.  Its
+// expectations are not very interesting.
+TEST_F(ProfileSyncServiceTest, FailToOpenDatabase) {
+  StartSyncServiceAndSetInitialSyncEnded(false, true, true, true, false,
+                                         syncer::STORAGE_INVALID);
 
   // 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/test_profile_sync_service.cc b/chrome/browser/sync/test_profile_sync_service.cc
index c98ffc3..8eb7365 100644
--- a/chrome/browser/sync/test_profile_sync_service.cc
+++ b/chrome/browser/sync/test_profile_sync_service.cc
@@ -12,7 +12,6 @@
 #include "chrome/browser/sync/test/test_http_bridge_factory.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "sync/internal_api/public/sessions/sync_session_snapshot.h"
-#include "sync/internal_api/public/test/test_internal_components_factory.h"
 #include "sync/internal_api/public/user_share.h"
 #include "sync/js/js_reply_handler.h"
 #include "sync/protocol/encryption.pb.h"
@@ -37,7 +36,7 @@ SyncBackendHostForProfileSyncTest::SyncBackendHostForProfileSyncTest(
     bool set_initial_sync_ended_on_init,
     bool synchronous_init,
     bool fail_initial_download,
-    bool use_real_database)
+    syncer::StorageOption storage_option)
     : browser_sync::SyncBackendHost(
         profile->GetDebugName(), profile, sync_prefs, invalidator_storage),
       id_factory_(id_factory),
@@ -45,7 +44,7 @@ SyncBackendHostForProfileSyncTest::SyncBackendHostForProfileSyncTest(
       set_initial_sync_ended_on_init_(set_initial_sync_ended_on_init),
       synchronous_init_(synchronous_init),
       fail_initial_download_(fail_initial_download),
-      use_real_database_(use_real_database) {}
+      storage_option_(storage_option) {}
 
 SyncBackendHostForProfileSyncTest::~SyncBackendHostForProfileSyncTest() {}
 
@@ -66,9 +65,7 @@ void SyncBackendHostForProfileSyncTest::InitCore(
   test_options.credentials.email = "testuser@gmail.com";
   test_options.credentials.sync_token = "token";
   test_options.restored_key_for_bootstrapping = "";
-  TestInternalComponentsFactory::StorageOption storage =
-      use_real_database_ ? TestInternalComponentsFactory::ON_DISK
-                         : TestInternalComponentsFactory::IN_MEMORY;
+  syncer::StorageOption storage = storage_option_;
 
   // It'd be nice if we avoided creating the InternalComponentsFactory in the
   // first place, but SyncBackendHost will have created one by now so we must
@@ -174,7 +171,7 @@ TestProfileSyncService::TestProfileSyncService(
       callback_(callback),
       set_initial_sync_ended_on_init_(true),
       fail_initial_download_(false),
-      use_real_database_(false) {
+      storage_option_(syncer::STORAGE_IN_MEMORY) {
   SetSyncSetupCompleted();
 }
 
@@ -216,8 +213,9 @@ void TestProfileSyncService::set_synchronous_sync_configuration() {
 void TestProfileSyncService::fail_initial_download() {
   fail_initial_download_ = true;
 }
-void TestProfileSyncService::set_use_real_database() {
-  use_real_database_ = true;
+void TestProfileSyncService::set_storage_option(
+    syncer::StorageOption storage_option) {
+  storage_option_ = storage_option;
 }
 
 void TestProfileSyncService::CreateBackend() {
@@ -230,5 +228,5 @@ void TestProfileSyncService::CreateBackend() {
       set_initial_sync_ended_on_init_,
       synchronous_backend_initialization_,
       fail_initial_download_,
-      use_real_database_));
+      storage_option_));
 }
diff --git a/chrome/browser/sync/test_profile_sync_service.h b/chrome/browser/sync/test_profile_sync_service.h
index 95a11da..d24c0fd 100644
--- a/chrome/browser/sync/test_profile_sync_service.h
+++ b/chrome/browser/sync/test_profile_sync_service.h
@@ -15,6 +15,7 @@
 #include "chrome/browser/sync/profile_sync_service.h"
 #include "chrome/browser/sync/sync_prefs.h"
 #include "chrome/test/base/profile_mock.h"
+#include "sync/internal_api/public/test/test_internal_components_factory.h"
 #include "sync/test/engine/test_id_factory.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
@@ -41,7 +42,7 @@ class SyncBackendHostForProfileSyncTest : public SyncBackendHost {
       bool set_initial_sync_ended_on_init,
       bool synchronous_init,
       bool fail_initial_download,
-      bool use_real_database);
+      syncer::StorageOption storage_option);
   virtual ~SyncBackendHostForProfileSyncTest();
 
   MOCK_METHOD1(RequestNudge, void(const tracked_objects::Location&));
@@ -72,7 +73,7 @@ class SyncBackendHostForProfileSyncTest : public SyncBackendHost {
   bool set_initial_sync_ended_on_init_;
   bool synchronous_init_;
   bool fail_initial_download_;
-  bool use_real_database_;
+  syncer::StorageOption storage_option_;
 };
 
 }  // namespace browser_sync
@@ -110,7 +111,7 @@ class TestProfileSyncService : public ProfileSyncService {
   void set_synchronous_sync_configuration();
 
   void fail_initial_download();
-  void set_use_real_database();
+  void set_storage_option(syncer::StorageOption option);
 
   syncer::TestIdFactory* id_factory();
 
@@ -136,7 +137,7 @@ class TestProfileSyncService : public ProfileSyncService {
   bool set_initial_sync_ended_on_init_;
 
   bool fail_initial_download_;
-  bool use_real_database_;
+  syncer::StorageOption storage_option_;
 };
 
 #endif  // CHROME_BROWSER_SYNC_TEST_PROFILE_SYNC_SERVICE_H_