sync: Gracefully handle early shutdown

Introduce a new object to communicate cross-thread cancellation signals.
This new object, the CancellationSignal, is protected by a lock. It
allows the receiving thread to query whether or not a stop has been
requested. It also allows the receiving thread to safely register a
cross-thread callback to be invoked immediately when a stop is
requested.

We use two instances of this class to ensure we meet all the
requirements for a safe and fast sync backend shutdown.

The first instance is used with the HttpBridgeFactory to allow the UI
thread to force it to drop all refereces to its RequestContextGetter
immediately.  This is an important part of our plan to ensure that all
references to that object are released before
ProfileSyncService::Shutdown() returns, which is necessary to avoid
racy crashes at shutdown.  (See crbug.com/236451)

The second instance is used with the ServerConnectionManager and the
Syncer.  Once signalled, it ensures that any active connections are
released (possibly decrementing another ref to the
RequestContextGetter), that any blocking I/O is aborted, and that no
more connections will be instantiated.  It's important to prevent the
creation of more connections because the HttpBridgeFactory might trigger
a crash if we asked it to create another connection after it had been
shut down.

The syncer's interaction with the second cancelation signal is more
passive.  It does not execute any callbacks when the signal is sent.
Instead, it queries the signal as it performs sync cycles, and will cut
short any existing sync cycle if it notices the signal has been sent.

Finally, this CL includes one important change to the initialization of
the HttpBridgeFactory.  In order to properly register with the
cancellation signal while still allowing the creation of the user agent
to occur on the sync thread, its initialization had to be split into two
parts.  This class now has an Init() method in addition to its
constructor.

BUG=236451

Review URL: https://chromiumcodereview.appspot.com/23717047

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@224014 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 65 insertions, 63 deletions

diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc
index 2f91728..cf765bd 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -44,6 +44,7 @@
 #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"
@@ -198,11 +199,11 @@ class SyncBackendHost::Core
   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();
 
@@ -236,6 +237,12 @@ class SyncBackendHost::Core
   // 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* GetRequestContextCancelationSignal() {
+    return &release_request_context_signal_;
+  }
+
  private:
   friend class base::RefCountedThreadSafe<SyncBackendHost::Core>;
   friend class SyncBackendHostForProfileSyncTest;
@@ -286,6 +293,14 @@ class SyncBackendHost::Core
 
   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 release_request_context_signal_;
+  syncer::CancelationSignal stop_syncing_signal_;
+
   DISALLOW_COPY_AND_ASSIGN(Core);
 };
 
@@ -324,21 +339,6 @@ SyncBackendHost::~SyncBackendHost() {
   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,
@@ -388,9 +388,11 @@ void SyncBackendHost::Initialize(
       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_->GetRequestContextCancelationSignal())),
       credentials,
       invalidator_->GetInvalidatorClientId(),
       sync_manager_factory.Pass(),
