[Sync] Cleanup datatype configuration error handling.

components/sync_driver/data_type_manager_impl.cc

Index: components/sync_driver/data_type_manager_impl.cc
diff --git a/components/sync_driver/data_type_manager_impl.cc b/components/sync_driver/data_type_manager_impl.cc
index 8f75985bca9b400a62ba6726fa94bbfc3770c364..dde56d3bfdcedb7eb34e6800589620c4c7f009cb 100644
--- a/components/sync_driver/data_type_manager_impl.cc
+++ b/components/sync_driver/data_type_manager_impl.cc
@@ -88,7 +88,9 @@ void DataTypeManagerImpl::Configure(syncer::ModelTypeSet desired_types,
     if (syncer::IsControlType(type.Get()) ||
         iter != controllers_->end()) {
       if (iter != controllers_->end()) {
-        if (!iter->second->ReadyForStart()) {
+        if (!iter->second->ReadyForStart() &&
+            !failed_data_types_handler_->GetUnreadyErrorTypes().Has(

[haitaol1, 2014/07/30 23:02:44]

Is the end result same as before, i.e. unready type is tagged in
failed_data_types_handler_? If so, current code seems more straightforward even
with a little redundancy.

[Nicolas Zea, 2014/07/30 23:20:36]

It is, but without this condition we'll be spamming the console with the errors.

+                type.Get())) {
           // Add the type to the unready types set to prevent purging it. It's
           // up to the datatype controller to, if necessary, explicitly
           // mark the type as broken to trigger a purge.
@@ -99,7 +101,7 @@ void DataTypeManagerImpl::Configure(syncer::ModelTypeSet desired_types,
           std::map<syncer::ModelType, syncer::SyncError> errors;
           errors[type.Get()] = error;
           failed_data_types_handler_->UpdateFailedDataTypes(errors);
-        } else {
+        } else if (iter->second->ReadyForStart()) {
           failed_data_types_handler_->ResetUnreadyErrorFor(type.Get());
         }
       }
@@ -345,14 +347,16 @@ void DataTypeManagerImpl::DownloadReady(
   if (!failed_configuration_types.Empty()) {
     if (!unrecoverable_error_method_.is_null())
       unrecoverable_error_method_.Run();
-    std::string error_msg =
-        "Configuration failed for types " +
-        syncer::ModelTypeSetToString(failed_configuration_types);
-    syncer::SyncError error(FROM_HERE,
-                            syncer::SyncError::UNRECOVERABLE_ERROR,
-                            error_msg,
-                            failed_configuration_types.First().Get());
-    Abort(UNRECOVERABLE_ERROR, error);
+    syncer::SyncError error(
+        FROM_HERE,
+        syncer::SyncError::UNRECOVERABLE_ERROR,
+        "Backend failed to configure types " +
+            syncer::ModelTypeSetToString(failed_configuration_types),
+        failed_configuration_types.First().Get());
+    FailedDataTypesHandler::TypeErrorMap errors;
+    errors[failed_configuration_types.First().Get()] = error;

[haitaol1, 2014/07/30 23:02:44]

Do you need to set error for all failed types?

[Nicolas Zea, 2014/07/30 23:20:36]

There's only support for tracking one unrecoverable error at a time right now.
It results in shutting down the syncer, so tracking more than that doesn't seem
necessary.

+    failed_data_types_handler_->UpdateFailedDataTypes(errors);
+    Abort(UNRECOVERABLE_ERROR);
     return;
   }
 
@@ -400,8 +404,25 @@ void DataTypeManagerImpl::StartNextAssociation() {
 }
 
 void DataTypeManagerImpl::OnSingleDataTypeWillStop(
-    syncer::ModelType type) {
+    syncer::ModelType type,
+    const syncer::SyncError& error) {
   configurer_->DeactivateDataType(type);
+  if (error.IsSet()) {
+    FailedDataTypesHandler::TypeErrorMap failed_types;
+    failed_types[type] = error;
+    failed_data_types_handler_->UpdateFailedDataTypes(
+            failed_types);
+
+    // Unrecoverable errors will shut down the entire backend, so no need to
+    // reconfigure.
+    if (error.error_type() != syncer::SyncError::UNRECOVERABLE_ERROR) {
+      needs_reconfigure_ = true;
+      if (state_ == CONFIGURED)

[haitaol1, 2014/07/30 23:02:44]

Doesn't hurt to always call ProcessReconfigure().

[Nicolas Zea, 2014/07/30 23:20:36]

Done.

+        ProcessReconfigure();
+    } else {
+      DCHECK_EQ(state_, CONFIGURING);
+    }
+  }
 }
 
 void DataTypeManagerImpl::OnSingleDataTypeAssociationDone(
@@ -449,25 +470,6 @@ void DataTypeManagerImpl::OnModelAssociationDone(
   if (state_ == STOPPING)
     return;
 
-  // Don't reconfigure due to failed data types if we have an unrecoverable
-  // error or have already aborted.
-  if (result.status  == PARTIAL_SUCCESS) {
-    if (!result.needs_crypto.Empty()) {
-      needs_reconfigure_ = true;
-      syncer::ModelTypeSet encrypted_types = result.needs_crypto;
-      encrypted_types.RemoveAll(
-          failed_data_types_handler_->GetCryptoErrorTypes());
-      FailedDataTypesHandler::TypeErrorMap crypto_errors =
-          GenerateCryptoErrorsForTypes(encrypted_types);
-      failed_data_types_handler_->UpdateFailedDataTypes(crypto_errors);
-    }
-    if (!result.failed_data_types.empty()) {
-      needs_reconfigure_ = true;
-      failed_data_types_handler_->UpdateFailedDataTypes(
-          result.failed_data_types);
-    }
-  }
-
   // Ignore abort/unrecoverable error if we need to reconfigure anyways.
   if (needs_reconfigure_) {
     association_types_queue_ = std::queue<AssociationTypesInfo>();
@@ -476,38 +478,18 @@ void DataTypeManagerImpl::OnModelAssociationDone(
   }
 
   if (result.status == ABORTED || result.status == UNRECOVERABLE_ERROR) {
-    Abort(result.status, result.failed_data_types.size() >= 1 ?
-                         result.failed_data_types.begin()->second :
-                         syncer::SyncError());
+    Abort(result.status);
     return;
   }
 
-  DCHECK(result.status == PARTIAL_SUCCESS || result.status == OK);
-  DCHECK(result.status != OK ||
-         (result.needs_crypto.Empty() && result.failed_data_types.empty()));
-
-  // It's possible this is a retry to disable failed types, in which case
-  // the association would be SUCCESS, but the overall configuration should
-  // still be PARTIAL_SUCCESS.
-  syncer::ModelTypeSet failed_data_types =
-      failed_data_types_handler_->GetFailedTypes();
-  ConfigureStatus status = result.status;
-  if (!syncer::Intersection(last_requested_types_,
-                            failed_data_types).Empty() && result.status == OK) {
-    status = PARTIAL_SUCCESS;
-  }
+  DCHECK(result.status == OK);
 
   association_types_queue_.pop();
   if (!association_types_queue_.empty()) {
     StartNextAssociation();
   } else if (download_types_queue_.empty()) {
     state_ = CONFIGURED;
-    ConfigureResult configure_result(status,
-                                     result.requested_types,
-                                     failed_data_types_handler_->GetAllErrors(),
-                                     result.unfinished_data_types,
-                                     result.needs_crypto);
-    NotifyDone(configure_result);
+    NotifyDone(result);
   }
 }
 
@@ -521,29 +503,19 @@ void DataTypeManagerImpl::Stop() {
 
   if (need_to_notify) {
     ConfigureResult result(ABORTED,
-                           last_requested_types_,
-                           std::map<syncer::ModelType, syncer::SyncError>(),
-                           syncer::ModelTypeSet(),
-                           syncer::ModelTypeSet());
+                           last_requested_types_);
     NotifyDone(result);
   }
 }
 
-void DataTypeManagerImpl::Abort(ConfigureStatus status,
-                                const syncer::SyncError& error) {
+void DataTypeManagerImpl::Abort(ConfigureStatus status) {
   DCHECK(state_ == DOWNLOAD_PENDING || state_ == CONFIGURING);
 
   StopImpl();
 
   DCHECK_NE(OK, status);
-  std::map<syncer::ModelType, syncer::SyncError> errors;
-  if (error.IsSet())
-    errors[error.model_type()] = error;
   ConfigureResult result(status,
-                         last_requested_types_,
-                         errors,
-                         syncer::ModelTypeSet(),
-                         syncer::ModelTypeSet());
+                         last_requested_types_);
   NotifyDone(result);
 }
 
@@ -593,12 +565,7 @@ void DataTypeManagerImpl::NotifyDone(const ConfigureResult& result) {
       UMA_HISTOGRAM_LONG_TIMES("Sync.ConfigureTime_Long.UNRECOVERABLE_ERROR",
                                configure_time_delta_);
       break;
-    case DataTypeManager::PARTIAL_SUCCESS:
-      DVLOG(1) << "NotifyDone called with result: PARTIAL_SUCCESS";
-      UMA_HISTOGRAM_LONG_TIMES("Sync.ConfigureTime_Long.PARTIAL_SUCCESS",
-                               configure_time_delta_);
-      break;
-    default:
+    case DataTypeManager::UNKNOWN:
       NOTREACHED();
       break;
   }