Make HttpPostProviderInterface refcounted

This allows the corresponding factory to return refptrs instead of raw
pointers, and lets remove the explicit Destroy() method.
One downside is that now all implementations have to be refcounted, but
this only affects some test implementations.

One particular test implementation, TestHttpBridge[Factory], was unused
and is deleted.

Bug: 951350
Change-Id: I8c581136389e2ce470786dffefa3145e88c5d19c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2466237
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Victor Vianna <victorvianna@google.com>
Cr-Commit-Position: refs/heads/master@{#816494}

chrome/browser/BUILD.gn

@@ -6485,7 +6485,6 @@ static_library("test_support") {
     "//chrome/browser/subresource_filter:test_support",
     "//chrome/common",
     "//chrome/common/safe_browsing:proto",
-    "//components/browser_sync:test_support",
     "//components/consent_auditor:test_support",
     "//components/invalidation/impl",
     "//components/invalidation/impl:test_support",

chrome/test/BUILD.gn

@@ -4091,7 +4091,6 @@ test("unit_tests") {
     "//chrome/services/machine_learning/public/cpp:test_support",
     "//components/account_id",
     "//components/autofill/content/renderer:test_support",
-    "//components/browser_sync:test_support",
     "//components/browsing_data/content:test_support",
     "//components/captive_portal/core:buildflags",
     "//components/component_updater:test_support",

components/browser_sync/BUILD.gn

@@ -47,7 +47,6 @@ source_set("unit_tests") {
 
   deps = [
     ":browser_sync",
-    ":test_support",
     "//base",
     "//base/test:test_support",
     "//components/autofill/core/browser:test_support",
@@ -73,24 +72,3 @@ source_set("unit_tests") {
     "//testing/gtest",
   ]
 }
-
-static_library("test_support") {
-  testonly = true
-  sources = [
-    "test_http_bridge_factory.cc",
-    "test_http_bridge_factory.h",
-  ]
-
-  deps = [
-    ":browser_sync",
-    "//base",
-    "//base/test:test_support",
-    "//components/bookmarks/browser",
-    "//components/history/core/browser",
-    "//components/sync",
-    "//components/sync:test_support",
-    "//components/sync_preferences:test_support",
-    "//components/sync_sessions:test_support",
-    "//services/network:test_support",
-  ]
-}

components/browser_sync/test_http_bridge_factory.cc

-// Copyright (c) 2012 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 "components/browser_sync/test_http_bridge_factory.h"
-
-namespace browser_sync {
-
-bool TestHttpBridge::MakeSynchronousPost(int* net_error_code,
-                                         int* http_status_code) {
-  return false;
-}
-
-int TestHttpBridge::GetResponseContentLength() const {
-  return 0;
-}
-
-const char* TestHttpBridge::GetResponseContent() const {
-  return nullptr;
-}
-
-const std::string TestHttpBridge::GetResponseHeaderValue(
-    const std::string&) const {
-  return std::string();
-}
-
-void TestHttpBridge::Abort() {}
-
-TestHttpBridgeFactory::TestHttpBridgeFactory() {}
-
-TestHttpBridgeFactory::~TestHttpBridgeFactory() {}
-
-syncer::HttpPostProviderInterface* TestHttpBridgeFactory::Create() {
-  return new TestHttpBridge();
-}
-
-void TestHttpBridgeFactory::Destroy(syncer::HttpPostProviderInterface* http) {
-  delete static_cast<TestHttpBridge*>(http);
-}
-
-}  // namespace browser_sync

components/browser_sync/test_http_bridge_factory.h

-// Copyright (c) 2012 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.
-
-#ifndef COMPONENTS_BROWSER_SYNC_TEST_HTTP_BRIDGE_FACTORY_H_
-#define COMPONENTS_BROWSER_SYNC_TEST_HTTP_BRIDGE_FACTORY_H_
-
-#include <string>
-
-#include "base/compiler_specific.h"
-#include "components/sync/engine/net/http_post_provider_factory.h"
-#include "components/sync/engine/net/http_post_provider_interface.h"
-
-namespace browser_sync {
-
-class TestHttpBridge : public syncer::HttpPostProviderInterface {
- public:
-  // Begin syncer::HttpPostProviderInterface implementation:
-  void SetExtraRequestHeaders(const char* headers) override {}
-
-  void SetURL(const char* url, int port) override {}
-
-  void SetPostPayload(const char* content_type,
-                      int content_length,
-                      const char* content) override {}
-
-  bool MakeSynchronousPost(int* net_error_code, int* http_status_code) override;
-
-  int GetResponseContentLength() const override;
-
-  const char* GetResponseContent() const override;
-
-  const std::string GetResponseHeaderValue(const std::string&) const override;
-
-  void Abort() override;
-  // End syncer::HttpPostProviderInterface implementation.
-};
-
-class TestHttpBridgeFactory : public syncer::HttpPostProviderFactory {
- public:
-  TestHttpBridgeFactory();
-  ~TestHttpBridgeFactory() override;
-
-  // syncer::HttpPostProviderFactory:
-  syncer::HttpPostProviderInterface* Create() override;
-  void Destroy(syncer::HttpPostProviderInterface* http) override;
-};
-
-}  // namespace browser_sync
-
-#endif  // COMPONENTS_BROWSER_SYNC_TEST_HTTP_BRIDGE_FACTORY_H_

components/sync/engine/net/http_bridge.cc

@@ -67,17 +67,12 @@ HttpBridgeFactory::HttpBridgeFactory(
 
 HttpBridgeFactory::~HttpBridgeFactory() = default;
 
-HttpPostProviderInterface* HttpBridgeFactory::Create() {
+scoped_refptr<HttpPostProviderInterface> HttpBridgeFactory::Create() {
   DCHECK(url_loader_factory_);
 
-  scoped_refptr<HttpBridge> http = new HttpBridge(
+  scoped_refptr<HttpPostProviderInterface> http = new HttpBridge(
       user_agent_, url_loader_factory_->Clone(), network_time_update_callback_);
-  http->AddRef();
-  return http.get();
-}
-
-void HttpBridgeFactory::Destroy(HttpPostProviderInterface* http) {
-  static_cast<HttpBridge*>(http)->Release();
+  return http;
 }
 
 HttpBridge::URLFetchState::URLFetchState()
@@ -103,7 +98,7 @@ HttpBridge::HttpBridge(
                                      {base::MayBlock()})),
       network_time_update_callback_(network_time_update_callback) {}
 
-HttpBridge::~HttpBridge() {}
+HttpBridge::~HttpBridge() = default;
 
 void HttpBridge::SetExtraRequestHeaders(const char* headers) {
   DCHECK(extra_headers_.empty())

components/sync/engine/net/http_bridge.h

@@ -12,7 +12,7 @@
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
-#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/synchronization/lock.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/thread_checker.h"
@@ -39,10 +39,7 @@ namespace syncer {
 // Provides a way for the sync backend to use Chromium directly for HTTP
 // requests rather than depending on a third party provider (e.g libcurl).
 // This is a one-time use bridge. Create one for each request you want to make.
-// It is RefCountedThreadSafe because it can PostTask to the io loop, and thus
-// needs to stick around across context switches, etc.
-class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
-                   public HttpPostProviderInterface {
+class HttpBridge : public HttpPostProviderInterface {
  public:
   HttpBridge(const std::string& user_agent,
              std::unique_ptr<network::PendingSharedURLLoaderFactory>
@@ -85,7 +82,6 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
   }
 
  private:
-  friend class base::RefCountedThreadSafe<HttpBridge>;
   FRIEND_TEST_ALL_PREFIXES(SyncHttpBridgeTest,
                            AbortAndReleaseBeforeFetchComplete);
   // Test is disabled on Android.
@@ -204,8 +200,7 @@ class HttpBridgeFactory : public HttpPostProviderFactory {
   ~HttpBridgeFactory() override;
 
   // HttpPostProviderFactory:
-  HttpPostProviderInterface* Create() override;
-  void Destroy(HttpPostProviderInterface* http) override;
+  scoped_refptr<HttpPostProviderInterface> Create() override;
 
  private:
   // The user agent to use in all requests.

components/sync/engine/net/http_post_provider_factory.h

@@ -9,6 +9,7 @@
 #include <string>
 
 #include "base/callback.h"
+#include "base/memory/scoped_refptr.h"
 #include "components/sync/engine/net/network_time_update_callback.h"
 
 namespace network {
@@ -25,19 +26,10 @@ class HttpPostProviderInterface;
 // HttpPostProviders.
 class HttpPostProviderFactory {
  public:
-  virtual ~HttpPostProviderFactory() {}
+  virtual ~HttpPostProviderFactory() = default;
 
   // Obtain a new HttpPostProviderInterface instance, owned by caller.
-  virtual HttpPostProviderInterface* Create() = 0;
-
-  // When the provider is no longer needed (ready to be cleaned up), clients
-  // must call Destroy().
-  // This allows actual HttpPostProvider subclass implementations to be
-  // reference counted, which is useful if a particular implementation uses
-  // multiple threads to serve network requests.
-  // TODO(crbug.com/951350): Either pass out unique_ptrs to providers, or make
-  // the provider interface refcounted, to avoid this manual destruction.
-  virtual void Destroy(HttpPostProviderInterface* http) = 0;
+  virtual scoped_refptr<HttpPostProviderInterface> Create() = 0;
 };
 
 using CreateHttpPostProviderFactory =

components/sync/engine/net/http_post_provider_interface.h

@@ -7,13 +7,19 @@
 
 #include <string>
 
+#include "base/memory/ref_counted.h"
+
 namespace syncer {
 
 // An interface the embedding application (e.g. Chromium) implements to provide
 // required HTTP POST functionality to the syncer backend. This interface is
 // designed for one-time use. You create one, use it, and create another if you
 // want to make a subsequent POST.
-class HttpPostProviderInterface {
+// It is RefCountedThreadSafe because its implementations may PostTask to
+// background task runners, and thus need to stick around across context
+// switches, etc.
+class HttpPostProviderInterface
+    : public base::RefCountedThreadSafe<HttpPostProviderInterface> {
  public:
   // Add additional headers to the request.
   virtual void SetExtraRequestHeaders(const char* headers) = 0;
@@ -55,7 +61,8 @@ class HttpPostProviderInterface {
   virtual void Abort() = 0;
 
  protected:
-  virtual ~HttpPostProviderInterface() {}
+  friend class base::RefCountedThreadSafe<HttpPostProviderInterface>;
+  virtual ~HttpPostProviderInterface() = default;
 };
 
 }  // namespace syncer

components/sync/engine_impl/net/sync_server_connection_manager.cc

@@ -76,7 +76,7 @@ class Connection : public CancelationObserver {
   // operation should be aborted.
   CancelationSignal* const cancelation_signal_;
 
-  HttpPostProviderInterface* const post_provider_;
+  scoped_refptr<HttpPostProviderInterface> const post_provider_;
 
   std::string buffer_;
 
@@ -93,29 +93,24 @@ Connection::Connection(HttpPostProviderFactory* factory,
   DCHECK(post_provider_);
 }
 
-Connection::~Connection() {
-  factory_->Destroy(post_provider_);
-}
+Connection::~Connection() = default;
 
 bool Connection::Init(const std::string& connection_url,
                       int sync_server_port,
                       const std::string& access_token,
                       const std::string& payload,
                       HttpResponse* response) {
-  std::string sync_server;
-
-  HttpPostProviderInterface* http = post_provider_;
-  http->SetURL(connection_url.c_str(), sync_server_port);
+  post_provider_->SetURL(connection_url.c_str(), sync_server_port);
 
   if (!access_token.empty()) {
     std::string headers;
     headers = "Authorization: Bearer " + access_token;
-    http->SetExtraRequestHeaders(headers.c_str());
+    post_provider_->SetExtraRequestHeaders(headers.c_str());
   }
 
   // Must be octet-stream, or the payload may be parsed for a cookie.
-  http->SetPostPayload("application/octet-stream", payload.length(),
-                       payload.data());
+  post_provider_->SetPostPayload("application/octet-stream", payload.length(),
+                                 payload.data());
 
   // Issue the POST, blocking until it finishes.
   int net_error_code = 0;
@@ -130,7 +125,8 @@ bool Connection::Init(const std::string& connection_url,
       &CancelationSignal::UnregisterHandler,
       base::Unretained(cancelation_signal_), base::Unretained(this)));
 
-  if (!http->MakeSynchronousPost(&net_error_code, &http_status_code)) {
+  if (!post_provider_->MakeSynchronousPost(&net_error_code,
+                                           &http_status_code)) {
     DCHECK_NE(net_error_code, net::OK);
     DVLOG(1) << "Http POST failed, error returns: " << net_error_code;
     response->server_status = HttpResponse::CONNECTION_UNAVAILABLE;
@@ -141,9 +137,9 @@ bool Connection::Init(const std::string& connection_url,
   // We got a server response, copy over response codes and content.
   response->http_status_code = http_status_code;
   response->content_length =
-      static_cast<int64_t>(http->GetResponseContentLength());
+      static_cast<int64_t>(post_provider_->GetResponseContentLength());
   response->payload_length =
-      static_cast<int64_t>(http->GetResponseContentLength());
+      static_cast<int64_t>(post_provider_->GetResponseContentLength());
   if (response->http_status_code == net::HTTP_OK)
     response->server_status = HttpResponse::SERVER_CONNECTION_OK;
   else if (response->http_status_code == net::HTTP_UNAUTHORIZED)
@@ -152,7 +148,8 @@ bool Connection::Init(const std::string& connection_url,
     response->server_status = HttpResponse::SYNC_SERVER_ERROR;
 
   // Write the content into our buffer.
-  buffer_.assign(http->GetResponseContent(), http->GetResponseContentLength());
+  buffer_.assign(post_provider_->GetResponseContent(),
+                 post_provider_->GetResponseContentLength());
   return true;
 }
 

components/sync/engine_impl/net/sync_server_connection_manager_unittest.cc

@@ -27,7 +27,6 @@ class BlockingHttpPost : public HttpPostProviderInterface {
   BlockingHttpPost()
       : wait_for_abort_(base::WaitableEvent::ResetPolicy::AUTOMATIC,
                         base::WaitableEvent::InitialState::NOT_SIGNALED) {}
-  ~BlockingHttpPost() override {}
 
   void SetExtraRequestHeaders(const char* headers) override {}
   void SetURL(const char* url, int port) override {}
@@ -49,19 +48,18 @@ class BlockingHttpPost : public HttpPostProviderInterface {
   void Abort() override { wait_for_abort_.Signal(); }
 
  private:
+  ~BlockingHttpPost() override = default;
+
   base::WaitableEvent wait_for_abort_;
 };
 
 class BlockingHttpPostFactory : public HttpPostProviderFactory {
  public:
-  ~BlockingHttpPostFactory() override {}
+  ~BlockingHttpPostFactory() override = default;
 
-  HttpPostProviderInterface* Create() override {
+  scoped_refptr<HttpPostProviderInterface> Create() override {
     return new BlockingHttpPost();
   }
-  void Destroy(HttpPostProviderInterface* http) override {
-    delete static_cast<BlockingHttpPost*>(http);
-  }
 };
 
 }  // namespace
@@ -130,7 +128,6 @@ class FailingHttpPost : public HttpPostProviderInterface {
  public:
   explicit FailingHttpPost(int net_error_code)
       : net_error_code_(net_error_code) {}
-  ~FailingHttpPost() override {}
 
   void SetExtraRequestHeaders(const char* headers) override {}
   void SetURL(const char* url, int port) override {}
@@ -151,6 +148,8 @@ class FailingHttpPost : public HttpPostProviderInterface {
   void Abort() override {}
 
  private:
+  ~FailingHttpPost() override = default;
+
   int net_error_code_;
 };
 
@@ -158,14 +157,11 @@ class FailingHttpPostFactory : public HttpPostProviderFactory {
  public:
   explicit FailingHttpPostFactory(int net_error_code)
       : net_error_code_(net_error_code) {}
-  ~FailingHttpPostFactory() override {}
+  ~FailingHttpPostFactory() override = default;
 
-  HttpPostProviderInterface* Create() override {
+  scoped_refptr<HttpPostProviderInterface> Create() override {
     return new FailingHttpPost(net_error_code_);
   }
-  void Destroy(HttpPostProviderInterface* http) override {
-    delete static_cast<FailingHttpPost*>(http);
-  }
 
  private:
   int net_error_code_;

components/sync/engine_impl/sync_manager_impl_unittest.cc

@@ -69,8 +69,6 @@ namespace {
 
 class TestHttpPostProviderInterface : public HttpPostProviderInterface {
  public:
-  ~TestHttpPostProviderInterface() override {}
-
   void SetExtraRequestHeaders(const char* headers) override {}
   void SetURL(const char* url, int port) override {}
   void SetPostPayload(const char* content_type,
@@ -87,17 +85,17 @@ class TestHttpPostProviderInterface : public HttpPostProviderInterface {
     return std::string();
   }
   void Abort() override {}
+
+ private:
+  ~TestHttpPostProviderInterface() override = default;
 };
 
 class TestHttpPostProviderFactory : public HttpPostProviderFactory {
  public:
-  ~TestHttpPostProviderFactory() override {}
-  HttpPostProviderInterface* Create() override {
+  ~TestHttpPostProviderFactory() override = default;
+  scoped_refptr<HttpPostProviderInterface> Create() override {
     return new TestHttpPostProviderInterface();
   }
-  void Destroy(HttpPostProviderInterface* http) override {
-    delete static_cast<TestHttpPostProviderInterface*>(http);
-  }
 };
 
 class SyncManagerObserverMock : public SyncManager::Observer {

components/sync/test/fake_server/fake_server_http_post_provider.cc

@@ -24,18 +24,12 @@ FakeServerHttpPostProviderFactory::FakeServerHttpPostProviderFactory(
     : fake_server_(fake_server),
       fake_server_task_runner_(fake_server_task_runner) {}
 
-FakeServerHttpPostProviderFactory::~FakeServerHttpPostProviderFactory() {}
+FakeServerHttpPostProviderFactory::~FakeServerHttpPostProviderFactory() =
+    default;
 
-syncer::HttpPostProviderInterface* FakeServerHttpPostProviderFactory::Create() {
-  FakeServerHttpPostProvider* http =
-      new FakeServerHttpPostProvider(fake_server_, fake_server_task_runner_);
-  http->AddRef();
-  return http;
-}
-
-void FakeServerHttpPostProviderFactory::Destroy(
-    syncer::HttpPostProviderInterface* http) {
-  static_cast<FakeServerHttpPostProvider*>(http)->Release();
+scoped_refptr<syncer::HttpPostProviderInterface>
+FakeServerHttpPostProviderFactory::Create() {
+  return new FakeServerHttpPostProvider(fake_server_, fake_server_task_runner_);
 }
 
 FakeServerHttpPostProvider::FakeServerHttpPostProvider(

components/sync/test/fake_server/fake_server_http_post_provider.h

@@ -22,9 +22,7 @@ namespace fake_server {
 
 class FakeServer;
 
-class FakeServerHttpPostProvider
-    : public syncer::HttpPostProviderInterface,
-      public base::RefCountedThreadSafe<FakeServerHttpPostProvider> {
+class FakeServerHttpPostProvider : public syncer::HttpPostProviderInterface {
  public:
   FakeServerHttpPostProvider(
       const base::WeakPtr<FakeServer>& fake_server,
@@ -90,8 +88,7 @@ class FakeServerHttpPostProviderFactory
   ~FakeServerHttpPostProviderFactory() override;
 
   // HttpPostProviderFactory:
-  syncer::HttpPostProviderInterface* Create() override;
-  void Destroy(syncer::HttpPostProviderInterface* http) override;
+  scoped_refptr<syncer::HttpPostProviderInterface> Create() override;
 
  private:
   // |fake_server_| should only be dereferenced on the same thread as

ios/web_view/BUILD.gn

@@ -475,7 +475,6 @@ test("ios_web_view_unittests") {
     "//components/autofill/core/browser:test_support",
     "//components/autofill/ios/browser:test_support",
     "//components/autofill/ios/form_util:test_support",
-    "//components/browser_sync:test_support",
     "//components/invalidation/impl:test_support",
     "//components/password_manager/core/browser:test_support",
     "//components/prefs:test_support",