sync: take 2 at aborting active HTTP requests.

Original issue is codereview.chromium.org/7792022.
Problem was a race between Connection destruction and TerminateAllIO. If TerminateAllIO won the race,
it would call Abort() on a Connection object *after* the BridgedConnection destructor had run, while
~Connection was waiting on the termination lock.  This would cause a purecall exception.

BUG=93829, 19757
TEST=SyncAPIServerConnectionManagerTest, integration tests

Review URL: http://codereview.chromium.org/7841013

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

chrome/browser/sync/engine/net/server_connection_manager.h

@@ -10,9 +10,11 @@
 #include <string>
 
 #include "base/atomicops.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/observer_list.h"
 #include "base/string_util.h"
 #include "base/threading/non_thread_safe.h"
+#include "base/threading/thread_checker.h"
 #include "base/synchronization/lock.h"
 #include "chrome/browser/sync/syncable/syncable_id.h"
 #include "chrome/common/net/http_return.h"
@@ -138,7 +140,7 @@ class ScopedServerStatusWatcher : public base::NonThreadSafe {
 // Use this class to interact with the sync server.
 // The ServerConnectionManager currently supports POSTing protocol buffers.
 //
-class ServerConnectionManager : public base::NonThreadSafe {
+class ServerConnectionManager {
  public:
   // buffer_in - will be POSTed
   // buffer_out - string will be overwritten with response
@@ -151,11 +153,10 @@ class ServerConnectionManager : public base::NonThreadSafe {
   // Abstract class providing network-layer functionality to the
   // ServerConnectionManager. Subclasses implement this using an HTTP stack of
   // their choice.
-  class Post {
+  class Connection {
    public:
-    explicit Post(ServerConnectionManager* scm) : scm_(scm) {
-    }
-    virtual ~Post() { }
+    explicit Connection(ServerConnectionManager* scm);
+    virtual ~Connection();
 
     // Called to initialize and perform an HTTP POST.
     virtual bool Init(const char* path,
@@ -163,6 +164,10 @@ class ServerConnectionManager : public base::NonThreadSafe {
                       const std::string& payload,
                       HttpResponse* response) = 0;
 
+    // Immediately abandons a pending HTTP POST request and unblocks caller
+    // in Init.
+    virtual void Abort() = 0;
+
     bool ReadBufferResponse(std::string* buffer_out, HttpResponse* response,
                             bool require_response);
     bool ReadDownloadResponse(HttpResponse* response, std::string* buffer_out);
@@ -222,7 +227,7 @@ class ServerConnectionManager : public base::NonThreadSafe {
   inline std::string user_agent() const { return user_agent_; }
 
   inline HttpResponse::ServerConnectionCode server_status() const {
-    DCHECK(CalledOnValidThread());
+    DCHECK(thread_checker_.CalledOnValidThread());
     return server_status_;
   }
 
@@ -245,19 +250,25 @@ class ServerConnectionManager : public base::NonThreadSafe {
 
   std::string GetServerHost() const;
 
-  // Factory method to create a Post object we can use for communication with
-  // the server.
-  virtual Post* MakePost();
+  // Factory method to create an Connection object we can use for
+  // communication with the server.
+  virtual Connection* MakeConnection();
+
+  // Aborts any active HTTP POST request.
+  // We expect this to get called on a different thread than the valid
+  // ThreadChecker thread, as we want to kill any pending http traffic without
+  // having to wait for the request to complete.
+  void TerminateAllIO();
 
   void set_client_id(const std::string& client_id) {
-    DCHECK(CalledOnValidThread());
+    DCHECK(thread_checker_.CalledOnValidThread());
     DCHECK(client_id_.empty());
     client_id_.assign(client_id);
   }
 
   // Returns true if the auth token is succesfully set and false otherwise.
   bool set_auth_token(const std::string& auth_token) {
-    DCHECK(CalledOnValidThread());
+    DCHECK(thread_checker_.CalledOnValidThread());
     if (previously_invalidated_token != auth_token) {
       auth_token_.assign(auth_token);
       previously_invalidated_token = std::string();
@@ -267,7 +278,7 @@ class ServerConnectionManager : public base::NonThreadSafe {
   }
 
   void InvalidateAndClearAuthToken() {
-    DCHECK(CalledOnValidThread());
+    DCHECK(thread_checker_.CalledOnValidThread());
     // Copy over the token to previous invalid token.
     if (!auth_token_.empty()) {
       previously_invalidated_token.assign(auth_token_);
@@ -276,7 +287,7 @@ class ServerConnectionManager : public base::NonThreadSafe {
   }
 
   const std::string auth_token() const {
-    DCHECK(CalledOnValidThread());
+    DCHECK(thread_checker_.CalledOnValidThread());
     return auth_token_;
   }
 
@@ -301,6 +312,15 @@ class ServerConnectionManager : public base::NonThreadSafe {
                                 const std::string& auth_token,
                                 ScopedServerStatusWatcher* watcher);
 
+  // Helper to check terminated flags and build a Connection object, installing
+  // it as the |active_connection_|.  If this ServerConnectionManager has been
+  // terminated, this will return NULL.
+  Connection* MakeActiveConnection();
+
+  // Called by Connection objects as they are destroyed to allow the
+  // ServerConnectionManager to cleanup active connections.
+  void OnConnectionDestroyed(Connection* connection);
+
   // The sync_server_ is the server that requests will be made to.
   std::string sync_server_;
 
@@ -333,10 +353,40 @@ class ServerConnectionManager : public base::NonThreadSafe {
   HttpResponse::ServerConnectionCode server_status_;
   bool server_reachable_;
 
+  base::ThreadChecker thread_checker_;
+
+  // Protects all variables below to allow bailing out of active connections.
+  base::Lock terminate_connection_lock_;
+
+  // If true, we've been told to terminate IO and expect to be destroyed
+  // shortly.  No future network requests will be made.
+  bool terminated_;
+
+  // A non-owning pointer to any active http connection, so that we can abort
+  // it if necessary.
+  Connection* active_connection_;
+
  private:
-  friend class Post;
+  friend class Connection;
   friend class ScopedServerStatusWatcher;
 
+  // A class to help deal with cleaning up active Connection objects when (for
+  // ex) multiple early-exits are present in some scope. ScopedConnectionHelper
+  // informs the ServerConnectionManager before the Connection object it takes
+  // ownership of is destroyed.
+  class ScopedConnectionHelper {
+   public:
+    // |manager| must outlive this. Takes ownership of |connection|.
+    ScopedConnectionHelper(ServerConnectionManager* manager,
+                           Connection* connection);
+    ~ScopedConnectionHelper();
+    Connection* get();
+   private:
+    ServerConnectionManager* manager_;
+    scoped_ptr<Connection> connection_;
+    DISALLOW_COPY_AND_ASSIGN(ScopedConnectionHelper);
+  };
+
   void NotifyStatusChanged();
 
   DISALLOW_COPY_AND_ASSIGN(ServerConnectionManager);

chrome/browser/sync/internal_api/sync_manager.cc

@@ -1254,6 +1254,10 @@ void SyncManager::SyncInternal::RequestEarlyExit() {
   if (scheduler()) {
     scheduler()->RequestEarlyExit();
   }
+
+  if (connection_manager_.get()) {
+    connection_manager_->TerminateAllIO();
+  }
 }
 
 void SyncManager::Shutdown() {

chrome/browser/sync/internal_api/syncapi_server_connection_manager.cc

@@ -8,30 +8,36 @@
 #include "chrome/browser/sync/internal_api/http_post_provider_interface.h"
 #include "chrome/browser/sync/util/oauth.h"
 #include "chrome/common/net/http_return.h"
+#include "net/base/net_errors.h"
 
 using browser_sync::HttpResponse;
 
 namespace sync_api {
 
-SyncAPIBridgedPost::SyncAPIBridgedPost(
+SyncAPIBridgedConnection::SyncAPIBridgedConnection(
     browser_sync::ServerConnectionManager* scm,
     HttpPostProviderFactory* factory)
-    : Post(scm), factory_(factory) {
+    : Connection(scm), factory_(factory) {
+  post_provider_ = factory_->Create();
 }
 
-SyncAPIBridgedPost::~SyncAPIBridgedPost() {}
+SyncAPIBridgedConnection::~SyncAPIBridgedConnection() {
+  DCHECK(post_provider_);
+  factory_->Destroy(post_provider_);
+  post_provider_ = NULL;
+}
 
-bool SyncAPIBridgedPost::Init(const char* path,
-                              const std::string& auth_token,
-                              const std::string& payload,
-                              HttpResponse* response) {
+bool SyncAPIBridgedConnection::Init(const char* path,
+                                    const std::string& auth_token,
+                                    const std::string& payload,
+                                    HttpResponse* response) {
   std::string sync_server;
   int sync_server_port = 0;
   bool use_ssl = false;
   GetServerParams(&sync_server, &sync_server_port, &use_ssl);
   std::string connection_url = MakeConnectionURL(sync_server, path, use_ssl);
 
-  HttpPostProviderInterface* http = factory_->Create();
+  HttpPostProviderInterface* http = post_provider_;
   http->SetUserAgent(scm_->user_agent().c_str());
   http->SetURL(connection_url.c_str(), sync_server_port);
 
@@ -54,8 +60,8 @@ bool SyncAPIBridgedPost::Init(const char* path,
   int response_code = 0;
   if (!http->MakeSynchronousPost(&os_error_code, &response_code)) {
     VLOG(1) << "Http POST failed, error returns: " << os_error_code;
-    response->server_status = HttpResponse::IO_ERROR;
-    factory_->Destroy(http);
+    response->server_status = os_error_code == net::ERR_ABORTED ?
+        HttpResponse::CONNECTION_UNAVAILABLE : HttpResponse::IO_ERROR;
     return false;
   }
 
@@ -77,12 +83,14 @@ bool SyncAPIBridgedPost::Init(const char* path,
 
   // Write the content into our buffer.
   buffer_.assign(http->GetResponseContent(), http->GetResponseContentLength());
-
-  // We're done with the HttpPostProvider.
-  factory_->Destroy(http);
   return true;
 }
 
+void SyncAPIBridgedConnection::Abort() {
+  DCHECK(post_provider_);
+  post_provider_->Abort();
+}
+
 SyncAPIServerConnectionManager::SyncAPIServerConnectionManager(
     const std::string& server,
     int port,
@@ -96,9 +104,9 @@ SyncAPIServerConnectionManager::SyncAPIServerConnectionManager(
 
 SyncAPIServerConnectionManager::~SyncAPIServerConnectionManager() {}
 
-browser_sync::ServerConnectionManager::Post*
-SyncAPIServerConnectionManager::MakePost() {
-  return new SyncAPIBridgedPost(this, post_provider_factory_.get());
+browser_sync::ServerConnectionManager::Connection*
+SyncAPIServerConnectionManager::MakeConnection() {
+  return new SyncAPIBridgedConnection(this, post_provider_factory_.get());
 }
 
 }  // namespace sync_api

chrome/browser/sync/internal_api/syncapi_server_connection_manager.h

@@ -8,34 +8,40 @@
 
 #include <string>
 
+#include "base/gtest_prod_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "chrome/browser/sync/engine/net/server_connection_manager.h"
 
 namespace sync_api {
 
 class HttpPostProviderFactory;
+class HttpPostProviderInterface;
 
 // This provides HTTP Post functionality through the interface provided
 // to the sync API by the application hosting the syncer backend.
-class SyncAPIBridgedPost
-    : public browser_sync::ServerConnectionManager::Post {
+class SyncAPIBridgedConnection
+    : public browser_sync::ServerConnectionManager::Connection {
  public:
-  SyncAPIBridgedPost(browser_sync::ServerConnectionManager* scm,
-                     HttpPostProviderFactory* factory);
+  SyncAPIBridgedConnection(browser_sync::ServerConnectionManager* scm,
+                           HttpPostProviderFactory* factory);
 
-  virtual ~SyncAPIBridgedPost();
+  virtual ~SyncAPIBridgedConnection();
 
   virtual bool Init(const char* path,
                     const std::string& auth_token,
                     const std::string& payload,
-                    browser_sync::HttpResponse* response);
+                    browser_sync::HttpResponse* response) OVERRIDE;
+
+  virtual void Abort() OVERRIDE;
 
  private:
   // Pointer to the factory we use for creating HttpPostProviders. We do not
   // own |factory_|.
   HttpPostProviderFactory* factory_;
 
-  DISALLOW_COPY_AND_ASSIGN(SyncAPIBridgedPost);
+  HttpPostProviderInterface* post_provider_;
+
+  DISALLOW_COPY_AND_ASSIGN(SyncAPIBridgedConnection);
 };
 
 // A ServerConnectionManager subclass used by the syncapi layer. We use a
@@ -52,10 +58,13 @@ class SyncAPIServerConnectionManager
                                  HttpPostProviderFactory* factory);
   virtual ~SyncAPIServerConnectionManager();
 
- protected:
-  virtual Post* MakePost();
+  // ServerConnectionManager overrides.
+  virtual Connection* MakeConnection() OVERRIDE;
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(SyncAPIServerConnectionManagerTest, EarlyAbortPost);
+  FRIEND_TEST_ALL_PREFIXES(SyncAPIServerConnectionManagerTest, AbortPost);
+
   // A factory creating concrete HttpPostProviders for use whenever we need to
   // issue a POST to sync servers.
   scoped_ptr<HttpPostProviderFactory> post_provider_factory_;

chrome/browser/sync/internal_api/syncapi_server_connection_manager_unittest.cc

+// Copyright (c) 2011 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/internal_api/syncapi_server_connection_manager.h"
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/compiler_specific.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/test/test_timeouts.h"
+#include "base/threading/thread.h"
+#include "base/time.h"
+#include "chrome/browser/sync/internal_api/http_post_provider_factory.h"
+#include "chrome/browser/sync/internal_api/http_post_provider_interface.h"
+#include "net/base/net_errors.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using base::TimeDelta;
+using browser_sync::HttpResponse;
+using browser_sync::ServerConnectionManager;
+using browser_sync::ScopedServerStatusWatcher;
+
+namespace sync_api {
+namespace {
+
+class BlockingHttpPost : public HttpPostProviderInterface {
+ public:
+  BlockingHttpPost() : wait_for_abort_(false, false) {}
+  virtual ~BlockingHttpPost() {}
+
+  virtual void SetUserAgent(const char* user_agent) OVERRIDE {}
+  virtual void SetExtraRequestHeaders(const char* headers) OVERRIDE {}
+  virtual void SetURL(const char* url, int port) OVERRIDE {}
+  virtual void SetPostPayload(const char* content_type,
+                              int content_length,
+                              const char* content) OVERRIDE {}
+  virtual bool MakeSynchronousPost(int* os_error_code, int* response_code)
+      OVERRIDE {
+    wait_for_abort_.TimedWait(TimeDelta::FromMilliseconds(
+        TestTimeouts::action_max_timeout_ms()));
+    *os_error_code = net::ERR_ABORTED;
+    return false;
+  }
+  virtual int GetResponseContentLength() const OVERRIDE {
+    return 0;
+  }
+  virtual const char* GetResponseContent() const OVERRIDE {
+    return "";
+  }
+  virtual const std::string GetResponseHeaderValue(
+      const std::string& name) const OVERRIDE {
+    return "";
+  }
+  virtual void Abort() OVERRIDE {
+    wait_for_abort_.Signal();
+  }
+ private:
+  base::WaitableEvent wait_for_abort_;
+};
+
+class BlockingHttpPostFactory : public HttpPostProviderFactory {
+ public:
+  virtual ~BlockingHttpPostFactory() {}
+  virtual HttpPostProviderInterface* Create() OVERRIDE {
+    return new BlockingHttpPost();
+  }
+  virtual void Destroy(HttpPostProviderInterface* http) OVERRIDE {
+    delete http;
+  }
+};
+
+}  // namespace
+
+TEST(SyncAPIServerConnectionManagerTest, EarlyAbortPost) {
+  SyncAPIServerConnectionManager server(
+      "server", 0, true, "1", new BlockingHttpPostFactory());
+
+  ServerConnectionManager::PostBufferParams params;
+  ScopedServerStatusWatcher watcher(&server, &params.response);
+
+  server.TerminateAllIO();
+  bool result = server.PostBufferToPath(
+      &params, "/testpath", "testauth", &watcher);
+
+  EXPECT_FALSE(result);
+  EXPECT_EQ(HttpResponse::CONNECTION_UNAVAILABLE,
+            params.response.server_status);
+}
+
+TEST(SyncAPIServerConnectionManagerTest, EarlyAbortCheckTime) {
+  SyncAPIServerConnectionManager server(
+      "server", 0, true, "1", new BlockingHttpPostFactory());
+  int32 time = 0;
+  server.TerminateAllIO();
+  bool result = server.CheckTime(&time);
+  EXPECT_FALSE(result);
+}
+
+TEST(SyncAPIServerConnectionManagerTest, AbortPost) {
+  SyncAPIServerConnectionManager server(
+      "server", 0, true, "1", new BlockingHttpPostFactory());
+
+  ServerConnectionManager::PostBufferParams params;
+  ScopedServerStatusWatcher watcher(&server, &params.response);
+
+  base::Thread abort_thread("Test_AbortThread");
+  ASSERT_TRUE(abort_thread.Start());
+  abort_thread.message_loop()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&ServerConnectionManager::TerminateAllIO,
+                 base::Unretained(&server)),
+      TestTimeouts::tiny_timeout_ms());
+
+  bool result = server.PostBufferToPath(
+      &params, "/testpath", "testauth", &watcher);
+
+  EXPECT_FALSE(result);
+  EXPECT_EQ(HttpResponse::CONNECTION_UNAVAILABLE,
+            params.response.server_status);
+  abort_thread.Stop();
+}
+
+TEST(SyncAPIServerConnectionManagerTest, AbortCheckTime) {
+  SyncAPIServerConnectionManager server(
+      "server", 0, true, "1", new BlockingHttpPostFactory());
+
+  base::Thread abort_thread("Test_AbortThread");
+  ASSERT_TRUE(abort_thread.Start());
+  abort_thread.message_loop()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&ServerConnectionManager::TerminateAllIO,
+                 base::Unretained(&server)),
+      TestTimeouts::tiny_timeout_ms());
+
+  int32 time = 0;
+  bool result = server.CheckTime(&time);
+  EXPECT_FALSE(result);
+  abort_thread.Stop();
+}
+
+}  // namespace sync_api

chrome/chrome_tests.gypi

@@ -3102,6 +3102,7 @@
         'browser/sync/engine/syncproto_unittest.cc',
         'browser/sync/engine/verify_updates_command_unittest.cc',
         'browser/sync/internal_api/syncapi_mock.h',
+        'browser/sync/internal_api/syncapi_server_connection_manager_unittest.cc',
         'browser/sync/internal_api/syncapi_unittest.cc',
         'browser/sync/js/js_arg_list_unittest.cc',
         'browser/sync/js/js_event_details_unittest.cc',