Retry: sync: Gracefully handle early shutdown

chrome/browser/sync/glue/sync_backend_host.cc

Index: chrome/browser/sync/glue/sync_backend_host.cc
diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc
index 2f91728d69096ca2a55838b0e751a67550093421..c5a6572178e63a821b57650b7c49673bf6501974 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* 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;
@@ -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 factory_cancelation_signal_;
+  syncer::CancelationSignal scm_cancelation_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_->GetFactoryCancelationSignal())),
       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,
+                      &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).
@@ -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.
+  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();