Revert 288464 "[Sync] Cleanup datatype configuration error handl..."

Reason for revert: see crbug.com/403098

> [Sync] Cleanup datatype configuration error handling.
> 
> The FailedDataTypeHandler is now informed immediately of failures, including
> datatype errors, and is therefore authoritative source for all errors. As such,
> partial success is no longer tracked and the various ModelTypeSets for error
> types in configure results are removed.
> 
> BUG=368834,403098
> 
> Review URL: https://codereview.chromium.org/420633002

TBR=zea@chromium.org

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

Cr-Commit-Position: refs/heads/master@{#289115}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@289115 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/sync/backend_migrator.cc

@@ -164,7 +164,8 @@ void BackendMigrator::OnConfigureDoneImpl(
     return;
   }
 
-  if (result.status != sync_driver::DataTypeManager::OK) {
+  if (result.status != sync_driver::DataTypeManager::OK &&
+      result.status != sync_driver::DataTypeManager::PARTIAL_SUCCESS) {
     // If this fails, and we're disabling types, a type may or may not be
     // disabled until the user restarts the browser.  If this wasn't an abort,
     // any failure will be reported as an unrecoverable error to the UI. If it

chrome/browser/sync/backend_migrator_unittest.cc

@@ -79,9 +79,13 @@ class SyncBackendMigratorTest : public testing::Test {
       DataTypeManager::ConfigureResult result(status, requested_types);
       migrator_->OnConfigureDone(result);
     } else {
+      std::map<syncer::ModelType, syncer::SyncError> errors;
       DataTypeManager::ConfigureResult result(
           status,
-          requested_types);
+          requested_types,
+          errors,
+          syncer::ModelTypeSet(),
+          syncer::ModelTypeSet());
       migrator_->OnConfigureDone(result);
     }
     message_loop_.RunUntilIdle();

chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc

@@ -158,7 +158,7 @@ class SyncAutofillDataTypeControllerTest : public testing::Test {
   }
 
   // Passed to AutofillDTC::Start().
-  void OnStartFinished(sync_driver::DataTypeController::ConfigureResult result,
+  void OnStartFinished(sync_driver::DataTypeController::StartResult result,
                        const syncer::SyncMergeResult& local_merge_result,
                        const syncer::SyncMergeResult& syncer_merge_result) {
     last_start_result_ = result;
@@ -189,7 +189,7 @@ class SyncAutofillDataTypeControllerTest : public testing::Test {
   scoped_refptr<AutofillDataTypeController> autofill_dtc_;
 
   // Stores arguments of most recent call of OnStartFinished().
-  sync_driver::DataTypeController::ConfigureResult last_start_result_;
+  sync_driver::DataTypeController::StartResult last_start_result_;
   syncer::SyncError last_start_error_;
   base::WeakPtrFactory<SyncAutofillDataTypeControllerTest> weak_ptr_factory_;
 };

chrome/browser/sync/glue/frontend_data_type_controller.cc

@@ -262,7 +262,7 @@ void FrontendDataTypeController::AbortModelLoad() {
 }
 
 void FrontendDataTypeController::StartDone(
-    ConfigureResult start_result,
+    StartResult start_result,
     const syncer::SyncMergeResult& local_merge_result,
     const syncer::SyncMergeResult& syncer_merge_result) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -295,7 +295,7 @@ void FrontendDataTypeController::RecordAssociationTime(base::TimeDelta time) {
 #undef PER_DATA_TYPE_MACRO
 }
 
-void FrontendDataTypeController::RecordStartFailure(ConfigureResult result) {
+void FrontendDataTypeController::RecordStartFailure(StartResult result) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeStartFailures",
                             ModelTypeToHistogramInt(type()),

chrome/browser/sync/glue/frontend_data_type_controller.h

@@ -93,14 +93,14 @@ class FrontendDataTypeController : public sync_driver::DataTypeController {
 
   // Helper method for cleaning up state and running the start callback.
   virtual void StartDone(
-      ConfigureResult start_result,
+      StartResult start_result,
       const syncer::SyncMergeResult& local_merge_result,
       const syncer::SyncMergeResult& syncer_merge_result);
 
   // Record association time.
   virtual void RecordAssociationTime(base::TimeDelta time);
   // Record causes of start failure.
-  virtual void RecordStartFailure(ConfigureResult result);
+  virtual void RecordStartFailure(StartResult result);
 
   virtual sync_driver::AssociatorInterface* model_associator() const;
   virtual void set_model_associator(

chrome/browser/sync/glue/frontend_data_type_controller_mock.h

@@ -34,9 +34,9 @@ class FrontendDataTypeControllerMock : public FrontendDataTypeController {
   MOCK_METHOD0(StartModels, bool());
   MOCK_METHOD0(Associate, bool());
   MOCK_METHOD0(CreateSyncComponents, void());
-  MOCK_METHOD2(StartFailed, void(ConfigureResult result,
+  MOCK_METHOD2(StartFailed, void(StartResult result,
                                  const syncer::SyncError& error));
-  MOCK_METHOD1(FinishStart, void(ConfigureResult result));
+  MOCK_METHOD1(FinishStart, void(StartResult result));
   MOCK_METHOD0(CleanUpState, void());
   MOCK_CONST_METHOD0(model_associator, sync_driver::AssociatorInterface*());
   MOCK_METHOD1(set_model_associator,
@@ -47,7 +47,7 @@ class FrontendDataTypeControllerMock : public FrontendDataTypeController {
   MOCK_METHOD2(RecordUnrecoverableError, void(const tracked_objects::Location&,
                                               const std::string&));
   MOCK_METHOD1(RecordAssociationTime, void(base::TimeDelta time));
-  MOCK_METHOD1(RecordStartFailure, void(ConfigureResult result));
+  MOCK_METHOD1(RecordStartFailure, void(StartResult result));
 
  protected:
   virtual ~FrontendDataTypeControllerMock();

chrome/browser/sync/glue/frontend_data_type_controller_unittest.cc

@@ -75,7 +75,7 @@ class FrontendDataTypeControllerFake : public FrontendDataTypeController {
     mock_->RecordAssociationTime(time);
   }
   virtual void RecordStartFailure(
-      DataTypeController::ConfigureResult result) OVERRIDE {
+      DataTypeController::StartResult result) OVERRIDE {
     mock_->RecordStartFailure(result);
   }
  private:
@@ -120,7 +120,7 @@ class SyncFrontendDataTypeControllerTest : public testing::Test {
     EXPECT_CALL(*dtc_mock_.get(), RecordAssociationTime(_));
   }
 
-  void SetActivateExpectations(DataTypeController::ConfigureResult result) {
+  void SetActivateExpectations(DataTypeController::StartResult result) {
     EXPECT_CALL(start_callback_, Run(result, _, _));
   }
 
@@ -131,7 +131,7 @@ class SyncFrontendDataTypeControllerTest : public testing::Test {
                 WillOnce(Return(syncer::SyncError()));
   }
 
-  void SetStartFailExpectations(DataTypeController::ConfigureResult result) {
+  void SetStartFailExpectations(DataTypeController::StartResult result) {
     if (DataTypeController::IsUnrecoverableResult(result))
       EXPECT_CALL(*dtc_mock_.get(), RecordUnrecoverableError(_, _));
     EXPECT_CALL(*dtc_mock_.get(), CleanUpState());

chrome/browser/sync/glue/non_frontend_data_type_controller.cc

@@ -340,7 +340,7 @@ bool NonFrontendDataTypeController::IsOnBackendThread() {
 }
 
 void NonFrontendDataTypeController::StartDone(
-    DataTypeController::ConfigureResult start_result,
+    DataTypeController::StartResult start_result,
     const syncer::SyncMergeResult& local_merge_result,
     const syncer::SyncMergeResult& syncer_merge_result) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -359,7 +359,7 @@ void NonFrontendDataTypeController::StartDone(
 }
 
 void NonFrontendDataTypeController::StartDoneImpl(
-    DataTypeController::ConfigureResult start_result,
+    DataTypeController::StartResult start_result,
     DataTypeController::State new_state,
     const syncer::SyncMergeResult& local_merge_result,
     const syncer::SyncMergeResult& syncer_merge_result) {
@@ -398,7 +398,7 @@ void NonFrontendDataTypeController::RecordAssociationTime(
 #undef PER_DATA_TYPE_MACRO
 }
 
-void NonFrontendDataTypeController::RecordStartFailure(ConfigureResult result) {
+void NonFrontendDataTypeController::RecordStartFailure(StartResult result) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeStartFailures",
                             ModelTypeToHistogramInt(type()),

chrome/browser/sync/glue/non_frontend_data_type_controller.h

@@ -134,13 +134,13 @@ class NonFrontendDataTypeController : public sync_driver::DataTypeController {
   // Start up complete, update the state and invoke the callback.
   // Note: this is performed on the datatype's thread.
   virtual void StartDone(
-      DataTypeController::ConfigureResult start_result,
+      DataTypeController::StartResult start_result,
       const syncer::SyncMergeResult& local_merge_result,
       const syncer::SyncMergeResult& syncer_merge_result);
 
   // UI thread implementation of StartDone.
   virtual void StartDoneImpl(
-      DataTypeController::ConfigureResult start_result,
+      DataTypeController::StartResult start_result,
       DataTypeController::State new_state,
       const syncer::SyncMergeResult& local_merge_result,
       const syncer::SyncMergeResult& syncer_merge_result);
@@ -153,7 +153,7 @@ class NonFrontendDataTypeController : public sync_driver::DataTypeController {
   // Record association time. Called on Datatype's thread.
   virtual void RecordAssociationTime(base::TimeDelta time);
   // Record causes of start failure. Called on UI thread.
-  virtual void RecordStartFailure(ConfigureResult result);
+  virtual void RecordStartFailure(StartResult result);
 
   // Handles the reporting of unrecoverable error. It records stuff in
   // UMA and reports to breakpad.

chrome/browser/sync/glue/non_frontend_data_type_controller_mock.h

@@ -39,11 +39,11 @@ class NonFrontendDataTypeControllerMock : public NonFrontendDataTypeController {
   MOCK_METHOD0(CreateSyncComponents,
                ProfileSyncComponentsFactory::SyncComponents());
   MOCK_METHOD3(StartDone,
-               void(DataTypeController::ConfigureResult result,
+               void(DataTypeController::StartResult result,
                     const syncer::SyncMergeResult& local_merge_result,
                     const syncer::SyncMergeResult& syncer_merge_result));
   MOCK_METHOD4(StartDoneImpl,
-               void(DataTypeController::ConfigureResult result,
+               void(DataTypeController::StartResult result,
                     DataTypeController::State new_state,
                     const syncer::SyncMergeResult& local_merge_result,
                     const syncer::SyncMergeResult& syncer_merge_result));
@@ -53,7 +53,7 @@ class NonFrontendDataTypeControllerMock : public NonFrontendDataTypeController {
   MOCK_METHOD2(RecordUnrecoverableError, void(const tracked_objects::Location&,
                                               const std::string&));
   MOCK_METHOD1(RecordAssociationTime, void(base::TimeDelta time));
-  MOCK_METHOD1(RecordStartFailure, void(ConfigureResult result));
+  MOCK_METHOD1(RecordStartFailure, void(StartResult result));
 
  protected:
   virtual ~NonFrontendDataTypeControllerMock();

chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc

@@ -97,7 +97,7 @@ class NonFrontendDataTypeControllerFake : public NonFrontendDataTypeController {
     mock_->RecordAssociationTime(time);
   }
   virtual void RecordStartFailure(
-      DataTypeController::ConfigureResult result) OVERRIDE {
+      DataTypeController::StartResult result) OVERRIDE {
     mock_->RecordStartFailure(result);
   }
   virtual void DisconnectProcessor(
@@ -160,7 +160,7 @@ class SyncNonFrontendDataTypeControllerTest : public testing::Test {
     EXPECT_CALL(*dtc_mock_.get(), RecordAssociationTime(_));
   }
 
-  void SetActivateExpectations(DataTypeController::ConfigureResult result) {
+  void SetActivateExpectations(DataTypeController::StartResult result) {
     EXPECT_CALL(start_callback_, Run(result, _, _));
   }
 
@@ -171,7 +171,7 @@ class SyncNonFrontendDataTypeControllerTest : public testing::Test {
                 WillOnce(Return(syncer::SyncError()));
   }
 
-  void SetStartFailExpectations(DataTypeController::ConfigureResult result) {
+  void SetStartFailExpectations(DataTypeController::StartResult result) {
     if (DataTypeController::IsUnrecoverableResult(result))
       EXPECT_CALL(*dtc_mock_.get(), RecordUnrecoverableError(_, _));
     if (model_associator_) {

chrome/browser/sync/profile_sync_service.cc

@@ -1470,7 +1470,8 @@ void ProfileSyncService::OnConfigureDone(
   configure_status_ = result.status;
 
   if (backend_mode_ != SYNC) {
-    if (configure_status_ == DataTypeManager::OK) {
+    if (configure_status_ == DataTypeManager::OK ||
+        configure_status_ == DataTypeManager::PARTIAL_SUCCESS) {
       StartSyncingWithServer();
     } else if (!expect_sync_configuration_aborted_) {
       DVLOG(1) << "Backup/rollback backend failed to configure.";
@@ -1481,7 +1482,8 @@ void ProfileSyncService::OnConfigureDone(
   }
 
   if (!sync_configure_start_time_.is_null()) {
-    if (result.status == DataTypeManager::OK) {
+    if (result.status == DataTypeManager::OK ||
+        result.status == DataTypeManager::PARTIAL_SUCCESS) {
       base::Time sync_configure_stop_time = base::Time::Now();
       base::TimeDelta delta = sync_configure_stop_time -
           sync_configure_start_time_;
@@ -1505,7 +1507,8 @@ void ProfileSyncService::OnConfigureDone(
   // The possible status values:
   //    ABORT - Configuration was aborted. This is not an error, if
   //            initiated by user.
-  //    OK - Some or all types succeeded.
+  //    OK - Everything succeeded.
+  //    PARTIAL_SUCCESS - Some datatypes failed to start.
   //    Everything else is an UnrecoverableError. So treat it as such.
 
   // First handle the abort case.
@@ -1517,18 +1520,18 @@ void ProfileSyncService::OnConfigureDone(
   }
 
   // Handle unrecoverable error.
-  if (configure_status_ != DataTypeManager::OK) {
+  if (configure_status_ != DataTypeManager::OK &&
+      configure_status_ != DataTypeManager::PARTIAL_SUCCESS) {
     // Something catastrophic had happened. We should only have one
     // error representing it.
-    syncer::SyncError error =
-        failed_data_types_handler_.GetUnrecoverableError();
+    DCHECK_EQ(result.failed_data_types.size(),
+              static_cast<unsigned int>(1));
+    syncer::SyncError error = result.failed_data_types.begin()->second;
     DCHECK(error.IsSet());
     std::string message =
         "Sync configuration failed with status " +
         DataTypeManager::ConfigureStatusToString(configure_status_) +
-        " caused by " +
-        syncer::ModelTypeSetToString(
-            failed_data_types_handler_.GetUnrecoverableErrorTypes()) +
+        " during " + syncer::ModelTypeToString(error.model_type()) +
         ": " + error.message();
     LOG(ERROR) << "ProfileSyncService error: " << message;
     OnInternalUnrecoverableError(error.location(),

chrome/browser/sync/profile_sync_service_startup_unittest.cc

@@ -45,7 +45,6 @@ using testing::DoAll;
 using testing::InvokeArgument;
 using testing::Mock;
 using testing::Return;
-using testing::SaveArg;
 
 ACTION_P(InvokeOnConfigureStart, pss) {
   ProfileSyncService* service =
@@ -53,13 +52,11 @@ ACTION_P(InvokeOnConfigureStart, pss) {
   service->OnConfigureStart();
 }
 
-ACTION_P3(InvokeOnConfigureDone, pss, error_callback, result) {
+ACTION_P2(InvokeOnConfigureDone, pss, result) {
   ProfileSyncService* service =
       static_cast<ProfileSyncService*>(pss);
   DataTypeManager::ConfigureResult configure_result =
       static_cast<DataTypeManager::ConfigureResult>(result);
-  if (result.status == sync_driver::DataTypeManager::ABORTED)
-    error_callback.Run();
   service->OnConfigureDone(configure_result);
 }
 
@@ -70,8 +67,7 @@ class ProfileSyncServiceStartupTest : public testing::Test {
                        content::TestBrowserThreadBundle::REAL_FILE_THREAD |
                        content::TestBrowserThreadBundle::REAL_IO_THREAD),
         profile_manager_(TestingBrowserProcess::GetGlobal()),
-        sync_(NULL),
-        failed_data_types_handler_(NULL) {}
+        sync_(NULL) {}
 
   virtual ~ProfileSyncServiceStartupTest() {
   }
@@ -131,16 +127,6 @@ class ProfileSyncServiceStartupTest : public testing::Test {
     return static_cast<FakeSigninManagerForTesting*>(sync_->signin());
   }
 
-  void SetError() {
-    sync_driver::FailedDataTypesHandler::TypeErrorMap errors;
-    errors[syncer::BOOKMARKS] =
-        syncer::SyncError(FROM_HERE,
-                          syncer::SyncError::UNRECOVERABLE_ERROR,
-                          "Error",
-                          syncer::BOOKMARKS);
-    failed_data_types_handler_->UpdateFailedDataTypes(errors);
-  }
-
  protected:
   void SimulateTestUserSignin() {
     profile_->GetPrefs()->SetString(prefs::kGoogleServicesUsername,
@@ -157,8 +143,7 @@ class ProfileSyncServiceStartupTest : public testing::Test {
     DataTypeManagerMock* data_type_manager = new DataTypeManagerMock();
     EXPECT_CALL(*components_factory_mock(),
                 CreateDataTypeManager(_, _, _, _, _, _)).
-        WillOnce(DoAll(SaveArg<5>(&failed_data_types_handler_),
-                       Return(data_type_manager)));
+        WillOnce(Return(data_type_manager));
     return data_type_manager;
   }
 
@@ -176,7 +161,6 @@ class ProfileSyncServiceStartupTest : public testing::Test {
   TestingProfile* profile_;
   ProfileSyncService* sync_;
   ProfileSyncServiceObserverMock observer_;
-  sync_driver::FailedDataTypesHandler* failed_data_types_handler_;
 };
 
 class ProfileSyncServiceStartupCrosTest : public ProfileSyncServiceStartupTest {
@@ -524,16 +508,23 @@ TEST_F(ProfileSyncServiceStartupTest, StartFailure) {
   SetUpSyncBackendHost();
   DataTypeManagerMock* data_type_manager = SetUpDataTypeManager();
   DataTypeManager::ConfigureStatus status = DataTypeManager::ABORTED;
+  syncer::SyncError error(
+      FROM_HERE,
+      syncer::SyncError::DATATYPE_ERROR,
+      "Association failed.",
+      syncer::BOOKMARKS);
+  std::map<syncer::ModelType, syncer::SyncError> errors;
+  errors[syncer::BOOKMARKS] = error;
   DataTypeManager::ConfigureResult result(
       status,
+      syncer::ModelTypeSet(),
+      errors,
+      syncer::ModelTypeSet(),
       syncer::ModelTypeSet());
-  EXPECT_CALL(*data_type_manager, Configure(_, _)).WillRepeatedly(
-      DoAll(InvokeOnConfigureStart(sync_),
-            InvokeOnConfigureDone(
-                sync_,
-                base::Bind(&ProfileSyncServiceStartupTest::SetError,
-                           base::Unretained(this)),
-                result)));
+  EXPECT_CALL(*data_type_manager, Configure(_, _)).
+      WillRepeatedly(
+          DoAll(InvokeOnConfigureStart(sync_),
+                InvokeOnConfigureDone(sync_, result)));
   EXPECT_CALL(*data_type_manager, state()).
       WillOnce(Return(DataTypeManager::STOPPED));
   EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber());

components/sync_driver/data_type_controller.cc

@@ -23,11 +23,11 @@ DataTypeController::DataTypeController(
 DataTypeController::~DataTypeController() {
 }
 
-bool DataTypeController::IsUnrecoverableResult(ConfigureResult result) {
+bool DataTypeController::IsUnrecoverableResult(StartResult result) {
   return (result == UNRECOVERABLE_ERROR);
 }
 
-bool DataTypeController::IsSuccessfulResult(ConfigureResult result) {
+bool DataTypeController::IsSuccessfulResult(StartResult result) {
   return (result == OK || result == OK_FIRST_RUN);
 }
 

components/sync_driver/data_type_controller.h

@@ -51,7 +51,7 @@ class DataTypeController
                     // so it is disabled waiting for it to be stopped.
   };
 
-  enum ConfigureResult {
+  enum StartResult {
     OK,                   // The data type has started normally.
     OK_FIRST_RUN,         // Same as OK, but sent on first successful
                           // start for this type for this user as
@@ -61,11 +61,10 @@ class DataTypeController
     UNRECOVERABLE_ERROR,  // An unrecoverable error occured.
     NEEDS_CRYPTO,         // The data type cannot be started yet because it
                           // depends on the cryptographer.
-    RUNTIME_ERROR,        // After starting, a runtime error was encountered.
     MAX_START_RESULT
   };
 
-  typedef base::Callback<void(ConfigureResult,
+  typedef base::Callback<void(StartResult,
                               const syncer::SyncMergeResult&,
                               const syncer::SyncMergeResult&)> StartCallback;
 
@@ -81,10 +80,10 @@ class DataTypeController
 
   // Returns true if the start result should trigger an unrecoverable error.
   // Public so unit tests can use this function as well.
-  static bool IsUnrecoverableResult(ConfigureResult result);
+  static bool IsUnrecoverableResult(StartResult result);
 
   // Returns true if the datatype started successfully.
-  static bool IsSuccessfulResult(ConfigureResult result);
+  static bool IsSuccessfulResult(StartResult result);
 
   // Begins asynchronous operation of loading the model to get it ready for
   // model association. Once the models are loaded the callback will be invoked

components/sync_driver/data_type_controller_mock.h

@@ -16,7 +16,7 @@ class StartCallbackMock {
   StartCallbackMock();
   virtual ~StartCallbackMock();
 
-  MOCK_METHOD3(Run, void(DataTypeController::ConfigureResult result,
+  MOCK_METHOD3(Run, void(DataTypeController::StartResult result,
                          const syncer::SyncMergeResult& local_merge_result,
                          const syncer::SyncMergeResult& syncer_merge_result));
 };

components/sync_driver/data_type_manager.cc

@@ -10,10 +10,25 @@ DataTypeManager::ConfigureResult::ConfigureResult()
     : status(UNKNOWN) {
 }
 
+DataTypeManager::ConfigureResult::ConfigureResult(ConfigureStatus status,
+                                                  syncer::ModelTypeSet
+                                                      requested_types)
+    : status(status),
+      requested_types(requested_types) {
+  DCHECK_EQ(OK, status);
+}
+
 DataTypeManager::ConfigureResult::ConfigureResult(
     ConfigureStatus status,
-    syncer::ModelTypeSet requested_types)
-    : status(status), requested_types(requested_types) {
+    syncer::ModelTypeSet requested_types,
+    std::map<syncer::ModelType, syncer::SyncError> failed_data_types,
+    syncer::ModelTypeSet unfinished_data_types,
+    syncer::ModelTypeSet needs_crypto)
+    : status(status),
+      requested_types(requested_types),
+      failed_data_types(failed_data_types),
+      unfinished_data_types(unfinished_data_types),
+      needs_crypto(needs_crypto) {
 }
 
 DataTypeManager::ConfigureResult::~ConfigureResult() {
@@ -28,11 +43,12 @@ std::string DataTypeManager::ConfigureStatusToString(ConfigureStatus status) {
       return "Aborted";
     case UNRECOVERABLE_ERROR:
       return "Unrecoverable Error";
-    case UNKNOWN:
+    case PARTIAL_SUCCESS:
+      return "Partial Success";
+    default:
       NOTREACHED();
       return std::string();
   }
-  return std::string();
 }
 
 }  // namespace sync_driver

components/sync_driver/data_type_manager.h

@@ -37,7 +37,8 @@ class DataTypeManager {
   // this.
   enum ConfigureStatus {
     UNKNOWN = -1,
-    OK,                  // Configuration finished some or all types.
+    OK,                  // Configuration finished without error.
+    PARTIAL_SUCCESS,     // Some data types had an error while starting up.
     ABORTED,             // Start was aborted by calling Stop() before
                          // all types were started.
     UNRECOVERABLE_ERROR  // We got an unrecoverable error during startup.
@@ -48,10 +49,28 @@ class DataTypeManager {
     ConfigureResult();
     ConfigureResult(ConfigureStatus status,
                     syncer::ModelTypeSet requested_types);
+    ConfigureResult(ConfigureStatus status,
+                    syncer::ModelTypeSet requested_types,
+                    std::map<syncer::ModelType, syncer::SyncError>
+                        failed_data_types,
+                    syncer::ModelTypeSet unfinished_data_types,
+                    syncer::ModelTypeSet needs_crypto);
     ~ConfigureResult();
-
     ConfigureStatus status;
     syncer::ModelTypeSet requested_types;
+
+    // These types encountered a failure in association.
+    std::map<syncer::ModelType, syncer::SyncError> failed_data_types;
+
+    // List of types that failed to finish loading/associating within our
+    // alloted time period(see |kAssociationTimeOutInSeconds|). We move
+    // forward here and allow these types to continue to load/associate in
+    // the background.
+    syncer::ModelTypeSet unfinished_data_types;
+
+    // Those types that are unable to start due to the cryptographer not being
+    // ready.
+    syncer::ModelTypeSet needs_crypto;
   };
 
   virtual ~DataTypeManager() {}

components/sync_driver/data_type_manager_impl.cc

@@ -88,9 +88,7 @@ void DataTypeManagerImpl::Configure(syncer::ModelTypeSet desired_types,
     if (syncer::IsControlType(type.Get()) ||
         iter != controllers_->end()) {
       if (iter != controllers_->end()) {
-        if (!iter->second->ReadyForStart() &&
-            !failed_data_types_handler_->GetUnreadyErrorTypes().Has(
-                type.Get())) {
+        if (!iter->second->ReadyForStart()) {
           // 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.
@@ -101,7 +99,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 if (iter->second->ReadyForStart()) {
+        } else {
           failed_data_types_handler_->ResetUnreadyErrorFor(type.Get());
         }
       }
@@ -347,18 +345,14 @@ void DataTypeManagerImpl::DownloadReady(
   if (!failed_configuration_types.Empty()) {
     if (!unrecoverable_error_method_.is_null())
       unrecoverable_error_method_.Run();
-    FailedDataTypesHandler::TypeErrorMap errors;
-    for (syncer::ModelTypeSet::Iterator iter =
-             failed_configuration_types.First(); iter.Good(); iter.Inc()) {
-      syncer::SyncError error(
-          FROM_HERE,
-          syncer::SyncError::UNRECOVERABLE_ERROR,
-          "Backend failed to download type.",
-          iter.Get());
-      errors[iter.Get()] = error;
-    }
-    failed_data_types_handler_->UpdateFailedDataTypes(errors);
-    Abort(UNRECOVERABLE_ERROR);
+    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);
     return;
   }
 
@@ -406,24 +400,8 @@ void DataTypeManagerImpl::StartNextAssociation() {
 }
 
 void DataTypeManagerImpl::OnSingleDataTypeWillStop(
-    syncer::ModelType type,
-    const syncer::SyncError& error) {
+    syncer::ModelType type) {
   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;
-      ProcessReconfigure();
-    } else {
-      DCHECK_EQ(state_, CONFIGURING);
-    }
-  }
 }
 
 void DataTypeManagerImpl::OnSingleDataTypeAssociationDone(
@@ -471,6 +449,25 @@ 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>();
@@ -479,18 +476,38 @@ void DataTypeManagerImpl::OnModelAssociationDone(
   }
 
   if (result.status == ABORTED || result.status == UNRECOVERABLE_ERROR) {
-    Abort(result.status);
+    Abort(result.status, result.failed_data_types.size() >= 1 ?
+                         result.failed_data_types.begin()->second :
+                         syncer::SyncError());
     return;
   }
 
-  DCHECK(result.status == OK);
+  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;
+  }
 
   association_types_queue_.pop();
   if (!association_types_queue_.empty()) {
     StartNextAssociation();
   } else if (download_types_queue_.empty()) {
     state_ = CONFIGURED;
-    NotifyDone(result);
+    ConfigureResult configure_result(status,
+                                     result.requested_types,
+                                     failed_data_types_handler_->GetAllErrors(),
+                                     result.unfinished_data_types,
+                                     result.needs_crypto);
+    NotifyDone(configure_result);
   }
 }
 
@@ -504,19 +521,29 @@ void DataTypeManagerImpl::Stop() {
 
   if (need_to_notify) {
     ConfigureResult result(ABORTED,
-                           last_requested_types_);
+                           last_requested_types_,
+                           std::map<syncer::ModelType, syncer::SyncError>(),
+                           syncer::ModelTypeSet(),
+                           syncer::ModelTypeSet());
     NotifyDone(result);
   }
 }
 
-void DataTypeManagerImpl::Abort(ConfigureStatus status) {
+void DataTypeManagerImpl::Abort(ConfigureStatus status,
+                                const syncer::SyncError& error) {
   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_);
+                         last_requested_types_,
+                         errors,
+                         syncer::ModelTypeSet(),
+                         syncer::ModelTypeSet());
   NotifyDone(result);
 }
 
@@ -566,7 +593,12 @@ void DataTypeManagerImpl::NotifyDone(const ConfigureResult& result) {
       UMA_HISTOGRAM_LONG_TIMES("Sync.ConfigureTime_Long.UNRECOVERABLE_ERROR",
                                configure_time_delta_);
       break;
-    case DataTypeManager::UNKNOWN:
+    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:
       NOTREACHED();
       break;
   }

components/sync_driver/data_type_manager_impl.h

@@ -68,9 +68,7 @@ class DataTypeManagerImpl : public DataTypeManager,
       const syncer::DataTypeAssociationStats& association_stats) OVERRIDE;
   virtual void OnModelAssociationDone(
       const DataTypeManager::ConfigureResult& result) OVERRIDE;
-  virtual void OnSingleDataTypeWillStop(
-      syncer::ModelType type,
-      const syncer::SyncError& error) OVERRIDE;
+  virtual void OnSingleDataTypeWillStop(syncer::ModelType type) OVERRIDE;
 
   // Used by unit tests. TODO(sync) : This would go away if we made
   // this class be able to do Dependency injection. crbug.com/129212.
@@ -82,7 +80,8 @@ class DataTypeManagerImpl : public DataTypeManager,
   friend class TestDataTypeManager;
 
   // Abort configuration and stop all data types due to configuration errors.
-  void Abort(ConfigureStatus status);
+  void Abort(ConfigureStatus status,
+             const syncer::SyncError& error);
 
   // Returns the priority types (control + priority user types).
   // Virtual for overriding during tests.

components/sync_driver/data_type_manager_impl_unittest.cc

@@ -394,7 +394,7 @@ TEST_F(SyncDataTypeManagerImplTest, OneWaitingForCrypto) {
   AddController(PASSWORDS);
 
   SetConfigureStartExpectation();
-  SetConfigureDoneExpectation(DataTypeManager::OK);
+  SetConfigureDoneExpectation(DataTypeManager::PARTIAL_SUCCESS);
 
   const ModelTypeSet types(PASSWORDS);
   dtm_->set_priority_types(AddHighPriorityTypesTo(types));
@@ -651,7 +651,7 @@ TEST_F(SyncDataTypeManagerImplTest, OneControllerFailsAssociation) {
   AddController(PREFERENCES);
 
   SetConfigureStartExpectation();
-  SetConfigureDoneExpectation(DataTypeManager::OK);
+  SetConfigureDoneExpectation(DataTypeManager::PARTIAL_SUCCESS);
 
   // Step 1.
   Configure(dtm_.get(), ModelTypeSet(BOOKMARKS, PREFERENCES));
@@ -1024,7 +1024,7 @@ TEST_F(SyncDataTypeManagerImplTest, HighPriorityAssociationFailure) {
 
   // Initial configure.
   SetConfigureStartExpectation();
-  SetConfigureDoneExpectation(DataTypeManager::OK);
+  SetConfigureDoneExpectation(DataTypeManager::PARTIAL_SUCCESS);
 
   // Initially only PREFERENCES is configured.
   configurer_.set_expected_configure_types(
@@ -1076,7 +1076,7 @@ TEST_F(SyncDataTypeManagerImplTest, LowPriorityAssociationFailure) {
 
   // Initial configure.
   SetConfigureStartExpectation();
-  SetConfigureDoneExpectation(DataTypeManager::OK);
+  SetConfigureDoneExpectation(DataTypeManager::PARTIAL_SUCCESS);
 
   // Initially only PREFERENCES is configured.
   configurer_.set_expected_configure_types(
@@ -1167,7 +1167,7 @@ TEST_F(SyncDataTypeManagerImplTest, ReenableAfterDataTypeError) {
   AddController(BOOKMARKS);    // Will be disabled due to datatype error.
 
   SetConfigureStartExpectation();
-  SetConfigureDoneExpectation(DataTypeManager::OK);
+  SetConfigureDoneExpectation(DataTypeManager::PARTIAL_SUCCESS);
 
   Configure(dtm_.get(), ModelTypeSet(BOOKMARKS, PREFERENCES));
   FinishDownload(*dtm_, ModelTypeSet(), ModelTypeSet());
@@ -1196,38 +1196,4 @@ TEST_F(SyncDataTypeManagerImplTest, ReenableAfterDataTypeError) {
   EXPECT_EQ(DataTypeController::RUNNING, GetController(BOOKMARKS)->state());
 }
 
-TEST_F(SyncDataTypeManagerImplTest, UnreadyType) {
-  AddController(BOOKMARKS);
-  GetController(BOOKMARKS)->SetReadyForStart(false);
-
-  // Bookmarks is never started due to being unready.
-  SetConfigureStartExpectation();
-  SetConfigureDoneExpectation(DataTypeManager::OK);
-  Configure(dtm_.get(), ModelTypeSet(BOOKMARKS));
-  FinishDownload(*dtm_, ModelTypeSet(), ModelTypeSet());
-  EXPECT_EQ(DataTypeController::NOT_RUNNING, GetController(BOOKMARKS)->state());
-  EXPECT_EQ(DataTypeManager::CONFIGURED, dtm_->state());
-  EXPECT_EQ(0U, configurer_.activated_types().Size());
-  Mock::VerifyAndClearExpectations(&observer_);
-
-  // Bookmarks should start normally now.
-  GetController(BOOKMARKS)->SetReadyForStart(true);
-  SetConfigureStartExpectation();
-  SetConfigureDoneExpectation(DataTypeManager::OK);
-  Configure(dtm_.get(), ModelTypeSet(BOOKMARKS));
-  EXPECT_EQ(DataTypeManager::DOWNLOAD_PENDING, dtm_->state());
-
-  FinishDownload(*dtm_, ModelTypeSet(), ModelTypeSet());
-  FinishDownload(*dtm_, ModelTypeSet(BOOKMARKS), ModelTypeSet());
-  EXPECT_EQ(DataTypeManager::CONFIGURING, dtm_->state());
-
-  GetController(BOOKMARKS)->FinishStart(DataTypeController::OK);
-  EXPECT_EQ(DataTypeManager::CONFIGURED, dtm_->state());
-  EXPECT_EQ(1U, configurer_.activated_types().Size());
-
-  dtm_->Stop();
-  EXPECT_EQ(DataTypeManager::STOPPED, dtm_->state());
-  EXPECT_TRUE(configurer_.activated_types().Empty());
-}
-
 }  // namespace sync_driver

components/sync_driver/failed_data_types_handler.cc

@@ -32,8 +32,6 @@ bool FailedDataTypesHandler::UpdateFailedDataTypes(const TypeErrorMap& errors) {
   if (errors.empty())
     return false;
 
-  DVLOG(1) << "Setting " << errors.size() << " new failed types.";
-
   for (TypeErrorMap::const_iterator iter = errors.begin(); iter != errors.end();
        ++iter) {
     syncer::SyncError::ErrorType failure_type = iter->second.error_type();
@@ -93,11 +91,11 @@ bool FailedDataTypesHandler::ResetUnreadyErrorFor(syncer::ModelType type) {
 FailedDataTypesHandler::TypeErrorMap FailedDataTypesHandler::GetAllErrors()
     const {
   TypeErrorMap result;
+  result = unrecoverable_errors_;
   result.insert(data_type_errors_.begin(), data_type_errors_.end());
   result.insert(crypto_errors_.begin(), crypto_errors_.end());
   result.insert(persistence_errors_.begin(), persistence_errors_.end());
   result.insert(unready_errors_.begin(), unready_errors_.end());
-  result.insert(unrecoverable_errors_.begin(), unrecoverable_errors_.end());
   return result;
 }
 
@@ -110,9 +108,8 @@ syncer::ModelTypeSet FailedDataTypesHandler::GetFailedTypes() const {
 
 syncer::ModelTypeSet FailedDataTypesHandler::GetFatalErrorTypes()
     const {
-  syncer::ModelTypeSet result;
+  syncer::ModelTypeSet result = GetTypesFromErrorMap(unrecoverable_errors_);
   result.PutAll(GetTypesFromErrorMap(data_type_errors_));
-  result.PutAll(GetTypesFromErrorMap(unrecoverable_errors_));
   return result;
 }
 
@@ -131,25 +128,10 @@ syncer::ModelTypeSet FailedDataTypesHandler::GetUnreadyErrorTypes() const {
   return result;
 }
 
-syncer::ModelTypeSet FailedDataTypesHandler::GetUnrecoverableErrorTypes()
-    const {
-  syncer::ModelTypeSet result = GetTypesFromErrorMap(unrecoverable_errors_);
-  return result;
-}
-
-syncer::SyncError FailedDataTypesHandler::GetUnrecoverableError() const {
-  // Just return the first one. It is assumed all the unrecoverable errors
-  // have the same cause. The others are just tracked to know which types
-  // were involved.
-  return (unrecoverable_errors_.empty()
-              ? syncer::SyncError()
-              : unrecoverable_errors_.begin()->second);
-}
-
 bool FailedDataTypesHandler::AnyFailedDataType() const {
   // Note: persistence errors are not failed types. They just trigger automatic
   // unapply + getupdates, at which point they are associated like normal.
-  return unrecoverable_errors_.empty() ||
+  return !unrecoverable_errors_.empty() ||
          !data_type_errors_.empty() ||
          !crypto_errors_.empty();
 }

components/sync_driver/failed_data_types_handler.h

@@ -61,18 +61,12 @@ class FailedDataTypesHandler {
   // Returns the types that cannot be configured due to not being ready.
   syncer::ModelTypeSet GetUnreadyErrorTypes() const;
 
-  // Returns the types that triggered the unrecoverable error.
-  syncer::ModelTypeSet GetUnrecoverableErrorTypes() const;
-
-  // Returns the current unrecoverable error, if there is one.
-  syncer::SyncError GetUnrecoverableError() const;
-
  private:
   // Returns true if there are any types with errors.
   bool AnyFailedDataType() const;
 
-  // The current unrecoverable errors. Only one unrecoverable error can be
-  // active at a time, but it may apply to more than one type.
+  // List of data types that failed due to unrecoverable errors and should
+  // be disabled.
   TypeErrorMap unrecoverable_errors_;
 
   // List of data types that failed due to runtime errors and should be

components/sync_driver/fake_data_type_controller.cc

@@ -16,8 +16,7 @@ FakeDataTypeController::FakeDataTypeController(ModelType type)
                            DisableTypeCallback()),
         state_(NOT_RUNNING),
         model_load_delayed_(false),
-        type_(type),
-        ready_for_start_(true) {}
+        type_(type) {}
 
 FakeDataTypeController::~FakeDataTypeController() {
 }
@@ -55,7 +54,7 @@ void FakeDataTypeController::StartAssociating(
 
 // MODEL_STARTING | ASSOCIATING -> RUNNING | DISABLED | NOT_RUNNING
 // (depending on |result|)
-void FakeDataTypeController::FinishStart(ConfigureResult result) {
+void FakeDataTypeController::FinishStart(StartResult result) {
   // We should have a callback from Start().
   if (last_start_callback_.is_null()) {
     ADD_FAILURE();
@@ -74,13 +73,6 @@ void FakeDataTypeController::FinishStart(ConfigureResult result) {
                           syncer::SyncError::DATATYPE_ERROR,
                           "Association failed",
                           type()));
-  } else if (result == UNRECOVERABLE_ERROR) {
-    state_ = NOT_RUNNING;
-    local_merge_result.set_error(
-        syncer::SyncError(FROM_HERE,
-                          syncer::SyncError::UNRECOVERABLE_ERROR,
-                          "Unrecoverable error",
-                          type()));
   } else {
     state_ = NOT_RUNNING;
     local_merge_result.set_error(
@@ -89,12 +81,15 @@ void FakeDataTypeController::FinishStart(ConfigureResult result) {
                           "Fake error",
                           type()));
   }
-  last_start_callback_.Run(result, local_merge_result, syncer_merge_result);
+  StartCallback start_callback = last_start_callback_;
+  last_start_callback_.Reset();
+  start_callback.Run(result,
+                     local_merge_result,
+                     syncer_merge_result);
 }
 
 // * -> NOT_RUNNING
 void FakeDataTypeController::Stop() {
-  DataTypeController::State old_state = state_;
   state_ = NOT_RUNNING;
   if (!model_load_callback_.is_null()) {
     // Real data type controllers run the callback and specify "ABORTED" as an
@@ -104,7 +99,7 @@ void FakeDataTypeController::Stop() {
   }
 
   // The DTM still expects |last_start_callback_| to be called back.
-  if (old_state != NOT_RUNNING && !last_start_callback_.is_null()) {
+  if (!last_start_callback_.is_null()) {
     syncer::SyncError error(FROM_HERE,
                             syncer::SyncError::DATATYPE_ERROR,
                             "Fake error",
@@ -140,16 +135,7 @@ DataTypeController::State FakeDataTypeController::state() const {
 void FakeDataTypeController::OnSingleDatatypeUnrecoverableError(
     const tracked_objects::Location& from_here,
     const std::string& message) {
-  syncer::SyncError error(
-      from_here, syncer::SyncError::DATATYPE_ERROR, message, type_);
-  syncer::SyncMergeResult local_merge_result(type());
-  local_merge_result.set_error(error);
-  last_start_callback_.Run(
-      RUNTIME_ERROR, local_merge_result, syncer::SyncMergeResult(type_));
-}
-
-bool FakeDataTypeController::ReadyForStart() const {
-  return ready_for_start_;
+  ADD_FAILURE() << message;
 }
 
 void FakeDataTypeController::SetDelayModelLoad() {
@@ -166,8 +152,4 @@ void FakeDataTypeController::SimulateModelLoadFinishing() {
   model_load_callback_.Reset();
 }
 
-void FakeDataTypeController::SetReadyForStart(bool ready) {
-  ready_for_start_ = ready;
-}
-
 }  // namespace sync_driver

components/sync_driver/fake_data_type_controller.h

@@ -25,28 +25,34 @@ class FakeDataTypeController : public DataTypeController {
 
   virtual void LoadModels(
       const ModelLoadCallback& model_load_callback) OVERRIDE;
+
   virtual void OnModelLoaded() OVERRIDE;
+
   virtual void StartAssociating(const StartCallback& start_callback) OVERRIDE;
+
+  void FinishStart(StartResult result);
+
   virtual void Stop() OVERRIDE;
+
   virtual syncer::ModelType type() const OVERRIDE;
+
   virtual std::string name() const OVERRIDE;
+
   virtual syncer::ModelSafeGroup model_safe_group() const OVERRIDE;
+
   virtual ChangeProcessor* GetChangeProcessor() const OVERRIDE;
+
   virtual State state() const OVERRIDE;
+
   virtual void OnSingleDatatypeUnrecoverableError(
       const tracked_objects::Location& from_here,
       const std::string& message) OVERRIDE;
-  virtual bool ReadyForStart() const OVERRIDE;
-
-  void FinishStart(ConfigureResult result);
 
-  void SetDelayModelLoad();
+  virtual void SetDelayModelLoad();
 
   void SetModelLoadError(syncer::SyncError error);
 
-  void SimulateModelLoadFinishing();
-
-  void SetReadyForStart(bool ready);
+  virtual void SimulateModelLoadFinishing();
 
  protected:
   virtual ~FakeDataTypeController();
@@ -58,7 +64,6 @@ class FakeDataTypeController : public DataTypeController {
   StartCallback last_start_callback_;
   ModelLoadCallback model_load_callback_;
   syncer::SyncError load_error_;
-  bool ready_for_start_;
 };
 
 }  // namespace sync_driver

components/sync_driver/model_association_manager.h

@@ -36,8 +36,7 @@ class ModelAssociationManagerDelegate {
   // Called when the ModelAssociationManager has decided it must stop |type|,
   // likely because it is no longer a desired data type or sync is shutting
   // down.
-  virtual void OnSingleDataTypeWillStop(syncer::ModelType type,
-                                        const syncer::SyncError& error) = 0;
+  virtual void OnSingleDataTypeWillStop(syncer::ModelType type) = 0;
 
   // Called when the ModelAssociationManager has tried to perform model
   // association for all desired types and has nothing left to do.
@@ -99,7 +98,7 @@ class ModelAssociationManager {
   // callback will be invoked when the model association is done.
   void TypeStartCallback(syncer::ModelType type,
                          base::TimeTicks type_start_time,
-                         DataTypeController::ConfigureResult start_result,
+                         DataTypeController::StartResult start_result,
                          const syncer::SyncMergeResult& local_merge_result,
                          const syncer::SyncMergeResult& syncer_merge_result);
 
@@ -107,12 +106,16 @@ class ModelAssociationManager {
   // will be passed to |LoadModels| function.
   void ModelLoadCallback(syncer::ModelType type, syncer::SyncError error);
 
+  // When a type fails to load or fails associating this method is invoked to
+  // do the book keeping and do the UMA reporting.
+  void AppendToFailedDatatypesAndLogError(const syncer::SyncError& error);
+
   // Called when all requested types are associated or association times out.
   // Notify |delegate_| of configuration results.
   void ModelAssociationDone();
 
   // A helper to stop an individual datatype.
-  void StopDatatype(const syncer::SyncError& error, DataTypeController* dtc);
+  void StopDatatype(DataTypeController* dtc);
 
   State state_;
 
@@ -133,6 +136,18 @@ class ModelAssociationManager {
   // reconfiguration if not disabled.
   syncer::ModelTypeSet associated_types_;
 
+  // Data types that are still loading/associating when configuration times
+  // out.
+  syncer::ModelTypeSet slow_types_;
+
+  // Collects the list of errors resulting from failing to start a type. This
+  // would eventually be sent to the listeners after all the types have
+  // been given a chance to start.
+  std::map<syncer::ModelType, syncer::SyncError> failed_data_types_info_;
+
+  // The set of types that can't configure due to cryptographer errors.
+  syncer::ModelTypeSet needs_crypto_types_;
+
   // Time when StartAssociationAsync() is called to associate for a set of data
   // types.
   base::TimeTicks association_start_time_;

components/sync_driver/non_ui_data_type_controller.cc

@@ -111,8 +111,11 @@ void NonUIDataTypeController::StartAssociating(
 
 void NonUIDataTypeController::Stop() {
   DCHECK(ui_thread_->BelongsToCurrentThread());
-  if (state() == NOT_RUNNING)
+  if (state() == NOT_RUNNING) {
+    // Stop() should never be called for datatypes that are already stopped.
+    NOTREACHED();
     return;
+  }
 
   // Disconnect the change processor. At this point, the
   // syncer::SyncableService can no longer interact with the Syncer, even if
@@ -172,6 +175,9 @@ void NonUIDataTypeController::OnSingleDatatypeUnrecoverableError(
   // TODO(tim): We double-upload some errors.  See bug 383480.
   if (!error_callback_.is_null())
     error_callback_.Run();
+  UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeRunFailures",
+                            ModelTypeToHistogramInt(type()),
+                            syncer::MODEL_TYPE_COUNT);
   ui_thread_->PostTask(from_here,
       base::Bind(&NonUIDataTypeController::DisableImpl,
                  this,
@@ -187,7 +193,7 @@ NonUIDataTypeController::NonUIDataTypeController()
 NonUIDataTypeController::~NonUIDataTypeController() {}
 
 void NonUIDataTypeController::StartDone(
-    DataTypeController::ConfigureResult start_result,
+    DataTypeController::StartResult start_result,
     const syncer::SyncMergeResult& local_merge_result,
     const syncer::SyncMergeResult& syncer_merge_result) {
   DCHECK(!ui_thread_->BelongsToCurrentThread());
@@ -209,7 +215,7 @@ void NonUIDataTypeController::StartDone(
 }
 
 void NonUIDataTypeController::StartDoneImpl(
-    DataTypeController::ConfigureResult start_result,
+    DataTypeController::StartResult start_result,
     DataTypeController::State new_state,
     const syncer::SyncMergeResult& local_merge_result,
     const syncer::SyncMergeResult& syncer_merge_result) {
@@ -226,6 +232,7 @@ void NonUIDataTypeController::StartDoneImpl(
   // (due to Stop being called) and then posted from the non-UI thread. In
   // this case, we drop the second call because we've already been stopped.
   if (state_ == NOT_RUNNING) {
+    DCHECK(start_callback_.is_null());
     return;
   }
 
@@ -236,7 +243,12 @@ void NonUIDataTypeController::StartDoneImpl(
     RecordStartFailure(start_result);
   }
 
-  start_callback_.Run(start_result, local_merge_result, syncer_merge_result);
+  // We have to release the callback before we call it, since it's possible
+  // invoking the callback will trigger a call to STOP(), which will get
+  // confused by the non-NULL start_callback_.
+  StartCallback callback = start_callback_;
+  start_callback_.Reset();
+  callback.Run(start_result, local_merge_result, syncer_merge_result);
 }
 
 void NonUIDataTypeController::RecordAssociationTime(base::TimeDelta time) {
@@ -247,7 +259,7 @@ void NonUIDataTypeController::RecordAssociationTime(base::TimeDelta time) {
 #undef PER_DATA_TYPE_MACRO
 }
 
-void NonUIDataTypeController::RecordStartFailure(ConfigureResult result) {
+void NonUIDataTypeController::RecordStartFailure(StartResult result) {
   DCHECK(ui_thread_->BelongsToCurrentThread());
   UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeStartFailures",
                             ModelTypeToHistogramInt(type()),
@@ -275,18 +287,8 @@ void NonUIDataTypeController::DisableImpl(
     const tracked_objects::Location& from_here,
     const std::string& message) {
   DCHECK(ui_thread_->BelongsToCurrentThread());
-  UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeRunFailures",
-                            ModelTypeToHistogramInt(type()),
-                            syncer::MODEL_TYPE_COUNT);
-  if (!start_callback_.is_null()) {
-    syncer::SyncError error(
-        from_here, syncer::SyncError::DATATYPE_ERROR, message, type());
-    syncer::SyncMergeResult local_merge_result(type());
-    local_merge_result.set_error(error);
-    start_callback_.Run(RUNTIME_ERROR,
-                        local_merge_result,
-                        syncer::SyncMergeResult(type()));
-  }
+  if (!disable_callback().is_null())
+    disable_callback().Run(from_here, message);
 }
 
 bool NonUIDataTypeController::StartAssociationAsync() {
@@ -346,11 +348,6 @@ void NonUIDataTypeController::
   }
 
   if (!shared_change_processor->CryptoReadyIfNecessary()) {
-    syncer::SyncError error(FROM_HERE,
-                            syncer::SyncError::CRYPTO_ERROR,
-                            "",
-                            type());
-    local_merge_result.set_error(error);
     StartDone(NEEDS_CRYPTO,
               local_merge_result,
               syncer_merge_result);

components/sync_driver/non_ui_data_type_controller.h

@@ -77,13 +77,13 @@ class NonUIDataTypeController : public DataTypeController {
   // Start up complete, update the state and invoke the callback.
   // Note: this is performed on the datatype's thread.
   virtual void StartDone(
-      DataTypeController::ConfigureResult start_result,
+      DataTypeController::StartResult start_result,
       const syncer::SyncMergeResult& local_merge_result,
       const syncer::SyncMergeResult& syncer_merge_result);
 
   // UI thread implementation of StartDone.
   virtual void StartDoneImpl(
-      DataTypeController::ConfigureResult start_result,
+      DataTypeController::StartResult start_result,
       DataTypeController::State new_state,
       const syncer::SyncMergeResult& local_merge_result,
       const syncer::SyncMergeResult& syncer_merge_result);
@@ -95,7 +95,7 @@ class NonUIDataTypeController : public DataTypeController {
   virtual void RecordAssociationTime(base::TimeDelta time);
 
   // Record causes of start failure.
-  virtual void RecordStartFailure(ConfigureResult result);
+  virtual void RecordStartFailure(StartResult result);
 
   // To allow unit tests to control thread interaction during non-ui startup
   // and shutdown, use a factory method to create the SharedChangeProcessor.

components/sync_driver/non_ui_data_type_controller_mock.h

@@ -37,16 +37,16 @@ class NonUIDataTypeControllerMock
                bool(const tracked_objects::Location&,
                     const base::Closure&));
   MOCK_METHOD3(StartDone,
-               void(DataTypeController::ConfigureResult result,
+               void(DataTypeController::StartResult result,
                     const syncer::SyncMergeResult& local_merge_result,
                     const syncer::SyncMergeResult& syncer_merge_result));
   MOCK_METHOD4(StartDoneImpl,
-               void(DataTypeController::ConfigureResult result,
+               void(DataTypeController::StartResult result,
                     DataTypeController::State new_state,
                     const syncer::SyncMergeResult& local_merge_result,
                     const syncer::SyncMergeResult& syncer_merge_result));
   MOCK_METHOD1(RecordAssociationTime, void(base::TimeDelta time));
-  MOCK_METHOD1(RecordStartFailure, void(ConfigureResult result));
+  MOCK_METHOD1(RecordStartFailure, void(StartResult result));
 
  protected:
   virtual ~NonUIDataTypeControllerMock();

components/sync_driver/non_ui_data_type_controller_unittest.cc

@@ -159,7 +159,7 @@ class NonUIDataTypeControllerFake
   virtual void RecordAssociationTime(base::TimeDelta time) OVERRIDE {
     mock_->RecordAssociationTime(time);
   }
-  virtual void RecordStartFailure(DataTypeController::ConfigureResult result)
+  virtual void RecordStartFailure(DataTypeController::StartResult result)
       OVERRIDE {
     mock_->RecordStartFailure(result);
   }
@@ -243,7 +243,7 @@ class SyncNonUIDataTypeControllerTest : public testing::Test {
     EXPECT_CALL(*dtc_mock_.get(), RecordAssociationTime(_));
   }
 
-  void SetActivateExpectations(DataTypeController::ConfigureResult result) {
+  void SetActivateExpectations(DataTypeController::StartResult result) {
     EXPECT_CALL(start_callback_, Run(result,_,_));
   }
 
@@ -252,7 +252,7 @@ class SyncNonUIDataTypeControllerTest : public testing::Test {
     EXPECT_CALL(*change_processor_.get(), Disconnect()).WillOnce(Return(true));
   }
 
-  void SetStartFailExpectations(DataTypeController::ConfigureResult result) {
+  void SetStartFailExpectations(DataTypeController::StartResult result) {
     EXPECT_CALL(*dtc_mock_.get(), StopModels()).Times(AtLeast(1));
     EXPECT_CALL(*dtc_mock_.get(), RecordStartFailure(result));
     EXPECT_CALL(start_callback_, Run(result, _, _));
@@ -496,17 +496,17 @@ TEST_F(SyncNonUIDataTypeControllerTest, StopStart) {
   EXPECT_EQ(DataTypeController::RUNNING, non_ui_dtc_->state());
 }
 
-TEST_F(SyncNonUIDataTypeControllerTest, OnSingleDatatypeUnrecoverableError) {
+TEST_F(SyncNonUIDataTypeControllerTest,
+       OnSingleDatatypeUnrecoverableError) {
   SetStartExpectations();
   SetAssociateExpectations();
   SetActivateExpectations(DataTypeController::OK);
+  SetStopExpectations();
   EXPECT_EQ(DataTypeController::NOT_RUNNING, non_ui_dtc_->state());
   Start();
   WaitForDTC();
   EXPECT_EQ(DataTypeController::RUNNING, non_ui_dtc_->state());
-
-  testing::Mock::VerifyAndClearExpectations(&start_callback_);
-  EXPECT_CALL(start_callback_, Run(DataTypeController::RUNTIME_ERROR, _, _));
+  // This should cause non_ui_dtc_->Stop() to be called.
   backend_thread_.message_loop_proxy()->PostTask(FROM_HERE, base::Bind(
       &NonUIDataTypeControllerFake::
           OnSingleDatatypeUnrecoverableError,
@@ -514,6 +514,8 @@ TEST_F(SyncNonUIDataTypeControllerTest, OnSingleDatatypeUnrecoverableError) {
       FROM_HERE,
       std::string("Test")));
   WaitForDTC();
+  EXPECT_EQ(DataTypeController::NOT_RUNNING, non_ui_dtc_->state());
+  EXPECT_TRUE(disable_callback_invoked_);
 }
 
 }  // namespace

components/sync_driver/ui_data_type_controller.cc

@@ -145,11 +145,6 @@ void UIDataTypeController::Associate() {
   }
 
   if (!shared_change_processor_->CryptoReadyIfNecessary()) {
-    syncer::SyncError error(FROM_HERE,
-                            syncer::SyncError::CRYPTO_ERROR,
-                            "",
-                            type());
-    local_merge_result.set_error(error);
     StartDone(NEEDS_CRYPTO,
               local_merge_result,
               syncer_merge_result);
@@ -242,7 +237,7 @@ void UIDataTypeController::AbortModelLoad() {
 }
 
 void UIDataTypeController::StartDone(
-    ConfigureResult start_result,
+    StartResult start_result,
     const syncer::SyncMergeResult& local_merge_result,
     const syncer::SyncMergeResult& syncer_merge_result) {
   DCHECK(ui_thread_->BelongsToCurrentThread());
@@ -262,7 +257,12 @@ void UIDataTypeController::StartDone(
     }
   }
 
-  start_callback_.Run(start_result, local_merge_result, syncer_merge_result);
+  // We have to release the callback before we call it, since it's possible
+  // invoking the callback will trigger a call to Stop(), which will get
+  // confused by the non-NULL start_callback_.
+  StartCallback callback = start_callback_;
+  start_callback_.Reset();
+  callback.Run(start_result, local_merge_result, syncer_merge_result);
 }
 
 void UIDataTypeController::Stop() {
@@ -285,6 +285,7 @@ void UIDataTypeController::Stop() {
     // still in MODEL_STARTING.
     return;
   }
+  DCHECK(start_callback_.is_null());
 
   StopModels();
 
@@ -326,15 +327,8 @@ 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::SyncError error(
-        from_here, syncer::SyncError::DATATYPE_ERROR, message, type());
-    syncer::SyncMergeResult local_merge_result(type());
-    local_merge_result.set_error(error);
-    start_callback_.Run(RUNTIME_ERROR,
-                        local_merge_result,
-                        syncer::SyncMergeResult(type()));
-  }
+  if (!disable_callback().is_null())
+    disable_callback().Run(from_here, message);
 }
 
 void UIDataTypeController::RecordAssociationTime(base::TimeDelta time) {
@@ -345,7 +339,7 @@ void UIDataTypeController::RecordAssociationTime(base::TimeDelta time) {
 #undef PER_DATA_TYPE_MACRO
 }
 
-void UIDataTypeController::RecordStartFailure(ConfigureResult result) {
+void UIDataTypeController::RecordStartFailure(StartResult result) {
   DCHECK(ui_thread_->BelongsToCurrentThread());
   UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeStartFailures",
                             ModelTypeToHistogramInt(type()),

components/sync_driver/ui_data_type_controller.h

@@ -81,14 +81,14 @@ class UIDataTypeController : public DataTypeController {
   virtual void OnModelLoaded() OVERRIDE;
 
   // Helper method for cleaning up state and invoking the start callback.
-  virtual void StartDone(ConfigureResult result,
+  virtual void StartDone(StartResult result,
                          const syncer::SyncMergeResult& local_merge_result,
                          const syncer::SyncMergeResult& syncer_merge_result);
 
   // Record association time.
   virtual void RecordAssociationTime(base::TimeDelta time);
   // Record causes of start failure.
-  virtual void RecordStartFailure(ConfigureResult result);
+  virtual void RecordStartFailure(StartResult result);
 
   SyncApiComponentFactory* const sync_factory_;
 

components/sync_driver/ui_data_type_controller_unittest.cc

@@ -15,7 +15,6 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::_;
-using testing::Invoke;
 using testing::InvokeWithoutArgs;
 using testing::Return;
 
@@ -197,10 +196,11 @@ TEST_F(SyncUIDataTypeControllerTest, OnSingleDatatypeUnrecoverableError) {
   EXPECT_FALSE(syncable_service_.syncing());
   Start();
   EXPECT_TRUE(syncable_service_.syncing());
-
-  testing::Mock::VerifyAndClearExpectations(&start_callback_);
-  EXPECT_CALL(start_callback_, Run(DataTypeController::RUNTIME_ERROR, _, _));
   preference_dtc_->OnSingleDatatypeUnrecoverableError(FROM_HERE, "Test");
+  PumpLoop();
+  EXPECT_EQ(DataTypeController::NOT_RUNNING, preference_dtc_->state());
+  EXPECT_TRUE(disable_callback_invoked_);
+  EXPECT_FALSE(syncable_service_.syncing());
 }
 
 }  // namespace

sync/api/sync_error.cc

@@ -149,7 +149,7 @@ std::string SyncError::ToString() const {
 void SyncError::PrintLogError() const {
   logging::LogSeverity logSeverity =
       (GetSeverity() == SYNC_ERROR_SEVERITY_INFO)
-          ? logging::LOG_VERBOSE : logging::LOG_ERROR;
+          ? logging::LOG_INFO : logging::LOG_ERROR;
 
   LAZY_STREAM(logging::LogMessage(location_->file_name(),
                                   location_->line_number(),