[Sync] Fix error handling during startup scenarios

Two cases are now improved:
1. Errors during model loading. Previously this would trigger a reconfiguration
despite being in the middle of an existing configuration.

2. Errors after model loading but before association. Previously these would do
nothing.

BUG=415757

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

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

8 files changed, 92 insertions, 69 deletions

diff --git a/components/sync_driver/data_type_manager_impl.cc b/components/sync_driver/data_type_manager_impl.cc
index 092f362..52b535f 100644
--- a/components/sync_driver/data_type_manager_impl.cc
+++ b/components/sync_driver/data_type_manager_impl.cc
@@ -128,7 +128,6 @@ void DataTypeManagerImpl::ReenableType(syncer::ModelType type) {
 }
 
 void DataTypeManagerImpl::ResetDataTypeErrors() {
-  DCHECK_EQ(state_, CONFIGURED);
   data_type_status_table_.Reset();
 }
 
@@ -265,14 +264,14 @@ void DataTypeManagerImpl::Restart(syncer::ConfigureReason reason) {
   if (state_ == STOPPED || state_ == CONFIGURED)
     NotifyStart();
 
-  model_association_manager_.Initialize(enabled_types);
-
+  state_ = DOWNLOAD_PENDING;
   download_types_queue_ = PrioritizeTypes(enabled_types);
   association_types_queue_ = std::queue<AssociationTypesInfo>();
 
+  model_association_manager_.Initialize(enabled_types);
+
   // Tell the backend about the new set of data types we wish to sync.
   // The task will be invoked when updates are downloaded.
-  state_ = DOWNLOAD_PENDING;
   configurer_->ConfigureDataTypes(
       reason,
       BuildDataTypeConfigStateMap(download_types_queue_.front()),
@@ -442,8 +441,6 @@ void DataTypeManagerImpl::OnSingleDataTypeWillStop(
       needs_reconfigure_ = true;
       last_configure_reason_ = syncer::CONFIGURE_REASON_PROGRAMMATIC;
       ProcessReconfigure();
-    } else {
-      DCHECK_EQ(state_, CONFIGURING);
     }
   }
 }
