Sync: Clear Sync data on all unrecoverable errors

Previously, we cleared Sync data on most, but not all types of
unrecoverable errors: Engine initialization failure was treated
specially. Per the TODO removed in this CL, there was no reason
for the special treatment anymore.

Bug: 839834
Change-Id: I3b473c65b837c810e6fe8456defffe6ebe6e53a2
Reviewed-on: https://chromium-review.googlesource.com/1107991Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569226}

components/browser_sync/profile_sync_service.cc

@@ -795,13 +795,12 @@ void ProfileSyncService::OnUnrecoverableError(const base::Location& from_here,
   // Unrecoverable errors that arrive via the syncer::UnrecoverableErrorHandler
   // interface are assumed to originate within the syncer.
   unrecoverable_error_reason_ = ERROR_REASON_SYNCER;
-  OnUnrecoverableErrorImpl(from_here, message, CLEAR_DATA);
+  OnUnrecoverableErrorImpl(from_here, message);
 }
 
 void ProfileSyncService::OnUnrecoverableErrorImpl(
     const base::Location& from_here,
-    const std::string& message,
-    SyncStopDataFate data_fate) {
+    const std::string& message) {
   DCHECK(HasUnrecoverableError());
   unrecoverable_error_message_ = message;
   unrecoverable_error_location_ = from_here;
@@ -814,12 +813,10 @@ void ProfileSyncService::OnUnrecoverableErrorImpl(
   NotifyObservers();
 
   // Shut all data types down.
-  syncer::ShutdownReason reason =
-      (data_fate == KEEP_DATA) ? syncer::STOP_SYNC : syncer::DISABLE_SYNC;
   base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::BindOnce(&ProfileSyncService::ShutdownImpl,
-                     sync_enabled_weak_factory_.GetWeakPtr(), reason));
+      FROM_HERE, base::BindOnce(&ProfileSyncService::ShutdownImpl,
+                                sync_enabled_weak_factory_.GetWeakPtr(),
+                                syncer::DISABLE_SYNC));
 }
 
 void ProfileSyncService::ReenableDatatype(syncer::ModelType type) {
@@ -862,18 +859,8 @@ void ProfileSyncService::OnEngineInitialized(
   if (!success) {
     // Something went unexpectedly wrong.  Play it safe: stop syncing at once
     // and surface error UI to alert the user sync has stopped.
-    // Keep the sync data around for now so that on restart we will retry
-    // again and potentially succeed in presence of transient file IO failures
-    // or permissions issues, etc.
-    //
-    // TODO(rlarocque): Consider making this UnrecoverableError less special.
-    // Unlike every other UnrecoverableError, it does not delete our sync data.
-    // This exception made sense at the time it was implemented, but our new
-    // directory corruption recovery mechanism makes it obsolete.  By the time
-    // we get here, we will have already tried and failed to delete the
-    // directory.  It would be no big deal if we tried to delete it again.
     OnInternalUnrecoverableError(FROM_HERE, "BackendInitialize failure",
-                                 KEEP_DATA, ERROR_REASON_ENGINE_INIT_FAILURE);
+                                 ERROR_REASON_ENGINE_INIT_FAILURE);
     return;
   }
 
@@ -1010,7 +997,7 @@ void ProfileSyncService::OnActionableError(
       // Trigger an unrecoverable error to stop syncing.
       OnInternalUnrecoverableError(FROM_HERE,
                                    last_actionable_error_.error_description,
-                                   CLEAR_DATA, ERROR_REASON_ACTIONABLE_ERROR);
+                                   ERROR_REASON_ACTIONABLE_ERROR);
       break;
     case syncer::DISABLE_SYNC_ON_CLIENT:
       if (error.error_type == syncer::NOT_MY_BIRTHDAY) {
@@ -1139,7 +1126,7 @@ void ProfileSyncService::OnConfigureDone(
             result.data_type_status_table.GetUnrecoverableErrorTypes()) +
         ": " + error.message();
     LOG(ERROR) << "ProfileSyncService error: " << message;
-    OnInternalUnrecoverableError(error.location(), message, CLEAR_DATA,
+    OnInternalUnrecoverableError(error.location(), message,
                                  ERROR_REASON_CONFIGURATION_FAILURE);
     return;
   }
@@ -2003,11 +1990,10 @@ syncer::ModelTypeSet ProfileSyncService::GetDataTypesFromPreferenceProviders()
 void ProfileSyncService::OnInternalUnrecoverableError(
     const base::Location& from_here,
     const std::string& message,
-    SyncStopDataFate data_fate,
     UnrecoverableErrorReason reason) {
   DCHECK(!HasUnrecoverableError());
   unrecoverable_error_reason_ = reason;
-  OnUnrecoverableErrorImpl(from_here, message, data_fate);
+  OnUnrecoverableErrorImpl(from_here, message);
 }
 
 bool ProfileSyncService::IsRetryingAccessTokenFetchForTest() const {

components/browser_sync/profile_sync_service.h

@@ -539,10 +539,9 @@ class ProfileSyncService : public syncer::SyncService,
   // Helper method for managing encryption UI.
   bool IsEncryptedDatatypeEnabled() const;
 
-  // Helper for OnUnrecoverableError.
+  // Helper for OnUnrecoverableError and OnInternalUnrecoverableError.
   void OnUnrecoverableErrorImpl(const base::Location& from_here,
-                                const std::string& message,
-                                SyncStopDataFate data_fate);
+                                const std::string& message);
 
   // Stops the sync engine. Does NOT set IsSyncRequested to false. Use
   // RequestStop for that. |data_fate| controls whether the local sync data is
@@ -583,7 +582,6 @@ class ProfileSyncService : public syncer::SyncService,
   // Sync.UnrecoverableErrors histogram.
   void OnInternalUnrecoverableError(const base::Location& from_here,
                                     const std::string& message,
-                                    SyncStopDataFate data_fate,
                                     UnrecoverableErrorReason reason);
 
   // Update UMA for syncing engine.