sync: Gracefully handle very early shutdown

sync/engine/net/server_connection_manager.cc

Index: sync/engine/net/server_connection_manager.cc
diff --git a/sync/engine/net/server_connection_manager.cc b/sync/engine/net/server_connection_manager.cc
index 0076543374fcdcedae2a86bfe62f2a1491ecb5e0..1fce7c6e357e015e0a9866a33fbbdeb6332644d2 100644
--- a/sync/engine/net/server_connection_manager.cc
+++ b/sync/engine/net/server_connection_manager.cc
@@ -16,6 +16,7 @@
 #include "net/http/http_status_code.h"
 #include "sync/engine/net/url_translator.h"
 #include "sync/engine/syncer.h"
+#include "sync/internal_api/public/base/cancellation_signal.h"
 #include "sync/protocol/sync.pb.h"
 #include "sync/syncable/directory.h"
 #include "url/gurl.h"
@@ -114,13 +115,32 @@ bool ServerConnectionManager::Connection::ReadDownloadResponse(
 }
 
 ServerConnectionManager::ScopedConnectionHelper::ScopedConnectionHelper(
-    ServerConnectionManager* manager, Connection* connection)
-    : manager_(manager), connection_(connection) {}
+    CancellationSignal* signaller, Connection* connection)
+    : cancellation_signal_(signaller), connection_(connection) {
+  if (!cancellation_signal_->TryRegisterHandler(this)) {
+    connection_.reset();
+  }
+}
+
+// This function may be called from another thread.
+void ServerConnectionManager::ScopedConnectionHelper::OnStopRequested() {
+  DCHECK(connection_);
+  connection_->Abort();
+}
 
 ServerConnectionManager::ScopedConnectionHelper::~ScopedConnectionHelper() {
-  if (connection_)
-    manager_->OnConnectionDestroyed(connection_.get());
-  connection_.reset();
+  // We should be registered iff connection_.get() != NULL.
+  if (connection_) {
+    // It is important that this be called before this destructor completes.
+    // Until the unregistration is complete, it's possible that the virtual
+    // OnStopRequested() function may be called from a different thread.  We
+    // need to unregister it before destruction modifies our vptr.
+    cancellation_signal_->UnregisterHandler(this);
+  }
+
+  if (connection_) {

[akalin, 2013/09/05 15:35:28]

i don't think this clause is needed

[rlarocque, 2013/09/05 22:37:06]

You're right.  Fixed.

+    connection_.reset();
+  }
 }
 
 ServerConnectionManager::Connection*
@@ -177,43 +197,20 @@ ServerConnectionManager::ServerConnectionManager(
     const string& server,
     int port,
     bool use_ssl,
-    bool use_oauth2_token)
+    bool use_oauth2_token,
+    CancellationSignal* cancellation_signal)
     : sync_server_(server),
       sync_server_port_(port),
       use_ssl_(use_ssl),
       use_oauth2_token_(use_oauth2_token),
       proto_sync_path_(kSyncServerSyncPath),
       server_status_(HttpResponse::NONE),
-      terminated_(false),
-      active_connection_(NULL) {
+      cancellation_signal_(cancellation_signal) {
 }
 
 ServerConnectionManager::~ServerConnectionManager() {
 }
 
-ServerConnectionManager::Connection*
-ServerConnectionManager::MakeActiveConnection() {
-  base::AutoLock lock(terminate_connection_lock_);
-  DCHECK(!active_connection_);
-  if (terminated_)
-    return NULL;
-
-  active_connection_ = MakeConnection();
-  return active_connection_;
-}
-
-void ServerConnectionManager::OnConnectionDestroyed(Connection* connection) {
-  DCHECK(connection);
-  base::AutoLock lock(terminate_connection_lock_);
-  // |active_connection_| can be NULL already if it was aborted. Also,
-  // it can legitimately be a different Connection object if a new Connection
-  // was created after a previous one was Aborted and destroyed.
-  if (active_connection_ != connection)
-    return;
-
-  active_connection_ = NULL;
-}
-
 bool ServerConnectionManager::SetAuthToken(const std::string& auth_token) {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (previously_invalidated_token != auth_token) {
@@ -278,7 +275,7 @@ bool ServerConnectionManager::PostBufferToPath(PostBufferParams* params,
 
   // When our connection object falls out of scope, it clears itself from
   // active_connection_.
-  ScopedConnectionHelper post(this, MakeActiveConnection());
+  ScopedConnectionHelper post(cancellation_signal_, MakeConnection());

[akalin, 2013/09/05 16:06:35]

ideally, MakeConnection() would return a scoped_ptr<> too

[rlarocque, 2013/09/05 22:37:06]

Done.

   if (!post.get()) {
     params->response.server_status = HttpResponse::CONNECTION_UNAVAILABLE;
     return false;
@@ -348,17 +345,6 @@ ServerConnectionManager::Connection* ServerConnectionManager::MakeConnection()
   return NULL;  // For testing.
 }
 
-void ServerConnectionManager::TerminateAllIO() {
-  base::AutoLock lock(terminate_connection_lock_);
-  terminated_ = true;
-  if (active_connection_)
-    active_connection_->Abort();
-
-  // Sever our ties to this connection object. Note that it still may exist,
-  // since we don't own it, but it has been neutered.
-  active_connection_ = NULL;
-}
-
 std::ostream& operator << (std::ostream& s, const struct HttpResponse& hr) {
   s << " Response Code (bogus on error): " << hr.response_code;
   s << " Content-Length (bogus on error): " << hr.content_length;