Retry: sync: Gracefully handle early shutdown

chrome/browser/sync/glue/sync_backend_host.h

Index: chrome/browser/sync/glue/sync_backend_host.h
diff --git a/chrome/browser/sync/glue/sync_backend_host.h b/chrome/browser/sync/glue/sync_backend_host.h
index 113ad42e0dd4f1fae9f9066f836547e6ced0b135..4ccfa4afdcdb9c7354b7c4749d840b8b9ed1dc8b 100644
--- a/chrome/browser/sync/glue/sync_backend_host.h
+++ b/chrome/browser/sync/glue/sync_backend_host.h
@@ -20,6 +20,7 @@
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "google_apis/gaia/google_service_auth_error.h"
+#include "sync/internal_api/public/base/cancelation_signal.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/internal_api/public/configure_reason.h"
 #include "sync/internal_api/public/engine/model_safe_worker.h"
@@ -218,10 +219,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 +313,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 +323,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,
@@ -339,7 +336,8 @@ class SyncBackendHost
             unrecoverable_error_handler,
         syncer::ReportUnrecoverableErrorFunction
             report_unrecoverable_error_function,
-        bool use_oauth2_token);
+        bool use_oauth2_token,
+        syncer::CancelationSignal* scm_cancelation_signal);
     ~DoInitializeOptions();
 
     base::MessageLoop* sync_loop;
@@ -350,7 +348,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;
@@ -363,6 +361,7 @@ class SyncBackendHost
     syncer::ReportUnrecoverableErrorFunction
         report_unrecoverable_error_function;
     bool use_oauth2_token;
+    syncer::CancelationSignal* const scm_cancelation_signal;
   };
 
   // Allows tests to perform alternate core initialization work.
@@ -523,10 +522,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_;
@@ -589,6 +584,14 @@ class SyncBackendHost
   invalidation::InvalidationService* invalidator_;
   bool invalidation_handler_registered_;
 
+  // These signals are (mostly) owned by the UI thread.  They 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 StopSyncingForShutdown() and Shutdown() for more details.
+  scoped_ptr<syncer::CancelationSignal> factory_cancelation_signal_;

[haitaol1, 2013/09/13 18:43:21]

Maybe move these into SyncBackendRegistrar, which is the last to be destroyed.

[rlarocque, 2013/09/13 20:27:17]

That should work.  However, there is a comment that describes
SyncBackendRegistrar as follows:

// A class that keep track of the workers, change processors, and
// routing info for the enabled sync types, and also routes change
// events to the right processors.

Based on that description, it's hard to justify why such a class should own
these cancelation signals.

Maybe there should be a separate object that owns these signals.  But I don't
think that class should be the one called "SyncBackendRegistrar".

+  scoped_ptr<syncer::CancelationSignal> scm_cancelation_signal_;
+
   DISALLOW_COPY_AND_ASSIGN(SyncBackendHost);
 };