sync: some fixups for --sync-enabled-deferred-init

chrome/browser/sync/profile_sync_service.cc

Index: chrome/browser/sync/profile_sync_service.cc
diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc
index 6bde4cca7f81b212fd414293d8584760798ffb47..180b3eb3ac775212d23c458a49d8f6435c7daa59 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -505,26 +505,30 @@ void ProfileSyncService::StartUp(StartUpDeferredOption deferred_option) {
 
   DCHECK(IsSyncEnabledAndLoggedIn());
 
-  last_synced_time_ = sync_prefs_.GetLastSyncedTime();
-
-  DCHECK(start_up_time_.is_null());
-  start_up_time_ = base::Time::Now();
+  if (start_up_time_.is_null()) {

[haitaol1, 2013/05/15 20:20:31]

Do you have to put all the block in if? Is it ok to set only start_up_time_ if
it's null?

[tim (not reviewing), 2013/05/15 23:18:14]

It is currently used to report the time spent deferred since the first call to
StartUp, which will be ProfileSyncService Initialization, which is done on
construction by the factory, which will eventually be done very early on when
the Profile is created.

[haitaol1, 2013/05/16 00:05:57]

Lock-free shutdown may be an issue in future because start_up_time_ is not null
if backend is shut down before initialized. Can you move 
start_up_time_ = base::Time();
in OnBackendInitialized() to ShutdownImpl()?

On 2013/05/15 23:18:14, timsteele wrote:

[haitaol1, 2013/05/16 00:06:59]

Actually, that could happen today, right?

On 2013/05/16 00:05:57, haitaol1 wrote:

[tim (not reviewing), 2013/05/16 00:17:25]

OnBackendInitialized should always be called if we've managed to ask the
SyncManager to Init itself, so I don't think this causes a problem in practice. 
But it seems fine to move it, so I did.

[haitaol1, 2013/05/16 16:17:33]

SyncBackendHost::StopSyncingForShutdown nulls frontend_. So I don't think
OnBackendInitialized is guaranteed to be called. Actually, another case where
start_up_time_ is not reset on shutdown is when shutdown is called but backend
is in deferred startup mode. 

On 2013/05/16 00:17:25, timsteele wrote:

+    start_up_time_ = base::Time::Now();
+    last_synced_time_ = sync_prefs_.GetLastSyncedTime();
 
 #if defined(OS_CHROMEOS)
-  std::string bootstrap_token = sync_prefs_.GetEncryptionBootstrapToken();
-  if (bootstrap_token.empty()) {
-    sync_prefs_.SetEncryptionBootstrapToken(
-        sync_prefs_.GetSpareBootstrapToken());
-  }
+    std::string bootstrap_token = sync_prefs_.GetEncryptionBootstrapToken();
+    if (bootstrap_token.empty()) {
+      sync_prefs_.SetEncryptionBootstrapToken(
+          sync_prefs_.GetSpareBootstrapToken());
+    }
 #endif
 
-  if (!sync_global_error_) {
+    if (!sync_global_error_) {
 #if !defined(OS_ANDROID)
-    sync_global_error_.reset(new SyncGlobalError(this, signin()));
+      sync_global_error_.reset(new SyncGlobalError(this, signin()));
 #endif
-    GlobalErrorServiceFactory::GetForProfile(profile_)->AddGlobalError(
-        sync_global_error_.get());
-    AddObserver(sync_global_error_.get());
+      GlobalErrorServiceFactory::GetForProfile(profile_)->AddGlobalError(
+          sync_global_error_.get());

[haitaol1, 2013/05/16 00:05:57]

sync_global_error_ is NULL if defined(OS_ANDROID). Is that right?

[tim (not reviewing), 2013/05/16 00:17:25]

I believe so, because we don't have errors showing up as Tools menu items on
Android.

+      AddObserver(sync_global_error_.get());
+    }
+  } else {
+    // We don't care to prevent multiple calls to StartUp in deferred mode
+    // because it's fast and has no side effects.
+    DCHECK_EQ(STARTUP_BACKEND_DEFERRED, deferred_option);
   }
 
   if (deferred_option == STARTUP_BACKEND_DEFERRED &&