Retry: sync: Gracefully handle early shutdown

chrome/browser/sync/glue/sync_backend_host.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/glue/sync_backend_host.h"
 
 #include <algorithm>
 #include <map>
 
 #include "base/bind.h"
@@ skipping 180 matching lines @@
   // This includes refreshing encryption, setting up the device info change
   // processor, etc.
   void DoInitialProcessControlTypes();
 
   // Some parts of DoInitialProcessControlTypes() may be executed on a different
   // thread.  This function asynchronously continues the work started in
   // DoInitialProcessControlTypes() once that other thread gets back to us.
   void DoFinishInitialProcessControlTypes();
 
   // The shutdown order is a bit complicated:
-  // 1) Call DoStopSyncManagerForShutdown() from |frontend_loop_| to request
-  //    sync manager to stop as soon as possible.
+  // 1) Call the SyncManagerStopHandle's RequestStop() from |frontend_loop_| to
+  //    request sync manager to stop as soon as possible.
   // 2) Post DoShutdown() to sync loop to clean up backend state, save
   //    directory and destroy sync manager.
-  void DoStopSyncManagerForShutdown();
-  void DoShutdown(bool sync_disabled);
+  void DoShutdown(bool sync_disabled,
+                  scoped_ptr<syncer::CancelationSignal> signal1,

[tim (not reviewing), 2013/09/16 18:30:11]

Explain here why we pass ownership of these signals.

[rlarocque, 2013/09/16 18:39:09]

It's explained in a comment near where this function is invoked. I've added a
short comment here that references the comment in SyncBackendHost::Shutdown().

+                  scoped_ptr<syncer::CancelationSignal> signal2);
   void DoDestroySyncManager();
 
   // Configuration methods that must execute on sync loop.
   void DoConfigureSyncer(
       syncer::ConfigureReason reason,
       const DoConfigureSyncerTypes& config_types,
       const syncer::ModelSafeRoutingInfo routing_info,
       const base::Callback<void(syncer::ModelTypeSet,
                                 syncer::ModelTypeSet)>& ready_task,
       const base::Closure& retry_callback);
@@ skipping 81 matching lines @@
       profile_(profile),
       name_(name),
       core_(new Core(name_, profile_->GetPath().Append(kSyncDataFolderName),
                      weak_ptr_factory_.GetWeakPtr())),
       initialization_state_(NOT_ATTEMPTED),
       sync_prefs_(sync_prefs),
       frontend_(NULL),
       cached_passphrase_type_(syncer::IMPLICIT_PASSPHRASE),
       invalidator_(
           invalidation::InvalidationServiceFactory::GetForProfile(profile)),
-      invalidation_handler_registered_(false) {
+      invalidation_handler_registered_(false),
+      factory_cancelation_signal_(new syncer::CancelationSignal()),
+      scm_cancelation_signal_(new syncer::CancelationSignal()) {
 }
 
 SyncBackendHost::SyncBackendHost(Profile* profile)
     : weak_ptr_factory_(this),
       frontend_loop_(base::MessageLoop::current()),
       profile_(profile),
       name_("Unknown"),
       initialization_state_(NOT_ATTEMPTED),
       frontend_(NULL),
       cached_passphrase_type_(syncer::IMPLICIT_PASSPHRASE),
       invalidation_handler_registered_(false) {
 }
 
 SyncBackendHost::~SyncBackendHost() {
   DCHECK(!core_.get() && !frontend_) << "Must call Shutdown before destructor.";
   DCHECK(!registrar_.get());
 }
 
-namespace {
-
-scoped_ptr<syncer::HttpPostProviderFactory> MakeHttpBridgeFactory(
-    const scoped_refptr<net::URLRequestContextGetter>& getter,
-    const NetworkTimeTracker::UpdateCallback& update_callback) {
-  chrome::VersionInfo version_info;
-  return scoped_ptr<syncer::HttpPostProviderFactory>(
-      new syncer::HttpBridgeFactory(
-          getter.get(),
-          DeviceInfo::MakeUserAgentForSyncApi(version_info),
-          update_callback));
-}
-
-}  // namespace
-
 void SyncBackendHost::Initialize(
     SyncFrontend* frontend,
     scoped_ptr<base::Thread> sync_thread,
     const syncer::WeakHandle<syncer::JsEventHandler>& event_handler,
     const GURL& sync_service_url,
     const SyncCredentials& credentials,
     bool delete_sync_data_folder,
     scoped_ptr<syncer::SyncManagerFactory> sync_manager_factory,
     scoped_ptr<syncer::UnrecoverableErrorHandler> unrecoverable_error_handler,
     syncer::ReportUnrecoverableErrorFunction
@@ skipping 29 matching lines @@
   initialization_state_ = CREATING_SYNC_MANAGER;
 
   scoped_ptr<DoInitializeOptions> init_opts(new DoInitializeOptions(
       registrar_->sync_thread()->message_loop(),
       registrar_.get(),
       routing_info,
       workers,
       extensions_activity_monitor_.GetExtensionsActivity(),
       event_handler,
       sync_service_url,
-      base::Bind(&MakeHttpBridgeFactory,
-                 make_scoped_refptr(profile_->GetRequestContext()),
-                 NetworkTimeTracker::BuildNotifierUpdateCallback()),
+      scoped_ptr<syncer::HttpPostProviderFactory>(
+          new syncer::HttpBridgeFactory(
+              make_scoped_refptr(profile_->GetRequestContext()),
+              NetworkTimeTracker::BuildNotifierUpdateCallback(),
+              factory_cancelation_signal_.get())),
       credentials,
       invalidator_->GetInvalidatorClientId(),
       sync_manager_factory.Pass(),
       delete_sync_data_folder,
       sync_prefs_->GetEncryptionBootstrapToken(),
       sync_prefs_->GetKeystoreEncryptionBootstrapToken(),
       scoped_ptr<InternalComponentsFactory>(
           new InternalComponentsFactoryImpl(factory_switches)).Pass(),
       unrecoverable_error_handler.Pass(),
       report_unrecoverable_error_function,
-      !cl->HasSwitch(switches::kSyncDisableOAuth2Token)));
+      !cl->HasSwitch(switches::kSyncDisableOAuth2Token),
+      scm_cancelation_signal_.get()));
   InitCore(init_opts.Pass());
 }
 
 void SyncBackendHost::UpdateCredentials(const SyncCredentials& credentials) {
   DCHECK(registrar_->sync_thread()->IsRunning());
   registrar_->sync_thread()->message_loop()->PostTask(FROM_HERE,
       base::Bind(&SyncBackendHost::Core::DoUpdateCredentials,
                  core_.get(),
                  credentials));
 }
