Properly handle initialization failure and following manager shutdown without

crashing.

BUG=388948

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

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

6 files changed, 143 insertions, 36 deletions

diff --git a/sync/internal_api/sync_backup_manager.cc b/sync/internal_api/sync_backup_manager.cc
index d421dfd..91927e7 100644
--- a/sync/internal_api/sync_backup_manager.cc
+++ b/sync/internal_api/sync_backup_manager.cc
@@ -50,17 +50,22 @@ void SyncBackupManager::Init(
                                 report_unrecoverable_error_function,
                                 cancelation_signal);
 
+  if (!initialized())
+    return;
+
   GetUserShare()->directory->CollectMetaHandleCounts(
-      &status_.num_entries_by_type,
-      &status_.num_to_delete_entries_by_type);
+    &status_.num_entries_by_type,
+    &status_.num_to_delete_entries_by_type);
 
   HideSyncPreference(PRIORITY_PREFERENCES);
   HideSyncPreference(PREFERENCES);
 }
 
 void SyncBackupManager::SaveChanges() {
-  NormalizeEntries();
-  GetUserShare()->directory->SaveChanges();
+  if (initialized()) {
+    NormalizeEntries();
+    GetUserShare()->directory->SaveChanges();
+  }
 }
 
 SyncStatus SyncBackupManager::GetDetailedStatus() const {
diff --git a/sync/internal_api/sync_backup_manager_unittest.cc b/sync/internal_api/sync_backup_manager_unittest.cc
index a130a63..c075354 100644
--- a/sync/internal_api/sync_backup_manager_unittest.cc
+++ b/sync/internal_api/sync_backup_manager_unittest.cc
@@ -5,15 +5,22 @@
 #include "sync/internal_api/sync_backup_manager.h"
 
 #include "base/files/scoped_temp_dir.h"
+#include "base/run_loop.h"
 #include "sync/internal_api/public/read_node.h"
 #include "sync/internal_api/public/read_transaction.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/write_node.h"
 #include "sync/internal_api/public/write_transaction.h"
 #include "sync/syncable/entry.h"
 #include "sync/test/test_directory_backing_store.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using ::testing::_;
+using ::testing::Invoke;
+using ::testing::WithArgs;
+
 namespace syncer {
 
 namespace {
@@ -22,16 +29,36 @@ void OnConfigDone(bool success) {
   EXPECT_TRUE(success);
 }
 
-class SyncBackupManagerTest : public testing::Test {
+class SyncBackupManagerTest : public syncer::SyncManager::Observer,
+                              public testing::Test {
+ public:
+  MOCK_METHOD1(OnSyncCycleCompleted,
+               void(const sessions::SyncSessionSnapshot&));
+  MOCK_METHOD1(OnConnectionStatusChange, void(ConnectionStatus));
+  MOCK_METHOD1(OnActionableError, void(const SyncProtocolError&));
+  MOCK_METHOD1(OnMigrationRequested, void(ModelTypeSet));;
+  MOCK_METHOD1(OnProtocolEvent, void(const ProtocolEvent&));
+  MOCK_METHOD4(OnInitializationComplete,
+               void(const WeakHandle<JsBackend>&,
+                    const WeakHandle<DataTypeDebugInfoListener>&,
+                    bool, ModelTypeSet));
+
  protected:
   virtual void SetUp() OVERRIDE {
     CHECK(temp_dir_.CreateUniqueTempDir());
   }
 
-  void InitManager(SyncManager* manager) {
+  void InitManager(SyncManager* manager, StorageOption storage_option) {
+    manager_ = manager;
+    EXPECT_CALL(*this, OnInitializationComplete(_, _, _, _))
+        .WillOnce(WithArgs<2>(Invoke(this,
+                                     &SyncBackupManagerTest::HandleInit)));
+
     TestInternalComponentsFactory factory(InternalComponentsFactory::Switches(),
-                                          STORAGE_ON_DISK);
+                                          storage_option);
+    manager->AddObserver(this);
 
+    base::RunLoop run_loop;
     manager->Init(temp_dir_.path(),
                   MakeWeakHandle(base::WeakPtr<JsEventHandler>()),
                   "", 0, true, scoped_ptr<HttpPostProviderFactory>().Pass(),
@@ -39,13 +66,8 @@ class SyncBackupManagerTest : public testing::Test {
                   NULL, NULL, SyncCredentials(), "", "", "", &factory,
                   NULL, scoped_ptr<UnrecoverableErrorHandler>().Pass(),
                   NULL, NULL);
-    manager->ConfigureSyncer(
-          CONFIGURE_REASON_NEW_CLIENT,
-          ModelTypeSet(SEARCH_ENGINES),
-          ModelTypeSet(), ModelTypeSet(), ModelTypeSet(),
-          ModelSafeRoutingInfo(),
-          base::Bind(&OnConfigDone, true),
-          base::Bind(&OnConfigDone, false));
+    loop_.PostTask(FROM_HERE, run_loop.QuitClosure());
+    run_loop.Run();
   }
 
   void CreateEntry(UserShare* user_share, ModelType type,
@@ -59,13 +81,34 @@ class SyncBackupManagerTest : public testing::Test {
               node.InitUniqueByCreation(type, type_root, client_tag));
   }
 
+ private:
+  void ConfigureSyncer() {
+    manager_->ConfigureSyncer(CONFIGURE_REASON_NEW_CLIENT,
+                              ModelTypeSet(SEARCH_ENGINES),
+                              ModelTypeSet(), ModelTypeSet(), ModelTypeSet(),
+                              ModelSafeRoutingInfo(),
+                              base::Bind(&OnConfigDone, true),
+                              base::Bind(&OnConfigDone, false));
+  }
+
+  void HandleInit(bool success) {
+    if (success) {
+      loop_.PostTask(FROM_HERE,
+                     base::Bind(&SyncBackupManagerTest::ConfigureSyncer,
+                                base::Unretained(this)));
+    } else {
+      manager_->ShutdownOnSyncThread();
+    }
+  }
+
   base::ScopedTempDir temp_dir_;
   base::MessageLoop loop_;    // Needed for WeakHandle
+  SyncManager* manager_;
 };
 
 TEST_F(SyncBackupManagerTest, NormalizeAndPersist) {
   scoped_ptr<SyncBackupManager> manager(new SyncBackupManager);
-  InitManager(manager.get());
+  InitManager(manager.get(), STORAGE_ON_DISK);
 
   CreateEntry(manager->GetUserShare(), SEARCH_ENGINES, "test");
 
@@ -94,7 +137,7 @@ TEST_F(SyncBackupManagerTest, NormalizeAndPersist) {
 
   // Reopen db to verify entry is persisted.
   manager.reset(new SyncBackupManager);
-  InitManager(manager.get());
+  InitManager(manager.get(), STORAGE_ON_DISK);
   {
     ReadTransaction trans(FROM_HERE, manager->GetUserShare());
     ReadNode pref(&trans);
@@ -105,6 +148,12 @@ TEST_F(SyncBackupManagerTest, NormalizeAndPersist) {
   }
 }
 
+TEST_F(SyncBackupManagerTest, FailToInitialize) {
+  // Test graceful shutdown on initialization failure.
+  scoped_ptr<SyncBackupManager> manager(new SyncBackupManager);
+  InitManager(manager.get(), STORAGE_INVALID);
+}
+
 }  // anonymous namespace
 
 }  // namespace syncer
diff --git a/sync/internal_api/sync_rollback_manager.cc b/sync/internal_api/sync_rollback_manager.cc
index 84367b0..9a54f03 100644
--- a/sync/internal_api/sync_rollback_manager.cc
+++ b/sync/internal_api/sync_rollback_manager.cc
@@ -53,6 +53,9 @@ void SyncRollbackManager::Init(
                                 report_unrecoverable_error_function,
                                 cancelation_signal);
 
+  if (!initialized())
+    return;
+
   change_delegate_ = change_delegate;
 
   for (size_t i = 0; i < workers.size(); ++i) {
diff --git a/sync/internal_api/sync_rollback_manager_base.cc b/sync/internal_api/sync_rollback_manager_base.cc
index 6f0e534..28afb8b 100644
--- a/sync/internal_api/sync_rollback_manager_base.cc
+++ b/sync/internal_api/sync_rollback_manager_base.cc
@@ -43,7 +43,8 @@ namespace syncer {
 SyncRollbackManagerBase::SyncRollbackManagerBase()
     : report_unrecoverable_error_function_(NULL),
       weak_ptr_factory_(this),
-      dummy_handler_(new DummyEntryptionHandler) {
+      dummy_handler_(new DummyEntryptionHandler),
+      initialized_(false) {
 }
 
 SyncRollbackManagerBase::~SyncRollbackManagerBase() {
@@ -77,6 +78,7 @@ void SyncRollbackManagerBase::Init(
     return;
   }
 
+  initialized_ = true;
   NotifyInitializationSuccess();
 }
 
@@ -151,10 +153,11 @@ void SyncRollbackManagerBase::SaveChanges() {
 }
 
 void SyncRollbackManagerBase::ShutdownOnSyncThread() {
-  if (share_.directory) {
+  if (initialized_) {
     SaveChanges();
     share_.directory->Close();
     share_.directory.reset();
+    initialized_ = false;
   }
 }
 
diff --git a/sync/internal_api/sync_rollback_manager_base.h b/sync/internal_api/sync_rollback_manager_base.h
index 17892a8e..8d32d7d 100644
--- a/sync/internal_api/sync_rollback_manager_base.h
+++ b/sync/internal_api/sync_rollback_manager_base.h
@@ -123,6 +123,10 @@ class SYNC_EXPORT_PRIVATE SyncRollbackManagerBase :
       syncer::TypeDebugInfoObserver* observer) OVERRIDE;
   virtual void RequestEmitDebugInfo() OVERRIDE;
 
+  bool initialized() const {
+    return initialized_;
+  }
+
  private:
   void NotifyInitializationSuccess();
   void NotifyInitializationFailure();
@@ -144,6 +148,8 @@ class SYNC_EXPORT_PRIVATE SyncRollbackManagerBase :
 
   scoped_ptr<SyncEncryptionHandler> dummy_handler_;
 
+  bool initialized_;
+
   DISALLOW_COPY_AND_ASSIGN(SyncRollbackManagerBase);
 };
 
diff --git a/sync/internal_api/sync_rollback_manager_unittest.cc b/sync/internal_api/sync_rollback_manager_unittest.cc
index d66ee06..11f875f 100644
--- a/sync/internal_api/sync_rollback_manager_unittest.cc
+++ b/sync/internal_api/sync_rollback_manager_unittest.cc
@@ -5,6 +5,7 @@
 #include "sync/internal_api/sync_rollback_manager.h"
 
 #include "base/files/scoped_temp_dir.h"
+#include "base/run_loop.h"
 #include "sync/internal_api/public/read_node.h"
 #include "sync/internal_api/public/read_transaction.h"
 #include "sync/internal_api/public/sessions/sync_session_snapshot.h"
@@ -76,9 +77,9 @@ class SyncRollbackManagerTest : public testing::Test,
                void(const sessions::SyncSessionSnapshot&));
   MOCK_METHOD1(OnConnectionStatusChange, void(ConnectionStatus));
   MOCK_METHOD4(OnInitializationComplete,
-      void(const WeakHandle<JsBackend>&,
-           const WeakHandle<DataTypeDebugInfoListener>&,
-           bool, ModelTypeSet));
+               void(const WeakHandle<JsBackend>&,
+                    const WeakHandle<DataTypeDebugInfoListener>&,
+                    bool, ModelTypeSet));
   MOCK_METHOD1(OnActionableError, void(const SyncProtocolError&));
   MOCK_METHOD1(OnMigrationRequested, void(ModelTypeSet));;
   MOCK_METHOD1(OnProtocolEvent, void(const ProtocolEvent&));
@@ -100,11 +101,19 @@ class SyncRollbackManagerTest : public testing::Test,
   }
 
   void InitManager(SyncManager* manager, ModelTypeSet types,
-                   TestChangeDelegate* delegate) {
+                   TestChangeDelegate* delegate, StorageOption storage_option) {
+    manager_ = manager;
+    types_ = types;
+
+    EXPECT_CALL(*this, OnInitializationComplete(_, _, _, _))
+        .WillOnce(WithArgs<2>(Invoke(this,
+                                     &SyncRollbackManagerTest::HandleInit)));
+
     manager->AddObserver(this);
     TestInternalComponentsFactory factory(InternalComponentsFactory::Switches(),
-                                          STORAGE_ON_DISK);
+                                          storage_option);
 
+    base::RunLoop run_loop;
     manager->Init(temp_dir_.path(),
                   MakeWeakHandle(base::WeakPtr<JsEventHandler>()),
                   "", 0, true, scoped_ptr<HttpPostProviderFactory>().Pass(),
@@ -113,21 +122,16 @@ class SyncRollbackManagerTest : public testing::Test,
                   NULL, delegate, SyncCredentials(), "", "", "", &factory,
                   NULL, scoped_ptr<UnrecoverableErrorHandler>().Pass(),
                   NULL, NULL);
-    manager->ConfigureSyncer(
-        CONFIGURE_REASON_NEW_CLIENT,
-        types,
-        ModelTypeSet(), ModelTypeSet(), ModelTypeSet(), ModelSafeRoutingInfo(),
-        base::Bind(&SyncRollbackManagerTest::OnConfigDone,
-                   base::Unretained(this), true),
-        base::Bind(&SyncRollbackManagerTest::OnConfigDone,
-                   base::Unretained(this), false));
+    loop_.PostTask(FROM_HERE, run_loop.QuitClosure());
+    run_loop.Run();
   }
 
   // Create and persist an entry by unique tag in DB.
   void PrepopulateDb(ModelType type, const std::string& client_tag) {
     SyncBackupManager backup_manager;
     TestChangeDelegate delegate;
-    InitManager(&backup_manager, ModelTypeSet(type), &delegate);
+    InitManager(&backup_manager, ModelTypeSet(type), &delegate,
+                STORAGE_ON_DISK);
     CreateEntry(backup_manager.GetUserShare(), type, client_tag);
     backup_manager.ShutdownOnSyncThread();
   }
@@ -140,9 +144,34 @@ class SyncRollbackManagerTest : public testing::Test,
     return BaseNode::INIT_OK == node.InitByClientTagLookup(type, client_tag);
   }
 
+ private:
+  void ConfigureSyncer() {
+    manager_->ConfigureSyncer(
+          CONFIGURE_REASON_NEW_CLIENT,
+          types_,
+          ModelTypeSet(), ModelTypeSet(), ModelTypeSet(),
+          ModelSafeRoutingInfo(),
+          base::Bind(&SyncRollbackManagerTest::OnConfigDone,
+                     base::Unretained(this), true),
+          base::Bind(&SyncRollbackManagerTest::OnConfigDone,
+                     base::Unretained(this), false));
+  }
+
+  void HandleInit(bool success) {
+    if (success) {
+      loop_.PostTask(FROM_HERE,
+                     base::Bind(&SyncRollbackManagerTest::ConfigureSyncer,
+                                base::Unretained(this)));
+    } else {
+      manager_->ShutdownOnSyncThread();
+    }
+  }
+
   base::ScopedTempDir temp_dir_;
   scoped_refptr<ModelSafeWorker> worker_;
   base::MessageLoop loop_;    // Needed for WeakHandle
+  SyncManager* manager_;
+  ModelTypeSet types_;
 };
 
 bool IsRollbackDoneAction(SyncProtocolError e) {
@@ -154,7 +183,8 @@ TEST_F(SyncRollbackManagerTest, RollbackBasic) {
 
   TestChangeDelegate delegate;
   SyncRollbackManager rollback_manager;
-  InitManager(&rollback_manager, ModelTypeSet(PREFERENCES), &delegate);
+  InitManager(&rollback_manager, ModelTypeSet(PREFERENCES), &delegate,
+              STORAGE_ON_DISK);
 
   // Simulate a new entry added during type initialization.
   int64 new_pref_id =
@@ -177,7 +207,8 @@ TEST_F(SyncRollbackManagerTest, NoRollbackOfTypesNotBackedUp) {
 
   TestChangeDelegate delegate;
   SyncRollbackManager rollback_manager;
-  InitManager(&rollback_manager, ModelTypeSet(PREFERENCES, APPS), &delegate);
+  InitManager(&rollback_manager, ModelTypeSet(PREFERENCES, APPS), &delegate,
+              STORAGE_ON_DISK);
 
   // Simulate new entry added during type initialization.
   int64 new_pref_id =
@@ -204,7 +235,8 @@ TEST_F(SyncRollbackManagerTest, BackupDbNotChangedOnAbort) {
   TestChangeDelegate delegate;
   scoped_ptr<SyncRollbackManager> rollback_manager(
       new SyncRollbackManager);
-  InitManager(rollback_manager.get(), ModelTypeSet(PREFERENCES), &delegate);
+  InitManager(rollback_manager.get(), ModelTypeSet(PREFERENCES), &delegate,
+              STORAGE_ON_DISK);
 
   // Simulate a new entry added during type initialization.
   CreateEntry(rollback_manager->GetUserShare(), PREFERENCES, "pref2");
@@ -214,11 +246,20 @@ TEST_F(SyncRollbackManagerTest, BackupDbNotChangedOnAbort) {
 
   // Verify new entry was not persisted.
   rollback_manager.reset(new SyncRollbackManager);
-  InitManager(rollback_manager.get(), ModelTypeSet(PREFERENCES), &delegate);
+  InitManager(rollback_manager.get(), ModelTypeSet(PREFERENCES), &delegate,
+              STORAGE_ON_DISK);
   EXPECT_FALSE(VerifyEntry(rollback_manager->GetUserShare(), PREFERENCES,
                            "pref2"));
 }
 
+TEST_F(SyncRollbackManagerTest, OnInitializationFailure) {
+  // Test graceful shutdown on initialization failure.
+  scoped_ptr<SyncRollbackManager> rollback_manager(
+      new SyncRollbackManager);
+  InitManager(rollback_manager.get(), ModelTypeSet(PREFERENCES), NULL,
+              STORAGE_ON_DISK);
+}
+
 }  // anonymous namespace
 
 }  // namespace syncer