diff --git a/components/sync_driver/data_type_manager_impl_unittest.cc b/components/sync_driver/data_type_manager_impl_unittest.cc
index c4d2ec2..c2e32f7 100644
--- a/components/sync_driver/data_type_manager_impl_unittest.cc
+++ b/components/sync_driver/data_type_manager_impl_unittest.cc
@@ -44,24 +44,30 @@ DataTypeStatusTable BuildStatusTable(ModelTypeSet crypto_errors,
   DataTypeStatusTable::TypeErrorMap error_map;
   for (ModelTypeSet::Iterator iter = crypto_errors.First(); iter.Good();
        iter.Inc()) {
-    error_map[iter.Get()] = SyncError(
-        FROM_HERE, SyncError::CRYPTO_ERROR, "crypto error", iter.Get());
+    error_map[iter.Get()] = SyncError(FROM_HERE,
+                                      SyncError::CRYPTO_ERROR,
+                                      "crypto error expected",
+                                      iter.Get());
   }
   for (ModelTypeSet::Iterator iter = association_errors.First(); iter.Good();
        iter.Inc()) {
-    error_map[iter.Get()] = SyncError(
-        FROM_HERE, SyncError::DATATYPE_ERROR, "association error", iter.Get());
+    error_map[iter.Get()] = SyncError(FROM_HERE,
+                                      SyncError::DATATYPE_ERROR,
+                                      "association error expected",
+                                      iter.Get());
   }
   for (ModelTypeSet::Iterator iter = unready_errors.First(); iter.Good();
        iter.Inc()) {
-    error_map[iter.Get()] = SyncError(
-        FROM_HERE, SyncError::UNREADY_ERROR, "unready errors", iter.Get());
+    error_map[iter.Get()] = SyncError(FROM_HERE,
+                                      SyncError::UNREADY_ERROR,
+                                      "unready error expected",
+                                      iter.Get());
   }
   for (ModelTypeSet::Iterator iter = unrecoverable_errors.First(); iter.Good();
        iter.Inc()) {
     error_map[iter.Get()] = SyncError(FROM_HERE,
                                       SyncError::UNRECOVERABLE_ERROR,
-                                      "unrecoverable errors",
+                                      "unrecoverable error expected",
                                       iter.Get());
   }
   DataTypeStatusTable status_table;
@@ -1334,4 +1340,52 @@ TEST_F(SyncDataTypeManagerImplTest, UnreadyType) {
   EXPECT_TRUE(configurer_.activated_types().Empty());
 }
 
+TEST_F(SyncDataTypeManagerImplTest, ModelLoadError) {
+  AddController(BOOKMARKS);
+  GetController(BOOKMARKS)->SetModelLoadError(syncer::SyncError(
+        FROM_HERE, SyncError::DATATYPE_ERROR, "load error", BOOKMARKS));
+
+  // Bookmarks is never started due to hitting a model load error.
+  SetConfigureStartExpectation();
+  SetConfigureDoneExpectation(DataTypeManager::OK,
+                              BuildStatusTable(ModelTypeSet(),
+                                               ModelTypeSet(BOOKMARKS),
+                                               ModelTypeSet(),
+                                               ModelTypeSet()));
+  Configure(dtm_.get(), ModelTypeSet(BOOKMARKS));
+  FinishDownload(*dtm_, ModelTypeSet(), ModelTypeSet());
+  FinishDownload(*dtm_, ModelTypeSet(BOOKMARKS), ModelTypeSet());
+  EXPECT_EQ(DataTypeManager::CONFIGURED, dtm_->state());
+  EXPECT_EQ(DataTypeController::NOT_RUNNING, GetController(BOOKMARKS)->state());
+
+  EXPECT_EQ(0U, configurer_.activated_types().Size());
+}
+
+
+TEST_F(SyncDataTypeManagerImplTest, ErrorBeforeAssociation) {
+  AddController(BOOKMARKS);
+
+  // Bookmarks is never started due to hitting a datatype error while the DTM
+  // is still downloading types.
+  SetConfigureStartExpectation();
+  SetConfigureDoneExpectation(DataTypeManager::OK,
+                              BuildStatusTable(ModelTypeSet(),
+                                               ModelTypeSet(BOOKMARKS),
+                                               ModelTypeSet(),
+                                               ModelTypeSet()));
+  Configure(dtm_.get(), ModelTypeSet(BOOKMARKS));
+  FinishDownload(*dtm_, ModelTypeSet(), ModelTypeSet());
+  GetController(BOOKMARKS)->OnSingleDataTypeUnrecoverableError(
+      syncer::SyncError(FROM_HERE,
+                        SyncError::DATATYPE_ERROR,
+                        "bookmarks error",
+                        BOOKMARKS));
+  FinishDownload(*dtm_, ModelTypeSet(BOOKMARKS), ModelTypeSet());
+  FinishDownload(*dtm_, ModelTypeSet(), ModelTypeSet());  // Reconfig for error.
+  EXPECT_EQ(DataTypeManager::CONFIGURED, dtm_->state());
+  EXPECT_EQ(DataTypeController::NOT_RUNNING, GetController(BOOKMARKS)->state());
+
+  EXPECT_EQ(0U, configurer_.activated_types().Size());
+}
+
 }  // namespace sync_driver
