Cleanup: Remove CancelationSignal from HttpBridgeFactory

Since crrev.com/c/1174655 landed (more than a year ago),
HttpBridgeFactory doesn't actually need the CancelationSignal anymore.
Removing it also lets us remove a bunch of plumbing.

Bug: 951350
Change-Id: Ic4332320845747aacae43bc7062094f6da77b13b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1912715
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714921}

components/sync/driver/glue/sync_engine_backend.cc

@@ -356,8 +356,7 @@ void SyncEngineBackend::DoInitialize(SyncEngine::InitParams params) {
   args.service_url = params.service_url;
   args.enable_local_sync_backend = params.enable_local_sync_backend;
   args.local_sync_backend_folder = params.local_sync_backend_folder;
-  args.post_factory = std::move(params.http_factory_getter)
-                          .Run(&release_request_context_signal_);
+  args.post_factory = std::move(params.http_factory_getter).Run();
   // Finish initializing the HttpBridgeFactory.  We do this here because
   // building the user agent may block on some platforms.
   args.post_factory->Init(params.sync_user_agent);
@@ -491,14 +490,6 @@ void SyncEngineBackend::ShutdownOnUIThread() {
   // initialized yet.  Those classes will receive the message when the sync
   // thread finally getes around to constructing them.
   stop_syncing_signal_.Signal();
-
-  // 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 stop_syncing_signal_ has already been signalled, which
-  // guarantees that the ServerConnectionManager will no longer attempt to
-  // create new connections.
-  release_request_context_signal_.Signal();
 }
 
 void SyncEngineBackend::DoShutdown(ShutdownReason reason) {

components/sync/driver/glue/sync_engine_backend.h

@@ -249,12 +249,11 @@ class SyncEngineBackend : public base::RefCountedThreadSafe<SyncEngineBackend>,
   WeakHandle<JsBackend> js_backend_;
   WeakHandle<DataTypeDebugInfoListener> debug_info_listener_;
 
-  // 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.
+  // This signal allows us to send requests to shut down the
+  // ServerConnectionManager without having to wait for it to finish
+  // initializing first.
   //
-  // See comments in SyncEngineBackend::ShutdownOnUIThread() for more details.
-  CancelationSignal release_request_context_signal_;
+  // See comment in ShutdownOnUIThread() for more details.
   CancelationSignal stop_syncing_signal_;
 
   // Set when we've been asked to forward sync protocol events to the frontend.

components/sync/engine/net/http_bridge.cc

@@ -19,7 +19,6 @@
 #include "base/strings/stringprintf.h"
 #include "base/task/post_task.h"
 #include "base/threading/thread_restrictions.h"
-#include "components/sync/base/cancelation_signal.h"
 #include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
 #include "net/http/http_cache.h"
@@ -55,30 +54,16 @@ base::LazyInstance<scoped_refptr<base::SequencedTaskRunner>>::Leaky
 HttpBridgeFactory::HttpBridgeFactory(
     std::unique_ptr<network::SharedURLLoaderFactoryInfo>
         url_loader_factory_info,
-    const NetworkTimeUpdateCallback& network_time_update_callback,
-    CancelationSignal* cancelation_signal)
-    : network_time_update_callback_(network_time_update_callback),
-      cancelation_signal_(cancelation_signal) {
+    const NetworkTimeUpdateCallback& network_time_update_callback)
+    : network_time_update_callback_(network_time_update_callback) {
   // Some tests pass null'ed out url_loader_factory_info instances.
   if (url_loader_factory_info) {
     url_loader_factory_ = network::SharedURLLoaderFactory::Create(
         std::move(url_loader_factory_info));
   }
-  // This registration is happening on the Sync thread, while signalling occurs
-  // on the UI thread. We must handle the possibility signalling has already
-  // occurred.
-  if (cancelation_signal_->TryRegisterHandler(this)) {
-    registered_for_cancelation_ = true;
-  } else {
-    OnSignalReceived();
-  }
 }
 
-HttpBridgeFactory::~HttpBridgeFactory() {
-  if (registered_for_cancelation_) {
-    cancelation_signal_->UnregisterHandler(this);
-  }
-}
+HttpBridgeFactory::~HttpBridgeFactory() = default;
 
 void HttpBridgeFactory::Init(const std::string& user_agent) {
   user_agent_ = user_agent;
@@ -97,16 +82,6 @@ void HttpBridgeFactory::Destroy(HttpPostProviderInterface* http) {
   static_cast<HttpBridge*>(http)->Release();
 }
 
-void HttpBridgeFactory::OnSignalReceived() {
-  // TODO(tonikitoo): Remove this method.
-  //
-  // Prior to the URLLoader conversion the sync code was holding on to a
-  // scoped_refptr of a URLRequestContextGetter it was changing the lifetime
-  // of net objects. With URLLoader, it's only holding on to mojo pipes
-  // that issue doesn't exist.
-  NOTIMPLEMENTED();
-}
-
 HttpBridge::URLFetchState::URLFetchState()
     : aborted(false),
       request_completed(false),

components/sync/engine/net/http_bridge.h

@@ -17,7 +17,6 @@
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/thread_checker.h"
 #include "base/timer/timer.h"
-#include "components/sync/base/cancelation_observer.h"
 #include "components/sync/engine/net/http_post_provider_factory.h"
 #include "components/sync/engine/net/http_post_provider_interface.h"
 #include "components/sync/engine/net/network_time_update_callback.h"
@@ -36,8 +35,6 @@ class SimpleURLLoader;
 
 namespace syncer {
 
-class CancelationSignal;
-
 // A bridge between the syncer and Chromium HTTP layers.
 // 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).
@@ -196,14 +193,12 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
   DISALLOW_COPY_AND_ASSIGN(HttpBridge);
 };
 
-class HttpBridgeFactory : public HttpPostProviderFactory,
-                          public CancelationObserver {
+class HttpBridgeFactory : public HttpPostProviderFactory {
  public:
   HttpBridgeFactory(
       std::unique_ptr<network::SharedURLLoaderFactoryInfo>
           url_loader_factory_info,
-      const NetworkTimeUpdateCallback& network_time_update_callback,
-      CancelationSignal* cancelation_signal);
+      const NetworkTimeUpdateCallback& network_time_update_callback);
   ~HttpBridgeFactory() override;
 
   // HttpPostProviderFactory:
@@ -211,9 +206,6 @@ class HttpBridgeFactory : public HttpPostProviderFactory,
   HttpPostProviderInterface* Create() override;
   void Destroy(HttpPostProviderInterface* http) override;
 
-  // CancelationObserver implementation:
-  void OnSignalReceived() override;
-
  private:
   // The user agent to use in all requests.
   std::string user_agent_;
@@ -223,9 +215,6 @@ class HttpBridgeFactory : public HttpPostProviderFactory,
 
   NetworkTimeUpdateCallback network_time_update_callback_;
 
-  CancelationSignal* const cancelation_signal_;
-  bool registered_for_cancelation_ = false;
-
   DISALLOW_COPY_AND_ASSIGN(HttpBridgeFactory);
 };
 

components/sync/engine/net/http_bridge_network_resources.cc

@@ -6,8 +6,6 @@
 
 #include <utility>
 
-#include "base/memory/ptr_util.h"
-#include "components/sync/base/cancelation_signal.h"
 #include "components/sync/engine/net/http_bridge.h"
 #include "components/sync/engine/net/http_post_provider_factory.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
@@ -20,11 +18,9 @@ std::unique_ptr<HttpPostProviderFactory>
 HttpBridgeNetworkResources::GetHttpPostProviderFactory(
     std::unique_ptr<network::SharedURLLoaderFactoryInfo>
         url_loader_factory_info,
-    const NetworkTimeUpdateCallback& network_time_update_callback,
-    CancelationSignal* cancelation_signal) {
-  return base::WrapUnique<HttpPostProviderFactory>(
-      new HttpBridgeFactory(std::move(url_loader_factory_info),
-                            network_time_update_callback, cancelation_signal));
+    const NetworkTimeUpdateCallback& network_time_update_callback) {
+  return std::make_unique<HttpBridgeFactory>(std::move(url_loader_factory_info),
+                                             network_time_update_callback);
 }
 
 }  // namespace syncer

components/sync/engine/net/http_bridge_network_resources.h

@@ -16,7 +16,6 @@ class SharedURLLoaderFactoryInfo;
 
 namespace syncer {
 
-class CancelationSignal;
 class HttpPostProviderFactory;
 
 class HttpBridgeNetworkResources : public NetworkResources {
@@ -27,8 +26,7 @@ class HttpBridgeNetworkResources : public NetworkResources {
   std::unique_ptr<HttpPostProviderFactory> GetHttpPostProviderFactory(
       std::unique_ptr<network::SharedURLLoaderFactoryInfo>
           url_loader_factory_info,
-      const NetworkTimeUpdateCallback& network_time_update_callback,
-      CancelationSignal* cancelation_signal) override;
+      const NetworkTimeUpdateCallback& network_time_update_callback) override;
 };
 
 }  // namespace syncer

components/sync/engine/net/network_resources.h

@@ -15,7 +15,6 @@ class SharedURLLoaderFactoryInfo;
 
 namespace syncer {
 
-class CancelationSignal;
 class HttpPostProviderFactory;
 
 class NetworkResources {
@@ -25,8 +24,7 @@ class NetworkResources {
   virtual std::unique_ptr<HttpPostProviderFactory> GetHttpPostProviderFactory(
       std::unique_ptr<network::SharedURLLoaderFactoryInfo>
           url_loader_factory_info,
-      const NetworkTimeUpdateCallback& network_time_update_callback,
-      CancelationSignal* cancelation_signal) = 0;
+      const NetworkTimeUpdateCallback& network_time_update_callback) = 0;
 };
 
 }  // namespace syncer

components/sync/engine/sync_engine.h

@@ -32,7 +32,6 @@ class GURL;
 
 namespace syncer {
 
-class CancelationSignal;
 class HttpPostProviderFactory;
 class SyncEngineHost;
 class SyncManagerFactory;
@@ -47,8 +46,7 @@ class SyncEngine : public ModelTypeConfigurer {
   using AllNodesCallback =
       base::OnceCallback<void(ModelType, std::unique_ptr<base::ListValue>)>;
   using HttpPostProviderFactoryGetter =
-      base::OnceCallback<std::unique_ptr<HttpPostProviderFactory>(
-          CancelationSignal*)>;
+      base::OnceCallback<std::unique_ptr<HttpPostProviderFactory>()>;
 
   // Utility struct for holding initialization options.
   struct InitParams {

components/sync/test/fake_server/fake_server_network_resources.cc

@@ -5,13 +5,11 @@
 #include "components/sync/test/fake_server/fake_server_network_resources.h"
 
 #include "base/threading/thread_task_runner_handle.h"
-#include "components/sync/base/cancelation_signal.h"
 #include "components/sync/engine/net/http_post_provider_factory.h"
 #include "components/sync/test/fake_server/fake_server.h"
 #include "components/sync/test/fake_server/fake_server_http_post_provider.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 
-using syncer::CancelationSignal;
 using syncer::HttpPostProviderFactory;
 using syncer::NetworkTimeUpdateCallback;
 
@@ -28,8 +26,7 @@ std::unique_ptr<syncer::HttpPostProviderFactory>
 FakeServerNetworkResources::GetHttpPostProviderFactory(
     std::unique_ptr<network::SharedURLLoaderFactoryInfo>
         url_loader_factory_info,
-    const NetworkTimeUpdateCallback& network_time_update_callback,
-    CancelationSignal* cancelation_signal) {
+    const NetworkTimeUpdateCallback& network_time_update_callback) {
   return std::make_unique<FakeServerHttpPostProviderFactory>(
       fake_server_, fake_server_task_runner_);
 }

components/sync/test/fake_server/fake_server_network_resources.h

@@ -32,8 +32,8 @@ class FakeServerNetworkResources : public syncer::NetworkResources {
   std::unique_ptr<syncer::HttpPostProviderFactory> GetHttpPostProviderFactory(
       std::unique_ptr<network::SharedURLLoaderFactoryInfo>
           url_loader_factory_info,
-      const syncer::NetworkTimeUpdateCallback& network_time_update_callback,
-      syncer::CancelationSignal* cancelation_signal) override;
+      const syncer::NetworkTimeUpdateCallback& network_time_update_callback)
+      override;
 
  private:
   base::WeakPtr<FakeServer> fake_server_;