@@ -492,24 +494,9 @@ bool SyncBackendHost::SetDecryptionPassphrase(const std::string& passphrase) {
   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;
@@ -521,7 +508,7 @@ void SyncBackendHost::StopSyncingForShutdown() {
 
   registrar_->RequestWorkerStopOnUIThread();
 
-  StopSyncManagerForShutdown();
+  core_->ShutdownOnUIThread();
 }
 
 scoped_ptr<base::Thread> SyncBackendHost::Shutdown(ShutdownOption option) {
@@ -872,7 +859,7 @@ SyncBackendHost::DoInitializeOptions::DoInitializeOptions(
     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,
@@ -891,7 +878,7 @@ SyncBackendHost::DoInitializeOptions::DoInitializeOptions(
       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()),
@@ -1133,6 +1120,12 @@ void SyncBackendHost::Core::DoInitialize(
   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) {
@@ -1156,7 +1149,7 @@ void SyncBackendHost::Core::DoInitialize(
                       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 */,
@@ -1168,7 +1161,8 @@ void SyncBackendHost::Core::DoInitialize(
                       &encryptor_,
                       options->unrecoverable_error_handler.Pass(),
                       options->report_unrecoverable_error_function,
-                      options->use_oauth2_token);
+                      options->use_oauth2_token,
+                      &stop_syncing_signal_);
 
   // |sync_manager_| may end up being NULL here in tests (in
   // synchronous initialization mode).
@@ -1278,13 +1272,30 @@ void SyncBackendHost::Core::DoEnableEncryptEverything() {
   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.
+  stop_syncing_signal_.Signal();
+
+  // 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 stop_syncing_signal_ has already been signalled, which
+  // guarantees that the ServerConnectionManager will no longer attempt to
+  // create new connections.
+  release_request_context_signal_.Signal();
 }
 
 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();
diff --git a/chrome/browser/sync/glue/sync_backend_host.h b/chrome/browser/sync/glue/sync_backend_host.h
index 113ad42..3a4e82a 100644
--- a/chrome/browser/sync/glue/sync_backend_host.h
+++ b/chrome/browser/sync/glue/sync_backend_host.h
@@ -218,10 +218,9 @@ class SyncBackendHost
   bool SetDecryptionPassphrase(const std::string& passphrase)
       WARN_UNUSED_RESULT;
 
-  // Called on |frontend_loop_| to kick off shutdown procedure. After this, no
-  // further sync activity will occur with the sync server and no further
-  // change applications will occur from changes already downloaded.
-  // Furthermore, no notifications will be sent to any invalidation handler.
+  // Called on |frontend_loop_| to kick off shutdown procedure.  Attempts to cut
+  // short any long-lived or blocking sync thread tasks so that the shutdown on
+  // sync thread task that we're about to post won't have to wait very long.
   virtual void StopSyncingForShutdown();
 
   // Called on |frontend_loop_| to kick off shutdown.
@@ -313,9 +312,6 @@ class SyncBackendHost
   // TODO(akalin): Figure out a better way for tests to hook into
   // SyncBackendHost.
 
-  typedef base::Callback<scoped_ptr<syncer::HttpPostProviderFactory>(void)>
-      MakeHttpBridgeFactoryFn;
-
   // Utility struct for holding initialization options.
   struct DoInitializeOptions {
     DoInitializeOptions(
@@ -326,7 +322,7 @@ class SyncBackendHost
         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,
@@ -350,7 +346,7 @@ class SyncBackendHost
     syncer::WeakHandle<syncer::JsEventHandler> event_handler;
     GURL service_url;
     // Overridden by tests.
-    MakeHttpBridgeFactoryFn make_http_bridge_factory_fn;
+    scoped_ptr<syncer::HttpPostProviderFactory> http_bridge_factory;
     syncer::SyncCredentials credentials;
     const std::string invalidator_client_id;
     scoped_ptr<syncer::SyncManagerFactory> sync_manager_factory;
@@ -523,10 +519,6 @@ class SyncBackendHost
   virtual void OnIncomingInvalidation(
       const syncer::ObjectIdInvalidationMap& invalidation_map) OVERRIDE;
 
-  // Handles stopping the core's SyncManager, accounting for whether
-  // initialization is done yet.
-  void StopSyncManagerForShutdown();
-
   base::WeakPtrFactory<SyncBackendHost> weak_ptr_factory_;
 
   content::NotificationRegistrar notification_registrar_;
diff --git a/chrome/browser/sync/test/test_http_bridge_factory.cc b/chrome/browser/sync/test/test_http_bridge_factory.cc
index 522fb6f..23b2c0e 100644
--- a/chrome/browser/sync/test/test_http_bridge_factory.cc
+++ b/chrome/browser/sync/test/test_http_bridge_factory.cc
@@ -31,6 +31,8 @@ TestHttpBridgeFactory::TestHttpBridgeFactory() {}
 
 TestHttpBridgeFactory::~TestHttpBridgeFactory() {}
 
+void TestHttpBridgeFactory::Init(const std::string& user_agent) {}
+
 syncer::HttpPostProviderInterface* TestHttpBridgeFactory::Create() {
   return new TestHttpBridge();
 }
@@ -39,6 +41,4 @@ void TestHttpBridgeFactory::Destroy(syncer::HttpPostProviderInterface* http) {
   delete static_cast<TestHttpBridge*>(http);
 }
 
-void TestHttpBridgeFactory::Shutdown() {}
-
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/test/test_http_bridge_factory.h b/chrome/browser/sync/test/test_http_bridge_factory.h
index ca29cb4..145d146 100644
--- a/chrome/browser/sync/test/test_http_bridge_factory.h
+++ b/chrome/browser/sync/test/test_http_bridge_factory.h
@@ -42,9 +42,9 @@ class TestHttpBridgeFactory : public syncer::HttpPostProviderFactory {
   virtual ~TestHttpBridgeFactory();
 
   // syncer::HttpPostProviderFactory:
+  virtual void Init(const std::string& user_agent) OVERRIDE;
   virtual syncer::HttpPostProviderInterface* Create() OVERRIDE;
   virtual void Destroy(syncer::HttpPostProviderInterface* http) OVERRIDE;
-  virtual void Shutdown() OVERRIDE;
 };
 
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/test_profile_sync_service.cc b/chrome/browser/sync/test_profile_sync_service.cc
index 91d3941..2edd436 100644
--- a/chrome/browser/sync/test_profile_sync_service.cc
+++ b/chrome/browser/sync/test_profile_sync_service.cc
@@ -64,8 +64,7 @@ scoped_ptr<syncer::HttpPostProviderFactory> MakeTestHttpBridgeFactory() {
 
 void SyncBackendHostForProfileSyncTest::InitCore(
     scoped_ptr<DoInitializeOptions> options) {
-  options->make_http_bridge_factory_fn =
-      base::Bind(&MakeTestHttpBridgeFactory);
+  options->http_bridge_factory = MakeTestHttpBridgeFactory();
   options->credentials.email = "testuser@gmail.com";
   options->credentials.sync_token = "token";
   options->restored_key_for_bootstrapping = "";