@@ skipping 70 matching lines @@
   // clicks OK, immediately reopens the advanced settings dialog, and gets an
   // unnecessary prompt for a passphrase.
   // Note: It is not guaranteed that the passphrase will be accepted by the
   // syncer thread, since we could receive a new nigori node while the task is
   // pending. This scenario is a valid race, and SetDecryptionPassphrase can
   // trigger a new OnPassphraseRequired if it needs to.
   NotifyPassphraseAccepted();
   return true;
 }
 
-void SyncBackendHost::StopSyncManagerForShutdown() {
-  DCHECK_GT(initialization_state_, NOT_ATTEMPTED);
-  if (initialization_state_ == CREATING_SYNC_MANAGER) {
-    // We post here to implicitly wait for the SyncManager to be created,
-    // if needed.  We have to wait, since we need to shutdown immediately,
-    // and we need to tell the SyncManager so it can abort any activity
-    // (net I/O, data application).
-    DCHECK(registrar_->sync_thread()->IsRunning());
-    registrar_->sync_thread()->message_loop()->PostTask(FROM_HERE,
-        base::Bind(&SyncBackendHost::Core::DoStopSyncManagerForShutdown,
-                   core_.get()));
-  } else {
-    core_->DoStopSyncManagerForShutdown();
-  }
-}
-
 void SyncBackendHost::StopSyncingForShutdown() {
   DCHECK_EQ(base::MessageLoop::current(), frontend_loop_);
+  DCHECK_GT(initialization_state_, NOT_ATTEMPTED);
 
   // Immediately stop sending messages to the frontend.
   frontend_ = NULL;
 
   // Stop listening for and forwarding locally-triggered sync refresh requests.
   notification_registrar_.RemoveAll();
 
   DCHECK(registrar_->sync_thread()->IsRunning());
 
   registrar_->RequestWorkerStopOnUIThread();
 
-  StopSyncManagerForShutdown();
+  // This will cut short any blocking network tasks, cut short any in-progress
+  // sync cycles, and prevent the creation of new blocking network tasks and new
+  // sync cycles.  If there was an in-progress network request, it would have
+  // had a reference to the RequestContextGetter.  This reference will be
+  // dropped by the time this function returns.
+  //
+  // It is safe to call this even if Sync's backend classes have not been
+  // initialized yet.  Those classes will receive the message when the sync
+  // thread finally getes around to constructing them.
+  scm_cancelation_signal_->RequestStop();
+
+  // This will drop the HttpBridgeFactory's reference to the
+  // RequestContextGetter.  Once this has been called, the HttpBridgeFactory can
+  // no longer be used to create new HttpBridge instances.  We can get away with
+  // this because the scm_cancelation_signal_ has already been signalled, which
+  // guarantees that the ServerConnectionManager will no longer attempt to
+  // create new connections.
+  factory_cancelation_signal_->RequestStop();
 }
 
 scoped_ptr<base::Thread> SyncBackendHost::Shutdown(ShutdownOption option) {
   // StopSyncingForShutdown() (which nulls out |frontend_|) should be
   // called first.
   DCHECK(!frontend_);
   DCHECK(registrar_->sync_thread()->IsRunning());
 
   bool sync_disabled = (option == DISABLE_AND_CLAIM_THREAD);
   bool sync_thread_claimed =
       (option == DISABLE_AND_CLAIM_THREAD || option == STOP_AND_CLAIM_THREAD);
 
   if (invalidation_handler_registered_) {
     if (sync_disabled) {
       UnregisterInvalidationIds();
     }
     invalidator_->UnregisterInvalidationHandler(this);
     invalidator_ = NULL;
   }
   invalidation_handler_registered_ = false;
 
   // Shut down and destroy sync manager.
+  //
+  // We're about to drop our reference to the |core_|.  The only thing keeping
+  // it alive will be the reference that lives inside this closure we're about
+  // to post to the sync thread.  If this closure doesn't run right away, it's
+  // possible that our Core will outlive us.
+  //
+  // This is a problem for the CancelationSignals, which must outlive the
+  // classes that reference them.  In this case, it is safe to destroy them only
+  // after the call to DoDestroySyncManager completes.  The SyncBackendHost
+  // might not live that long, but this closure will.  We pass ownership of the
+  // CancelationSignals to this closure to keep them alive.
   registrar_->sync_thread()->message_loop()->PostTask(
       FROM_HERE,
       base::Bind(&SyncBackendHost::Core::DoShutdown,
-                 core_.get(), sync_disabled));
+                 core_.get(), sync_disabled,
+                 base::Passed(&factory_cancelation_signal_),
+                 base::Passed(&scm_cancelation_signal_)));
   core_ = NULL;
 
   // Worker cleanup.
   SyncBackendRegistrar* detached_registrar = registrar_.release();
   detached_registrar->sync_thread()->message_loop()->PostTask(
       FROM_HERE,
       base::Bind(&SyncBackendRegistrar::Shutdown,
                  base::Unretained(detached_registrar)));
 
   js_backend_.Reset();