diff --git a/components/sync_driver/fake_data_type_controller.cc b/components/sync_driver/fake_data_type_controller.cc
index 3fb646c..74af3b3 100644
--- a/components/sync_driver/fake_data_type_controller.cc
+++ b/components/sync_driver/fake_data_type_controller.cc
@@ -24,6 +24,7 @@ FakeDataTypeController::~FakeDataTypeController() {
 // NOT_RUNNING ->MODEL_LOADED |MODEL_STARTING.
 void FakeDataTypeController::LoadModels(
     const ModelLoadCallback& model_load_callback) {
+  model_load_callback_ = model_load_callback;
   if (state_ != NOT_RUNNING) {
     ADD_FAILURE();
     return;
@@ -36,7 +37,6 @@ void FakeDataTypeController::LoadModels(
       state_ = MODEL_LOADED;
     model_load_callback.Run(type(), load_error_);
   } else {
-    model_load_callback_ = model_load_callback;
     state_ = MODEL_STARTING;
   }
 }
@@ -126,10 +126,8 @@ DataTypeController::State FakeDataTypeController::state() const {
 
 void FakeDataTypeController::OnSingleDataTypeUnrecoverableError(
     const syncer::SyncError& error) {
-  syncer::SyncMergeResult local_merge_result(type());
-  local_merge_result.set_error(error);
-  last_start_callback_.Run(
-      RUNTIME_ERROR, local_merge_result, syncer::SyncMergeResult(type_));
+  if (!model_load_callback_.is_null())
+    model_load_callback_.Run(type(), error);
 }
 
 bool FakeDataTypeController::ReadyForStart() const {
@@ -145,9 +143,7 @@ void FakeDataTypeController::SetModelLoadError(syncer::SyncError error) {
 }
 
 void FakeDataTypeController::SimulateModelLoadFinishing() {
-  ModelLoadCallback model_load_callback = model_load_callback_;
-  model_load_callback.Run(type(), load_error_);
-  model_load_callback_.Reset();
+  model_load_callback_.Run(type(), load_error_);
 }
 
 void FakeDataTypeController::SetReadyForStart(bool ready) {
diff --git a/components/sync_driver/model_association_manager.cc b/components/sync_driver/model_association_manager.cc
index 658b1bf..6706c4c 100644
--- a/components/sync_driver/model_association_manager.cc
+++ b/components/sync_driver/model_association_manager.cc
@@ -288,11 +288,6 @@ void ModelAssociationManager::ModelLoadCallback(syncer::ModelType type,
   DVLOG(1) << "ModelAssociationManager: ModelLoadCallback for "
       << syncer::ModelTypeToString(type);
 
-  // This happens when slow loading type is disabled by new configuration.
-  if (!desired_types_.Has(type))
-    return;
-
-  DCHECK(!loaded_types_.Has(type));
   if (error.IsSet()) {
     syncer::SyncMergeResult local_merge_result(type);
     local_merge_result.set_error(error);
@@ -304,6 +299,11 @@ void ModelAssociationManager::ModelLoadCallback(syncer::ModelType type,
     return;
   }
 
+  // This happens when slow loading type is disabled by new configuration.
+  if (!desired_types_.Has(type))
+    return;
+
+  DCHECK(!loaded_types_.Has(type));
   loaded_types_.Put(type);
   if (associating_types_.Has(type)) {
     DataTypeController* dtc = controllers_->find(type)->second.get();
diff --git a/components/sync_driver/non_ui_data_type_controller.cc b/components/sync_driver/non_ui_data_type_controller.cc
index 02845b0..ba2f8f9 100644
--- a/components/sync_driver/non_ui_data_type_controller.cc
+++ b/components/sync_driver/non_ui_data_type_controller.cc
@@ -34,7 +34,7 @@ NonUIDataTypeController::NonUIDataTypeController(
 void NonUIDataTypeController::LoadModels(
     const ModelLoadCallback& model_load_callback) {
   DCHECK(ui_thread_->BelongsToCurrentThread());
-  DCHECK(!model_load_callback.is_null());
+  model_load_callback_ = model_load_callback;
   if (state() != NOT_RUNNING) {
     model_load_callback.Run(type(),
                             syncer::SyncError(FROM_HERE,
@@ -50,7 +50,6 @@ void NonUIDataTypeController::LoadModels(
   DCHECK(!shared_change_processor_.get());
   shared_change_processor_ = CreateSharedChangeProcessor();
   DCHECK(shared_change_processor_.get());
-  model_load_callback_ = model_load_callback;
   if (!StartModels()) {
     // If we are waiting for some external service to load before associating
     // or we failed to start the models, we exit early.
@@ -63,12 +62,8 @@ void NonUIDataTypeController::LoadModels(
 
 void NonUIDataTypeController::OnModelLoaded() {
   DCHECK_EQ(state_, MODEL_STARTING);
-  DCHECK(!model_load_callback_.is_null());
   state_ = MODEL_LOADED;
-
-  ModelLoadCallback model_load_callback = model_load_callback_;
-  model_load_callback_.Reset();
-  model_load_callback.Run(type(), syncer::SyncError());
+  model_load_callback_.Run(type(), syncer::SyncError());
 }
 
 bool NonUIDataTypeController::StartModels() {
@@ -256,13 +251,10 @@ void NonUIDataTypeController::RecordStartFailure(ConfigureResult result) {
 void NonUIDataTypeController::AbortModelLoad() {
   state_ = NOT_RUNNING;
   StopModels();
-  ModelLoadCallback model_load_callback = model_load_callback_;
-  model_load_callback_.Reset();
-  model_load_callback.Run(type(),
-                          syncer::SyncError(FROM_HERE,
-                                            syncer::SyncError::DATATYPE_ERROR,
-                                            "ABORTED",
-                                            type()));
+  model_load_callback_.Run(
+      type(),
+      syncer::SyncError(
+          FROM_HERE, syncer::SyncError::DATATYPE_ERROR, "ABORTED", type()));
 }
 
 void NonUIDataTypeController::DisableImpl(
@@ -271,12 +263,10 @@ void NonUIDataTypeController::DisableImpl(
   UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeRunFailures",
                             ModelTypeToHistogramInt(type()),
                             syncer::MODEL_TYPE_COUNT);
-  if (!start_callback_.is_null()) {
+  if (!model_load_callback_.is_null()) {
     syncer::SyncMergeResult local_merge_result(type());
     local_merge_result.set_error(error);
-    start_callback_.Run(RUNTIME_ERROR,
-                        local_merge_result,
-                        syncer::SyncMergeResult(type()));
+    model_load_callback_.Run(type(), error);
   }
 }
 
diff --git a/components/sync_driver/non_ui_data_type_controller_unittest.cc b/components/sync_driver/non_ui_data_type_controller_unittest.cc
index 3f9e87b..5eef2e3 100644
--- a/components/sync_driver/non_ui_data_type_controller_unittest.cc
+++ b/components/sync_driver/non_ui_data_type_controller_unittest.cc
@@ -486,7 +486,7 @@ TEST_F(SyncNonUIDataTypeControllerTest, OnSingleDataTypeUnrecoverableError) {
   EXPECT_EQ(DataTypeController::RUNNING, non_ui_dtc_->state());
 
   testing::Mock::VerifyAndClearExpectations(&start_callback_);
-  EXPECT_CALL(start_callback_, Run(DataTypeController::RUNTIME_ERROR, _, _));
+  EXPECT_CALL(model_load_callback_, Run(_, _));
   syncer::SyncError error(FROM_HERE,
                           syncer::SyncError::DATATYPE_ERROR,
                           "error",
diff --git a/components/sync_driver/ui_data_type_controller.cc b/components/sync_driver/ui_data_type_controller.cc
index 23487a2..aa46d8a 100644
--- a/components/sync_driver/ui_data_type_controller.cc
+++ b/components/sync_driver/ui_data_type_controller.cc
@@ -52,8 +52,8 @@ UIDataTypeController::~UIDataTypeController() {
 void UIDataTypeController::LoadModels(
     const ModelLoadCallback& model_load_callback) {
   DCHECK(ui_thread_->BelongsToCurrentThread());
-  DCHECK(!model_load_callback.is_null());
   DCHECK(syncer::IsRealDataType(type_));
+  model_load_callback_ = model_load_callback;
   if (state_ != NOT_RUNNING) {
     model_load_callback.Run(type(),
                             syncer::SyncError(FROM_HERE,
@@ -67,7 +67,6 @@ void UIDataTypeController::LoadModels(
   DCHECK(!shared_change_processor_.get());
   shared_change_processor_ = new SharedChangeProcessor();
 
-  model_load_callback_ = model_load_callback;
   state_ = MODEL_STARTING;
   if (!StartModels()) {
     // If we are waiting for some external service to load before associating
@@ -78,19 +77,15 @@ void UIDataTypeController::LoadModels(
   }
 
   state_ = MODEL_LOADED;
-  model_load_callback_.Reset();
-  model_load_callback.Run(type(), syncer::SyncError());
+  model_load_callback_.Run(type(), syncer::SyncError());
 }
 
 void UIDataTypeController::OnModelLoaded() {
   DCHECK(ui_thread_->BelongsToCurrentThread());
-  DCHECK(!model_load_callback_.is_null());
   DCHECK_EQ(state_, MODEL_STARTING);
 
   state_ = MODEL_LOADED;
-  ModelLoadCallback model_load_callback = model_load_callback_;
-  model_load_callback_.Reset();
-  model_load_callback.Run(type(), syncer::SyncError());
+  model_load_callback_.Run(type(), syncer::SyncError());
 }
 
 void UIDataTypeController::StartAssociating(
@@ -227,13 +222,10 @@ void UIDataTypeController::AbortModelLoad() {
     shared_change_processor_ = NULL;
   }
 
-  ModelLoadCallback model_load_callback = model_load_callback_;
-  model_load_callback_.Reset();
-  model_load_callback.Run(type(),
-                          syncer::SyncError(FROM_HERE,
-                                            syncer::SyncError::DATATYPE_ERROR,
-                                            "Aborted",
-                                            type()));
+  model_load_callback_.Run(
+      type(),
+      syncer::SyncError(
+          FROM_HERE, syncer::SyncError::DATATYPE_ERROR, "Aborted", type()));
   // We don't want to continue loading models (e.g OnModelLoaded should never be
   // called after we've decided to abort).
   StopModels();
@@ -328,15 +320,9 @@ void UIDataTypeController::OnSingleDataTypeUnrecoverableError(
   // TODO(tim): We double-upload some errors.  See bug 383480.
   if (!error_callback_.is_null())
     error_callback_.Run();
-  if (!start_callback_.is_null()) {
-    syncer::SyncMergeResult local_merge_result(type());
-    local_merge_result.set_error(error);
+  if (!model_load_callback_.is_null()) {
     base::MessageLoop::current()->PostTask(
-        FROM_HERE,
-        base::Bind(start_callback_,
-                   RUNTIME_ERROR,
-                   local_merge_result,
-                   syncer::SyncMergeResult(type())));
+        FROM_HERE, base::Bind(model_load_callback_, type(), error));
   }
 }
 
diff --git a/components/sync_driver/ui_data_type_controller_unittest.cc b/components/sync_driver/ui_data_type_controller_unittest.cc
index da61cb7..abf2884 100644
--- a/components/sync_driver/ui_data_type_controller_unittest.cc
+++ b/components/sync_driver/ui_data_type_controller_unittest.cc
@@ -188,7 +188,7 @@ TEST_F(SyncUIDataTypeControllerTest, OnSingleDatatypeUnrecoverableError) {
   EXPECT_TRUE(syncable_service_.syncing());
 
   testing::Mock::VerifyAndClearExpectations(&start_callback_);
-  EXPECT_CALL(start_callback_, Run(DataTypeController::RUNTIME_ERROR, _, _));
+  EXPECT_CALL(model_load_callback_, Run(_, _));
   syncer::SyncError error(FROM_HERE,
                           syncer::SyncError::DATATYPE_ERROR,
                           "error",