Sync DataTypeManagerImpl: Mark failed data types on Sync startup

This fixes a bug where Sync configuration would never finish:
DataTypeManager is supposed to filter out failed data types before
passing them to ModelAssociationManager. Before this CL, it failed to
do that if a data type (in particular, the DataTypeController) was
already in a FAILED state at Sync startup. In that case,
ModelAssociationManager would forever wait for that type to start.
This could happen if, after a data type error was encountered, Sync got
stopped and restarted.

Bug: 967344
Change-Id: Ic3429ce4a056b5a505cc9bb1a2cc0c87e4d2ec5c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1645231
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#666632}

components/sync/driver/data_type_manager_impl.cc

@@ -77,6 +77,24 @@ DataTypeManagerImpl::DataTypeManagerImpl(
       weak_ptr_factory_(this) {
   DCHECK(configurer_);
   DCHECK(observer_);
+
+  // Check if any of the controllers are already in a FAILED state, and if so,
+  // mark them accordingly in the status table.
+  DataTypeStatusTable::TypeErrorMap existing_errors;
+  for (const auto& kv : *controllers_) {
+    ModelType type = kv.first;
+    const DataTypeController* controller = kv.second.get();
+    DataTypeController::State state = controller->state();
+    DCHECK(state == DataTypeController::NOT_RUNNING ||
+           state == DataTypeController::STOPPING ||
+           state == DataTypeController::FAILED);
+    if (state == DataTypeController::FAILED) {
+      existing_errors[type] =
+          SyncError(FROM_HERE, SyncError::DATATYPE_ERROR,
+                    "Preexisting controller error on Sync startup", type);
+    }
+  }
+  data_type_status_table_.UpdateFailedDataTypes(existing_errors);
 }
 
 DataTypeManagerImpl::~DataTypeManagerImpl() {}

components/sync/driver/data_type_manager_impl_unittest.cc

@@ -251,7 +251,9 @@ class SyncDataTypeManagerImplTest : public testing::Test {
   ~SyncDataTypeManagerImplTest() override {}
 
  protected:
-  void SetUp() override {
+  void SetUp() override { RecreateDataTypeManager(); }
+
+  void RecreateDataTypeManager() {
     dtm_ = std::make_unique<TestDataTypeManager>(
         ModelTypeSet(), WeakHandle<DataTypeDebugInfoListener>(), &controllers_,
         &encryption_handler_, &configurer_, &observer_);
@@ -1555,6 +1557,45 @@ TEST_F(SyncDataTypeManagerImplTest, ErrorBeforeAssociation) {
   EXPECT_EQ(0U, configurer_.activated_types().Size());
 }
 
+// Checks that DTM handles the case when a controller is already in a FAILED
+// state at the time the DTM is created. Regression test for crbug.com/967344.
+TEST_F(SyncDataTypeManagerImplTest, ErrorBeforeStartup) {
+  AddController(BOOKMARKS);
+  AddController(PREFERENCES);
+
+  // Produce an error (FAILED) state in the BOOKMARKS controller.
+  GetController(BOOKMARKS)->SetModelLoadError(SyncError(
+      FROM_HERE, SyncError::DATATYPE_ERROR, "bookmarks error", BOOKMARKS));
+  SetConfigureStartExpectation();
+  Configure({BOOKMARKS});
+  GetController(BOOKMARKS)->SimulateModelLoadFinishing();
+  ASSERT_EQ(GetController(BOOKMARKS)->state(), DataTypeController::FAILED);
+
+  // Now create a fresh DTM, simulating a Sync restart.
+  RecreateDataTypeManager();
+
+  ASSERT_EQ(GetController(BOOKMARKS)->state(), DataTypeController::FAILED);
+
+  // Now a configuration attempt for both types should complete successfully,
+  // but exclude the failed type.
+  SetConfigureStartExpectation();
+  Configure({BOOKMARKS, PREFERENCES});
+
+  SetConfigureDoneExpectation(
+      DataTypeManager::OK, BuildStatusTable(/*crypto_errors=*/{},
+                                            /*association_errors=*/{BOOKMARKS},
+                                            /*unready_errore=*/{},
+                                            /*unrecoverable_errors=*/{}));
+  FinishDownload(/*types_to_configure=*/{},
+                 /*failed_download_types=*/{});
+  FinishDownload(/*types_to_configure=*/{PREFERENCES},
+                 /*failed_download_types=*/{});
+  GetController(PREFERENCES)->FinishStart(DataTypeController::OK);
+
+  EXPECT_TRUE(dtm_->GetActiveDataTypes().Has(PREFERENCES));
+  EXPECT_FALSE(dtm_->GetActiveDataTypes().Has(BOOKMARKS));
+}
+
 TEST_F(SyncDataTypeManagerImplTest, AssociationNeverCompletes) {
   AddController(BOOKMARKS);
 

components/sync/driver/model_association_manager.cc

@@ -123,8 +123,14 @@ void ModelAssociationManager::Initialize(ModelTypeSet desired_types,
   // Only keep types that have controllers.
   desired_types_.Clear();
   for (ModelType type : desired_types) {
-    if (controllers_->find(type) != controllers_->end())
+    auto dtc_iter = controllers_->find(type);
+    if (dtc_iter != controllers_->end()) {
+      DataTypeController* dtc = dtc_iter->second.get();
+      // Controllers in a FAILED state should have been filtered out by the
+      // DataTypeManager.
+      DCHECK_NE(dtc->state(), DataTypeController::FAILED);
       desired_types_.Put(type);
+    }
   }
 
   DVLOG(1) << "ModelAssociationManager: Initializing for "