@@ skipping 304 matching lines @@
 }
 
 SyncBackendHost::DoInitializeOptions::DoInitializeOptions(
     base::MessageLoop* sync_loop,
     SyncBackendRegistrar* registrar,
     const syncer::ModelSafeRoutingInfo& routing_info,
     const std::vector<syncer::ModelSafeWorker*>& workers,
     const scoped_refptr<syncer::ExtensionsActivity>& extensions_activity,
     const syncer::WeakHandle<syncer::JsEventHandler>& event_handler,
     const GURL& service_url,
-    MakeHttpBridgeFactoryFn make_http_bridge_factory_fn,
+    scoped_ptr<syncer::HttpPostProviderFactory> http_bridge_factory,
     const syncer::SyncCredentials& credentials,
     const std::string& invalidator_client_id,
     scoped_ptr<syncer::SyncManagerFactory> sync_manager_factory,
     bool delete_sync_data_folder,
     const std::string& restored_key_for_bootstrapping,
     const std::string& restored_keystore_key_for_bootstrapping,
     scoped_ptr<InternalComponentsFactory> internal_components_factory,
     scoped_ptr<syncer::UnrecoverableErrorHandler> unrecoverable_error_handler,
     syncer::ReportUnrecoverableErrorFunction
         report_unrecoverable_error_function,
-    bool use_oauth2_token)
+    bool use_oauth2_token,
+    syncer::CancelationSignal* const scm_cancelation_signal)
     : sync_loop(sync_loop),
       registrar(registrar),
       routing_info(routing_info),
       workers(workers),
       extensions_activity(extensions_activity),
       event_handler(event_handler),
       service_url(service_url),
-      make_http_bridge_factory_fn(make_http_bridge_factory_fn),
+      http_bridge_factory(http_bridge_factory.Pass()),
       credentials(credentials),
       invalidator_client_id(invalidator_client_id),
       sync_manager_factory(sync_manager_factory.Pass()),
       delete_sync_data_folder(delete_sync_data_folder),
       restored_key_for_bootstrapping(restored_key_for_bootstrapping),
       restored_keystore_key_for_bootstrapping(
           restored_keystore_key_for_bootstrapping),
       internal_components_factory(internal_components_factory.Pass()),
       unrecoverable_error_handler(unrecoverable_error_handler.Pass()),
       report_unrecoverable_error_function(
           report_unrecoverable_error_function),
