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

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 487 matching lines @@
 
 void ProfileSyncService::StartUp(StartUpDeferredOption deferred_option) {
   // Don't start up multiple times.
   if (backend_) {
     DVLOG(1) << "Skipping bringing up backend host.";
     return;
   }
 
   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 &&
       CommandLine::ForCurrentProcess()->
           HasSwitch(switches::kSyncEnableDeferredStartup)) {
     return;
   }
 
   StartUpSlowBackendComponents();
 }
@@ skipping 1561 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);
 }