ModelAssociationManager: Cleanup State::ASSOCIATING and associating_types_

Inlining MarkDataTypeAssociationDone into its single call site makes it
obvious that State::ASSOCIATING and associating_types_ only exist / are
non-empty within StartAssociationAsync().

Bug: 1102837
Change-Id: I7b95ad3100001694b7a762d0ccae4b2c52cb16a5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2479423
Commit-Queue: Jesse Doherty <jwd@chromium.org>
Reviewed-by: Jesse Doherty <jwd@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Auto-Submit: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818977}

components/sync/driver/data_type_manager_impl.cc

@@ -440,10 +440,8 @@ void DataTypeManagerImpl::ProcessReconfigure() {
     return;
   }
 
-  // Wait for current download and association to finish.
-  if (!download_types_queue_.empty() ||
-      model_association_manager_.state() ==
-          ModelAssociationManager::ASSOCIATING) {
+  // Wait for current download to finish.
+  if (!download_types_queue_.empty()) {
     return;
   }
 
@@ -504,9 +502,7 @@ void DataTypeManagerImpl::DownloadCompleted(
     association_types_queue_.back().first_sync_types = first_sync_types;
     association_types_queue_.back().download_ready_time = base::Time::Now();
     StartNextAssociation(UNREADY_AT_CONFIG);
-  } else if (download_types_queue_.empty() &&
-             model_association_manager_.state() !=
-                 ModelAssociationManager::ASSOCIATING) {
+  } else if (download_types_queue_.empty()) {
     // There's nothing more to download or associate (implying either there were
     // no types to associate or they associated as part of |ready_types|).
     // If the model association manager is also finished, then we're done
@@ -666,14 +662,6 @@ ModelTypeSet DataTypeManagerImpl::PrepareConfigureParams(
 void DataTypeManagerImpl::StartNextAssociation(AssociationGroup group) {
   DCHECK(!association_types_queue_.empty());
 
-  // If the model association manager is already associating, let it finish.
-  // The model association done event will result in associating any remaining
-  // association groups.
-  if (model_association_manager_.state() !=
-      ModelAssociationManager::INITIALIZED) {
-    return;
-  }
-
   ModelTypeSet types_to_associate;
   if (group == READY_AT_CONFIG) {
     association_types_queue_.front().ready_association_request_time =
@@ -694,7 +682,7 @@ void DataTypeManagerImpl::StartNextAssociation(AssociationGroup group) {
   }
 
   DVLOG(1) << "Associating " << ModelTypeSetToString(types_to_associate);
-  model_association_manager_.StartAssociationAsync(types_to_associate);
+  model_association_manager_.Associate(types_to_associate);
 }
 
 void DataTypeManagerImpl::OnSingleDataTypeWillStop(ModelType type,

components/sync/driver/model_association_manager.cc

@@ -64,11 +64,6 @@ ModelAssociationManager::~ModelAssociationManager() = default;
 void ModelAssociationManager::Initialize(ModelTypeSet desired_types,
                                          ModelTypeSet preferred_types,
                                          const ConfigureContext& context) {
-  // state_ can be INITIALIZED if types are reconfigured when
-  // data is being downloaded, so StartAssociationAsync() is never called for
-  // the first configuration.
-  DCHECK_NE(ASSOCIATING, state_);
-
   // |desired_types| must be a subset of |preferred_types|.
   DCHECK(preferred_types.HasAll(desired_types));
 
@@ -163,7 +158,6 @@ void ModelAssociationManager::StopDatatypeImpl(
     DataTypeController::StopCallback callback) {
   loaded_types_.Remove(dtc->type());
   associated_types_.Remove(dtc->type());
-  associating_types_.Remove(dtc->type());
 
   DCHECK(error.IsSet() || (dtc->state() != DataTypeController::NOT_RUNNING));
 
@@ -197,39 +191,37 @@ void ModelAssociationManager::LoadDesiredTypes() {
   NotifyDelegateIfReadyForConfigure();
 }
 
-void ModelAssociationManager::StartAssociationAsync(
+void ModelAssociationManager::Associate(
     const ModelTypeSet& types_to_associate) {
   DCHECK_EQ(INITIALIZED, state_);
   DCHECK(notified_about_ready_for_configure_);
 
   DVLOG(1) << "Starting association for "
            << ModelTypeSetToString(types_to_associate);
-  state_ = ASSOCIATING;
 
   requested_types_ = types_to_associate;
 
-  associating_types_ = types_to_associate;
-  associating_types_.RetainAll(desired_types_);
-  associating_types_.RemoveAll(associated_types_);
+  ModelTypeSet associating_types = types_to_associate;
+  associating_types.RetainAll(desired_types_);
+  associating_types.RemoveAll(associated_types_);
 
-  DCHECK(loaded_types_.HasAll(associating_types_));
+  DCHECK(loaded_types_.HasAll(associating_types));
 
   // Assume success.
   configure_status_ = DataTypeManager::OK;
 
-  // Done if no types to associate.
-  if (associating_types_.Empty()) {
-    ModelAssociationDone(INITIALIZED);
-    return;
-  }
-
-  // Associate types that are already loaded in specified order.
+  // Associate types in specified order.
   for (ModelType type : kStartOrder) {
-    if (associating_types_.Has(type) && loaded_types_.Has(type))
-      MarkDataTypeAssociationDone(type);
+    if (associating_types.Has(type)) {
+      associated_types_.Put(type);
+      associating_types.Remove(type);
+
+      if (ProtocolTypes().Has(type))
+        delegate_->OnSingleDataTypeAssociationDone(type);
+    }
   }
-  DCHECK(associating_types_.Empty());
-  DCHECK_NE(ASSOCIATING, state_);
+  DCHECK(associating_types.Empty());
+  ModelAssociationDone(INITIALIZED);
 }
 
 void ModelAssociationManager::Stop(ShutdownReason shutdown_reason) {
@@ -252,15 +244,8 @@ void ModelAssociationManager::Stop(ShutdownReason shutdown_reason) {
   loaded_types_.Clear();
   associated_types_.Clear();
 
-  if (state_ == ASSOCIATING) {
-    if (configure_status_ == DataTypeManager::OK)
-      configure_status_ = DataTypeManager::ABORTED;
-    ModelAssociationDone(IDLE);
-  } else {
-    DCHECK(associating_types_.Empty());
-    DCHECK(requested_types_.Empty());
-    state_ = IDLE;
-  }
+  DCHECK(requested_types_.Empty());
+  state_ = IDLE;
 }
 
 void ModelAssociationManager::ModelLoadCallback(ModelType type,
@@ -268,8 +253,6 @@ void ModelAssociationManager::ModelLoadCallback(ModelType type,
   DVLOG(1) << "ModelAssociationManager: ModelLoadCallback for "
            << ModelTypeToString(type);
 
-  DCHECK(associating_types_.Empty());
-
   if (error.IsSet()) {
     DVLOG(1) << "ModelAssociationManager: Type encountered an error.";
     desired_types_.Remove(type);
@@ -289,50 +272,14 @@ void ModelAssociationManager::ModelLoadCallback(ModelType type,
   NotifyDelegateIfReadyForConfigure();
 }
 
-void ModelAssociationManager::MarkDataTypeAssociationDone(ModelType type) {
-  DCHECK_EQ(state_, ASSOCIATING);
-  DCHECK(desired_types_.Has(type));
-  DCHECK(associating_types_.Has(type));
-  DCHECK(loaded_types_.Has(type));
-  DCHECK(!associated_types_.Has(type));
-
-  associated_types_.Put(type);
-  associating_types_.Remove(type);
-
-  if (ProtocolTypes().Has(type))
-    delegate_->OnSingleDataTypeAssociationDone(type);
-
-  if (associating_types_.Empty())
-    ModelAssociationDone(INITIALIZED);
-}
-
 void ModelAssociationManager::ModelAssociationDone(State new_state) {
-  DCHECK_EQ(ASSOCIATING, state_);
-  DCHECK_NE(ASSOCIATING, new_state);
-
   DVLOG(1) << "Model association complete for "
            << ModelTypeSetToString(requested_types_);
 
-  // Treat any unfinished types as having errors.
-  desired_types_.RemoveAll(associating_types_);
-  for (const auto& type_and_dtc : *controllers_) {
-    DataTypeController* dtc = type_and_dtc.second.get();
-    if (associating_types_.Has(dtc->type()) &&
-        dtc->state() != DataTypeController::NOT_RUNNING &&
-        dtc->state() != DataTypeController::STOPPING) {
-      base::UmaHistogramEnumeration("Sync.ConfigureFailed",
-                                    ModelTypeHistogramValue(dtc->type()));
-      StopDatatypeImpl(SyncError(FROM_HERE, SyncError::DATATYPE_ERROR,
-                                 "Association timed out.", dtc->type()),
-                       STOP_SYNC, dtc, base::DoNothing());
-    }
-  }
-
   DataTypeManager::ConfigureResult result(configure_status_, requested_types_);
 
   // Need to reset state before invoking delegate in order to avoid re-entrancy
   // issues (delegate may trigger a reconfiguration).
-  associating_types_.Clear();
   requested_types_.Clear();
   state_ = new_state;
 

components/sync/driver/model_association_manager.h

@@ -54,17 +54,6 @@ class ModelAssociationManagerDelegate {
 // should disappear or be refactored.
 class ModelAssociationManager {
  public:
-  enum State {
-    // No configuration is in progress.
-    IDLE,
-    // The model association manager has been initialized with a set of desired
-    // types, but is not actively associating any.
-    INITIALIZED,
-    // One or more types from |desired_types_| are in the process of
-    // associating.
-    ASSOCIATING,
-  };
-
   ModelAssociationManager(const DataTypeController::TypeMap* controllers,
                           ModelAssociationManagerDelegate* delegate);
   virtual ~ModelAssociationManager();
@@ -73,8 +62,8 @@ class ModelAssociationManager {
   // loading of all |desired_types|. A subsequent Initialize() call is only
   // allowed after the ModelAssociationManager has invoked
   // OnModelAssociationDone() on the delegate. After this call, there should be
-  // several calls to StartAssociationAsync() to associate subsets of
-  // |desired_types|, which itself must be a subset of |preferred_types|.
+  // several calls to Associate() to associate subsets of |desired_types|, which
+  // itself must be a subset of |preferred_types|.
   // |preferred_types| contains all types selected by the user.
   void Initialize(ModelTypeSet desired_types,
                   ModelTypeSet preferred_types,
@@ -86,8 +75,8 @@ class ModelAssociationManager {
   // Must only be called after all data type models have been loaded, i.e. after
   // OnAllDataTypesReadyForConfigure() has been called on the delegate.
   // |types_to_associate| should be subset of |desired_types| in Initialize().
-  // When this is completed, |OnModelAssociationDone| will be invoked.
-  void StartAssociationAsync(const ModelTypeSet& types_to_associate);
+  // Synchronously invokes |OnModelAssociationDone| on the delegate.
+  void Associate(const ModelTypeSet& types_to_associate);
 
   // Stops an individual datatype |type| for |shutdown_reason|. |error| must be
   // an actual error (i.e. not UNSET).
@@ -95,9 +84,15 @@ class ModelAssociationManager {
                     ShutdownReason shutdown_reason,
                     SyncError error);
 
-  State state() const { return state_; }
-
  private:
+  enum State {
+    // No configuration is in progress.
+    IDLE,
+    // The model association manager has been initialized with a set of desired
+    // types.
+    INITIALIZED,
+  };
+
   // Start loading non-running types that are in |desired_types_|.
   void LoadDesiredTypes();
 
@@ -105,8 +100,6 @@ class ModelAssociationManager {
   // This callback is passed to |LoadModels| function.
   void ModelLoadCallback(ModelType type, const SyncError& error);
 
-  void MarkDataTypeAssociationDone(ModelType type);
-
   // Called when all requested types are associated or association times out.
   // Will clean up any unfinished types, and update |state_| to be |new_state|
   // Finally, it will notify |delegate_| of the configuration result.
@@ -140,10 +133,6 @@ class ModelAssociationManager {
   // ASSOCIATING.
   ModelTypeSet requested_types_;
 
-  // Data types currently being associated, including types still waiting for
-  // model load. Non-empty iff |state_| is ASSOCIATING.
-  ModelTypeSet associating_types_;
-
   // Data types that are loaded, i.e. ready to associate.
   ModelTypeSet loaded_types_;
 

components/sync/driver/model_association_manager_unittest.cc

@@ -107,7 +107,7 @@ TEST_F(SyncModelAssociationManagerTest, SimpleModelStart) {
             DataTypeController::MODEL_LOADED);
   EXPECT_EQ(GetController(APPS)->state(), DataTypeController::MODEL_LOADED);
 
-  model_association_manager.StartAssociationAsync(types);
+  model_association_manager.Associate(types);
 }
 
 // Start a type, let it finish and then call stop.
@@ -123,7 +123,7 @@ TEST_F(SyncModelAssociationManagerTest, StopAfterFinish) {
   model_association_manager.Initialize(/*desired_types=*/types,
                                        /*preferred_types=*/types,
                                        BuildConfigureContext());
-  model_association_manager.StartAssociationAsync(types);
+  model_association_manager.Associate(types);
 
   ASSERT_EQ(GetController(BOOKMARKS)->state(),
             DataTypeController::MODEL_LOADED);
@@ -151,7 +151,7 @@ TEST_F(SyncModelAssociationManagerTest, ModelLoadFailBeforeAssociationStart) {
                                        BuildConfigureContext());
 
   EXPECT_EQ(DataTypeController::FAILED, GetController(BOOKMARKS)->state());
-  model_association_manager.StartAssociationAsync(types);
+  model_association_manager.Associate(types);
   EXPECT_EQ(DataTypeController::FAILED, GetController(BOOKMARKS)->state());
 }
 
@@ -167,7 +167,7 @@ TEST_F(SyncModelAssociationManagerTest, StopAfterConfiguration) {
   model_association_manager.Initialize(/*desired_types=*/types,
                                        /*preferred_types=*/types,
                                        BuildConfigureContext());
-  model_association_manager.StartAssociationAsync(types);
+  model_association_manager.Associate(types);
 
   ASSERT_EQ(GetController(BOOKMARKS)->state(),
             DataTypeController::MODEL_LOADED);
@@ -389,7 +389,7 @@ TEST_F(SyncModelAssociationManagerTest, KeepsMetadataForPreferredDataType) {
 
   model_association_manager.Initialize(desired_types, preferred_types,
                                        BuildConfigureContext());
-  model_association_manager.StartAssociationAsync(desired_types);
+  model_association_manager.Associate(desired_types);
 
   ASSERT_EQ(GetController(BOOKMARKS)->state(),
             DataTypeController::MODEL_LOADED);
@@ -429,7 +429,7 @@ TEST_F(SyncModelAssociationManagerTest, ClearsMetadataForNotPreferredDataType) {
 
   model_association_manager.Initialize(desired_types, preferred_types,
                                        BuildConfigureContext());
-  model_association_manager.StartAssociationAsync(desired_types);
+  model_association_manager.Associate(desired_types);
 
   ASSERT_EQ(GetController(BOOKMARKS)->state(),
             DataTypeController::MODEL_LOADED);
@@ -476,7 +476,7 @@ TEST_F(SyncModelAssociationManagerTest,
 
   model_association_manager.Initialize(desired_types, preferred_types,
                                        configure_context);
-  model_association_manager.StartAssociationAsync(desired_types);
+  model_association_manager.Associate(desired_types);
 
   ASSERT_EQ(GetController(BOOKMARKS)->state(),
             DataTypeController::MODEL_LOADED);
@@ -498,7 +498,7 @@ TEST_F(SyncModelAssociationManagerTest,
 
   model_association_manager.Initialize(desired_types, preferred_types,
                                        configure_context);
-  model_association_manager.StartAssociationAsync(desired_types);
+  model_association_manager.Associate(desired_types);
 
   ASSERT_EQ(GetController(BOOKMARKS)->state(),
             DataTypeController::MODEL_LOADED);
@@ -532,7 +532,7 @@ TEST_F(SyncModelAssociationManagerTest,
 
   model_association_manager.Initialize(desired_types, preferred_types,
                                        configure_context);
-  model_association_manager.StartAssociationAsync(desired_types);
+  model_association_manager.Associate(desired_types);
 
   ASSERT_EQ(GetController(BOOKMARKS)->state(),
             DataTypeController::MODEL_LOADED);
@@ -554,7 +554,7 @@ TEST_F(SyncModelAssociationManagerTest,
 
   model_association_manager.Initialize(desired_types, preferred_types,
                                        configure_context);
-  model_association_manager.StartAssociationAsync(desired_types);
+  model_association_manager.Associate(desired_types);
 
   ASSERT_EQ(GetController(BOOKMARKS)->state(),
             DataTypeController::MODEL_LOADED);

tools/metrics/histograms/histograms_xml/sync/histograms.xml

@@ -167,7 +167,10 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
 </histogram>
 
 <histogram name="Sync.ConfigureFailed" enum="SyncModelTypes"
-    expires_after="2021-03-27">
+    expires_after="2020-10-16">
+  <obsolete>
+    Removed 10/2020 (and not recorded for some time before that).
+  </obsolete>
   <owner>mastiz@chromium.org</owner>
   <owner>treib@chromium.org</owner>
   <summary>Count of model association failures for each type.</summary>