-      use_oauth2_token(use_oauth2_token) {
+      use_oauth2_token(use_oauth2_token),
+      scm_cancelation_signal(scm_cancelation_signal) {
 }
 
 SyncBackendHost::DoInitializeOptions::~DoInitializeOptions() {}
 
 SyncBackendHost::Core::Core(const std::string& name,
                             const base::FilePath& sync_data_folder_path,
                             const base::WeakPtr<SyncBackendHost>& backend)
     : name_(name),
       sync_data_folder_path_(sync_data_folder_path),
       host_(backend),
@@ skipping 209 matching lines @@
   DCHECK_EQ(base::MessageLoop::current(), sync_loop_);
   sync_manager_->OnIncomingInvalidation(invalidation_map);
 }
 
 void SyncBackendHost::Core::DoInitialize(
     scoped_ptr<DoInitializeOptions> options) {
   DCHECK(!sync_loop_);
   sync_loop_ = options->sync_loop;
   DCHECK(sync_loop_);
 
+  // Finish initializing the HttpBridgeFactory.  We do this here because
+  // building the user agent may block on some platforms.
+  chrome::VersionInfo version_info;
+  options->http_bridge_factory->Init(
+      DeviceInfo::MakeUserAgentForSyncApi(version_info));
+
   // Blow away the partial or corrupt sync data folder before doing any more
   // initialization, if necessary.
   if (options->delete_sync_data_folder) {
     DeleteSyncDataFolder();
   }
 
   // Make sure that the directory exists before initializing the backend.
   // If it already exists, this will do no harm.
   if (!file_util::CreateDirectory(sync_data_folder_path_)) {
     DLOG(FATAL) << "Sync Data directory creation failed.";
   }
 
   DCHECK(!registrar_);
   registrar_ = options->registrar;
   DCHECK(registrar_);
 
   sync_manager_ = options->sync_manager_factory->CreateSyncManager(name_);
   sync_manager_->AddObserver(this);
   sync_manager_->Init(sync_data_folder_path_,
                       options->event_handler,
                       options->service_url.host() + options->service_url.path(),
                       options->service_url.EffectiveIntPort(),
                       options->service_url.SchemeIsSecure(),
-                      options->make_http_bridge_factory_fn.Run().Pass(),
+                      options->http_bridge_factory.Pass(),
                       options->workers,
                       options->extensions_activity,
                       options->registrar /* as SyncManager::ChangeDelegate */,
                       options->credentials,
                       options->invalidator_client_id,
                       options->restored_key_for_bootstrapping,
                       options->restored_keystore_key_for_bootstrapping,
                       options->internal_components_factory.get(),
                       &encryptor_,
                       options->unrecoverable_error_handler.Pass(),
                       options->report_unrecoverable_error_function,
-                      options->use_oauth2_token);
+                      options->use_oauth2_token,
+                      options->scm_cancelation_signal);
 
   // |sync_manager_| may end up being NULL here in tests (in
   // synchronous initialization mode).
   //
   // TODO(akalin): Fix this behavior (see http://crbug.com/140354).
   if (sync_manager_) {
     // Now check the command line to see if we need to simulate an
     // unrecoverable error for testing purpose. Note the error is thrown
     // only if the initialization succeeded. Also it makes sense to use this
     // flag only when restarting the browser with an account already setup. If
@@ skipping 89 matching lines @@
   DCHECK_EQ(base::MessageLoop::current(), sync_loop_);
   sync_manager_->GetEncryptionHandler()->SetDecryptionPassphrase(
       passphrase);
 }
 
 void SyncBackendHost::Core::DoEnableEncryptEverything() {
   DCHECK_EQ(base::MessageLoop::current(), sync_loop_);
   sync_manager_->GetEncryptionHandler()->EnableEncryptEverything();
 }
 
-void SyncBackendHost::Core::DoStopSyncManagerForShutdown() {
-  if (sync_manager_)
-    sync_manager_->StopSyncingForShutdown();
-}
+void SyncBackendHost::Core::DoShutdown(
+    bool sync_disabled,
+    scoped_ptr<syncer::CancelationSignal> signal1,
+    scoped_ptr<syncer::CancelationSignal> signal2) {
+  DCHECK_EQ(base::MessageLoop::current(), sync_loop_);
 
-void SyncBackendHost::Core::DoShutdown(bool sync_disabled) {
-  DCHECK_EQ(base::MessageLoop::current(), sync_loop_);
   // It's safe to do this even if the type was never activated.
   registrar_->DeactivateDataType(syncer::DEVICE_INFO);
   synced_device_tracker_.reset();
 
   DoDestroySyncManager();
 
   registrar_ = NULL;
 
   if (sync_disabled)
     DeleteSyncDataFolder();
@@ skipping 307 matching lines @@
 
 base::MessageLoop* SyncBackendHost::GetSyncLoopForTesting() {
   return registrar_->sync_thread()->message_loop();
 }
 
 #undef SDVLOG
 
 #undef SLOG
 
 }  // namespace browser_sync