sync: Attempt to recover from directory corruption

chrome/browser/sync/profile_sync_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/profile_sync_service.h"
 
 #include <cstddef>
 #include <map>
 #include <set>
 #include <utility>
@@ skipping 367 matching lines @@
   // In platforms with auto start we would just wait for the
   // configure to finish. In other platforms we would throw
   // an unrecoverable error. The reason we do this is so that
   // the login dialog would show an error and the user would have
   // to relogin.
   // Also if backend has been initialized(the user is authenticated
   // and nigori is downloaded) we would simply wait rather than going into
   // unrecoverable error, even if the platform has auto start disabled.
   // Note: In those scenarios the UI does not wait for the configuration
   // to finish.
+  //
+  // TODO(rlarocque): Consider making this UnrecoverableError less special.
+  // 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.
+  // Whether or not we try to delete it again after the error is thrown likely
+  // won't make a difference.

[tim (not reviewing), 2012/08/01 21:31:59]

Hmm... I don't follow.  I thought this was called when a sync cycle failed to
download nigori, but the scheduler has set up another pending job.  How or where
will we have tried to delete the directory ?

[rlarocque, 2012/08/01 21:53:50]

Woops, that was a dumb mistake.  This comment belongs next to the
UnrecoverableError on line 673.

   if (!auto_start_enabled_ && !backend_initialized_) {
     OnInternalUnrecoverableError(FROM_HERE,
                                  "Configure failed to download.",
                                  true,
                                  ERROR_REASON_CONFIGURATION_RETRY);
   }
 
   NotifyObservers();
 }
 
@@ skipping 235 matching lines @@
 
   MessageLoop::current()->PostTask(FROM_HERE,
       base::Bind(&ProfileSyncService::ReconfigureDatatypeManager,
                  weak_factory_.GetWeakPtr()));
 }
 
 void ProfileSyncService::OnBackendInitialized(
     const syncer::WeakHandle<syncer::JsBackend>& js_backend, bool success) {
   is_first_time_sync_configure_ = !HasSyncSetupCompleted();
 
   if (is_first_time_sync_configure_) {

[rlarocque, 2012/08/01 00:25:10]

I'm considering removing these UMAs.  They were useful mainly to track directory
corruption.  However, they're completely unaware of the delete and retry that
now resides in on_disk_directory_backing_store.cc.

Are there any objections to removing them?

     UMA_HISTOGRAM_BOOLEAN("Sync.BackendInitializeFirstTimeSuccess", success);
   } else {
     UMA_HISTOGRAM_BOOLEAN("Sync.BackendInitializeRestoreSuccess", success);
   }
 
   if (!start_up_time_.is_null()) {
     base::Time on_backend_initialized_time = base::Time::Now();
     base::TimeDelta delta = on_backend_initialized_time - start_up_time_;
     if (is_first_time_sync_configure_) {
       UMA_HISTOGRAM_LONG_TIMES("Sync.BackendInitializeFirstTime", delta);
@@ skipping 1101 matching lines @@
   // See http://stackoverflow.com/questions/6224121/is-new-this-myclass-undefined-behaviour-after-directly-calling-the-destru.
   ProfileSyncService* old_this = this;
   this->~ProfileSyncService();
   new(old_this) ProfileSyncService(
       new ProfileSyncComponentsFactoryImpl(profile,
                                            CommandLine::ForCurrentProcess()),
       profile,
       signin,
       behavior);
 }