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 26 matching lines @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/common/content_client.h"
 #include "google_apis/gaia/gaia_constants.h"
 #include "jingle/notifier/base/notification_method.h"
 #include "jingle/notifier/base/notifier_options.h"
 #include "net/base/host_port_pair.h"
 #include "net/url_request/url_request_context_getter.h"
+#include "sync/internal_api/public/base/cancelation_signal.h"
 #include "sync/internal_api/public/base_transaction.h"
 #include "sync/internal_api/public/engine/model_safe_worker.h"
 #include "sync/internal_api/public/http_bridge.h"
 #include "sync/internal_api/public/internal_components_factory_impl.h"
 #include "sync/internal_api/public/read_transaction.h"
 #include "sync/internal_api/public/sync_manager_factory.h"
 #include "sync/internal_api/public/util/experiments.h"
 #include "sync/internal_api/public/util/sync_string_conversions.h"
 #include "sync/protocol/encryption.pb.h"
 #include "sync/protocol/sync.pb.h"
@@ skipping 134 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 ShutdownOnUIThread() 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 ShutdownOnUIThread();
   void DoShutdown(bool sync_disabled);
   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,
@@ skipping 13 matching lines @@
 
   SyncedDeviceTracker* synced_device_tracker() {
     return synced_device_tracker_.get();
   }
 
   // Delete the sync data folder to cleanup backend data.  Happens the first
   // time sync is enabled for a user (to prevent accidentally reusing old
   // sync databases), as well as shutdown when you're no longer syncing.
   void DeleteSyncDataFolder();
 
+  // We expose this member because it's required in the construction of the
+  // HttpBridgeFactory.
+  syncer::CancelationSignal* GetFactoryCancelationSignal() {
+    return &factory_cancelation_signal_;

[tim (not reviewing), 2013/09/16 23:01:31]

|factory| in the name might not be the best choice, at first I thought this was
regarding the ComponentsFactory.  network_cancelation_signal or
http_cancelation_signal?

[rlarocque, 2013/09/17 00:03:22]

What about the UrlRequestContextGetter?  That object is the real reason we need
all this plumbing.  

request_context_cancelation_signal_?

[tim (not reviewing), 2013/09/17 21:56:41]

Hm.  That's more confusing I think. 
http_bridge_construction_signal_.RequestStop seems better. 

Also can you explain why we need two?  It's not obvious from looking at the
code.  If we just need one I'd probably call it syncer_cancelation_signal_ or
syncer_stop_requested_signal.  I think those are better names for the second one
(currently called scm_cancelation_signal) because they're more clear in
describing the desired outcome and because abbreviations (like scm) are bad in
code variable names (as per style guide, bad for grepping, etc).

[rlarocque, 2013/09/17 22:46:02]

I don't think http_bridge_construction_signal_ is right, either.  This signal
won't prevent HttpBridge construction.  Not gracefully, anyway.  We would
probably crash if we tried to construct an HttpBridge after it had been
signalled.
We technically don't need two, but we would need to use some very subtle tricks
to get away with using just one.

If we attempt to create a new HttpBridge after the factory_cancelation_signal_
is signalled, the HttpBridgeFactory will try to access a non-existent request
context.  The solution to that problem is to ensure that the
ServerConnectionManager is shut down first.

We could do this with just one signal and some sort of observer list that
guaranteed to call its observers in a specific order, but I think it's clearer
to use two separate signals.

I touched on this a bit in the comments around line 1165.  I'll add a few more
comments here.

[tim (not reviewing), 2013/09/18 00:37:56]

I see.  Logically we're just trying to sever ties with the network stack, but
it's awkward from a naming perspective. I guess 'RequestStop' is part of the
problem since signaling this isn't really "stopping" anything.  Except maybe
'hold_request_context_reference_signal_', I just wish we didn't have to be so
low-level...  But maybe that's the best option at this point?
That makes sense.  Two definitely seems like the right approach.

[rlarocque, 2013/09/18 00:45:09]

That's not bad.  Maybe we should make their names more verb-like:
- release_request_context_signal_
- stop_syncing_signal_

+  }
+
  private:
   friend class base::RefCountedThreadSafe<SyncBackendHost::Core>;
   friend class SyncBackendHostForProfileSyncTest;
 
   virtual ~Core();
 
   // Invoked when initialization of syncapi is complete and we can start
   // our timer.
   // This must be called from the thread on which SaveChanges is intended to
   // be run on; the host's |registrar_->sync_thread()|.
@@ skipping 30 matching lines @@
   ChromeEncryptor encryptor_;
 
   // A special ChangeProcessor that tracks the DEVICE_INFO type for us.
   scoped_ptr<SyncedDeviceTracker> synced_device_tracker_;
 
   // The top-level syncapi entry point.  Lives on the sync thread.
   scoped_ptr<syncer::SyncManager> sync_manager_;
 
   base::WeakPtrFactory<Core> weak_ptr_factory_;
 
+  // These signals allow us to send requests to shut down the HttpBridgeFactory
+  // and ServerConnectionManager without having to wait for those classes to
+  // finish initializing first.
+  //
+  // See comments in Core::ShutdownOnUIThread() for more details.
+  syncer::CancelationSignal factory_cancelation_signal_;
+  syncer::CancelationSignal scm_cancelation_signal_;
+
   DISALLOW_COPY_AND_ASSIGN(Core);
 };
 
 SyncBackendHost::SyncBackendHost(
     const std::string& name,
     Profile* profile,
     const base::WeakPtr<SyncPrefs>& sync_prefs)
     : weak_ptr_factory_(this),
       frontend_loop_(base::MessageLoop::current()),
       profile_(profile),
@@ skipping 18 matching lines @@
       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(),
+              core_->GetFactoryCancelationSignal())),
       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,
@@ skipping 81 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();
+  core_->ShutdownOnUIThread();
 }
 
 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 =
@@ skipping 330 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)
     : 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(
@@ skipping 221 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,
+                      &scm_cancelation_signal_);

[tim (not reviewing), 2013/09/16 23:01:31]

Hm, don't we pass this to the syncer? (yet it's called "scm cancelation
signal")?

[rlarocque, 2013/09/17 00:03:22]

Syncer and SCM, yes.  This signal is the descendant of the one CancelationSignal
that existed before I rebased on top of Haitao's patch.  

The way I think of it is that the SCM is the primary user of this signal, and
the Syncer just happens to check it every now and then, too.  The Syncer's early
exit is best-effort, so it would be OK to use either of the two signals.  This
one gets posted first, so it's slightly better to use it.   

Can you think of a better name for it?  Would scm_and_syncer_cancelation_signal
be an improvement?

 
   // |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::ShutdownOnUIThread() {
+  // 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();
 }
 
 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