Migrate components/sync/engine/net/http_bridge.cc to network::SimpleURLLoader

URLFetcher will stop working with advent of Network Service, and
SimpleURLLoader is the replacement API for most clients.
This CL migrates HttpBridge and FinancialPing away from URLFetcher.

Note that this CL slightly changes the flow of the HttpBridge{Factory} code.
Previously, an URLRequestContextGetter instance was acquired on UI thread,
passed to the Sync and IO threads.
However, URLLoaderFactory and SimpleURLLoader have stricter threading
restrictions than URLFetcher and URLRequestContextGetter. For instance, a
SharedURLLoaderFactory instance needs to be created, used and deleted on
the same thread.

For this reason methods like HttpBridgeFactory::OnSignalReceived
can not simply null out |url_loader_factory_| which is created on the "sync"
thread.

Additionally, some preexisting tests do not seem relevant for the context
of network service, and got removed, eg TestUsesSameHttpNetworkSession,
RequestContextGetterReleaseOrder and EarlyAbortFactory.
Particularly, the last two used to deal with URLRequestContextGetter
destruction. With the URLLoader migration, HttpBridge now holds on
to mojo pipes  instead of URLRequestContextGetter.

BUG=773295,844968

Change-Id: I185f814ba171dca69fec55b913b86a0cc9993ef1
Reviewed-on: https://chromium-review.googlesource.com/1174655
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Reviewed-by: Nicolas Zea <zea@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586115}

components/browser_sync/profile_sync_service.cc

@@ -411,7 +411,8 @@ syncer::SyncEngine::HttpPostProviderFactoryGetter
 ProfileSyncService::MakeHttpPostProviderFactoryGetter() {
   return base::BindRepeating(
       &syncer::NetworkResources::GetHttpPostProviderFactory,
-      base::Unretained(network_resources_.get()), url_request_context_,
+      base::Unretained(network_resources_.get()),
+      base::Passed(url_loader_factory_->Clone()),
       network_time_update_callback_);
 }
 

components/sync/BUILD.gn

@@ -1009,6 +1009,7 @@ static_library("test_support_fake_server") {
     ":sync",
     "//base",
     "//net",
+    "//services/network/public/cpp:cpp",
     "//testing/gtest",
     "//url",
   ]

components/sync/driver/glue/sync_backend_host_impl_unittest.cc

@@ -42,8 +42,7 @@
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "google/cacheinvalidation/include/types.h"
 #include "google_apis/gaia/gaia_constants.h"
-#include "net/url_request/test_url_fetcher_factory.h"
-#include "net/url_request/url_request_context_getter.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"

components/sync/engine/net/DEPS

 include_rules = [
   "+net",
+  "+services/network/public/cpp",
+  "+services/network/test",
   "+third_party/zlib/google",
 ]

components/sync/engine/net/http_bridge.h

@@ -21,17 +21,18 @@
 #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"
-#include "net/url_request/url_fetcher_delegate.h"
-#include "net/url_request/url_request_context.h"
-#include "net/url_request/url_request_context_getter.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "url/gurl.h"
 
 class HttpBridgeTest;
 
 namespace net {
 class HttpResponseHeaders;
-class URLFetcher;
-}
+}  // namespace net
+
+namespace network {
+class SimpleURLLoader;
+}  // namespace network
 
 namespace syncer {
 
@@ -44,11 +45,11 @@ class CancelationSignal;
 // 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,
-                   public net::URLFetcherDelegate {
+                   public HttpPostProviderInterface {
  public:
   HttpBridge(const std::string& user_agent,
-             const scoped_refptr<net::URLRequestContextGetter>& context,
+             std::unique_ptr<network::SharedURLLoaderFactoryInfo>
+                 url_loader_factory_info,
              const NetworkTimeUpdateCallback& network_time_update_callback,
              const BindToTrackerCallback& bind_to_tracker_callback);
 
@@ -70,17 +71,11 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
   const std::string GetResponseHeaderValue(
       const std::string& name) const override;
 
-  // net::URLFetcherDelegate implementation.
-  void OnURLFetchComplete(const net::URLFetcher* source) override;
-  void OnURLFetchDownloadProgress(const net::URLFetcher* source,
-                                  int64_t current,
-                                  int64_t total,
-                                  int64_t current_network_bytes) override;
-  void OnURLFetchUploadProgress(const net::URLFetcher* source,
-                                int64_t current,
-                                int64_t total) override;
+  void OnURLLoadComplete(std::unique_ptr<std::string> response_body);
+  void OnURLLoadUploadProgress(uint64_t position, uint64_t total);
 
-  net::URLRequestContextGetter* GetRequestContextGetterForTest() const;
+  static void SetIOCapableTaskRunnerForTest(
+      scoped_refptr<base::SequencedTaskRunner> task_runner);
 
  protected:
   ~HttpBridge() override;
@@ -88,9 +83,19 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
   // Protected virtual so the unit test can override to shunt network requests.
   virtual void MakeAsynchronousPost();
 
+  void set_url_loader_factory_for_testing(
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory) {
+    url_loader_factory_ = url_loader_factory;
+  }
+
  private:
   friend class base::RefCountedThreadSafe<HttpBridge>;
-  friend class SyncHttpBridgeTest;
+  FRIEND_TEST_ALL_PREFIXES(SyncHttpBridgeTest,
+                           AbortAndReleaseBeforeFetchComplete);
+  // Test is disabled on Android.
+  FRIEND_TEST_ALL_PREFIXES(DISABLED_SyncHttpBridgeTest,
+                           AbortAndReleaseBeforeFetchComplete);
+  friend class ShuntedHttpBridge;
   friend class ::HttpBridgeTest;
 
   // Called on the IO loop to issue the network request. The extra level
@@ -98,17 +103,25 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
   // still have a function to statically pass to PostTask.
   void CallMakeAsynchronousPost() { MakeAsynchronousPost(); }
 
+  // Actual implementation of the load complete callback. Called by tests too.
+  void OnURLLoadCompleteInternal(int response_code,
+                                 int net_error,
+                                 int64_t compressed_content_length,
+                                 const GURL& final_url,
+                                 std::unique_ptr<std::string> response_body);
+
   // Used to destroy a fetcher when the bridge is Abort()ed, to ensure that
   // a reference to |this| is held while flushing any pending fetch completion
   // callbacks coming from the IO thread en route to finally destroying the
   // fetcher.
-  void DestroyURLFetcherOnIOThread(net::URLFetcher* fetcher,
-                                   base::OneShotTimer* fetch_timer);
+  void DestroyURLLoaderOnIOThread(
+      std::unique_ptr<network::SimpleURLLoader> loader,
+      std::unique_ptr<base::OneShotTimer> loader_timer);
 
   void UpdateNetworkTime();
 
   // Helper method to abort the request if we timed out.
-  void OnURLFetchTimedOut();
+  void OnURLLoadTimedOut();
 
   // Used to check whether a method runs on the thread that we were created on.
   // This is the thread that will block on MakeSynchronousPost while the IO
@@ -141,7 +154,7 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
     // NOTE: This is not a unique_ptr for a reason. It must be deleted on the
     // same thread that created it, which isn't the same thread |this| gets
     // deleted on. We must manually delete url_poster_ on the IO loop.
-    net::URLFetcher* url_poster;
+    std::unique_ptr<network::SimpleURLLoader> url_loader;
 
     // Start and finish time of request. Set immediately before sending
     // request and after receiving response.
@@ -164,16 +177,20 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
     std::unique_ptr<base::OneShotTimer> http_request_timeout_timer;
   };
 
-  // This lock synchronizes use of state involved in the flow to fetch a URL
-  // using URLFetcher, including |fetch_state_| and |request_context_getter_| on
-  // any thread, for example, this flow needs to be synchronized to gracefully
+  // This lock synchronizes use of state involved in the flow to load a URL
+  // using URLLoader, including |fetch_state_| on any thread, for example,
+  // this flow needs to be synchronized to gracefully
   // clean up URLFetcher and return appropriate values in |error_code|.
+  //
+  // TODO(crbug.com/844968): Check whether we can get rid of |fetch_state_lock_|
+  // altogether after the migration to SimpleURLLoader.
   mutable base::Lock fetch_state_lock_;
   URLFetchState fetch_state_;
 
-  scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
+  std::unique_ptr<network::SharedURLLoaderFactoryInfo> url_loader_factory_info_;
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
 
-  const scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_;
+  const scoped_refptr<base::SequencedTaskRunner> network_task_runner_;
 
   // Callback for updating network time.
   NetworkTimeUpdateCallback network_time_update_callback_;
@@ -189,8 +206,8 @@ class HttpBridgeFactory : public HttpPostProviderFactory,
                           public CancelationObserver {
  public:
   HttpBridgeFactory(
-      const scoped_refptr<net::URLRequestContextGetter>&
-          baseline_context_getter,
+      std::unique_ptr<network::SharedURLLoaderFactoryInfo>
+          url_loader_factory_info,
       const NetworkTimeUpdateCallback& network_time_update_callback,
       CancelationSignal* cancelation_signal);
   ~HttpBridgeFactory() override;
@@ -208,12 +225,8 @@ class HttpBridgeFactory : public HttpPostProviderFactory,
   // The user agent to use in all requests.
   std::string user_agent_;
 
-  // Protects |request_context_getter_| to allow releasing it's reference from
-  // the sync thread, even when it's in use on the IO thread.
-  base::Lock request_context_getter_lock_;
-
-  // The request context getter used for making all requests.
-  scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
+  // The URL loader factory used for making all requests.
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
 
   NetworkTimeUpdateCallback network_time_update_callback_;
 

components/sync/engine/net/http_bridge_network_resources.cc

@@ -4,11 +4,13 @@
 
 #include "components/sync/engine/net/http_bridge_network_resources.h"
 
+#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 "net/url_request/url_request_context_getter.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 
 namespace syncer {
 
@@ -16,11 +18,12 @@ HttpBridgeNetworkResources::~HttpBridgeNetworkResources() {}
 
 std::unique_ptr<HttpPostProviderFactory>
 HttpBridgeNetworkResources::GetHttpPostProviderFactory(
-    const scoped_refptr<net::URLRequestContextGetter>& baseline_context_getter,
+    std::unique_ptr<network::SharedURLLoaderFactoryInfo>
+        url_loader_factory_info,
     const NetworkTimeUpdateCallback& network_time_update_callback,
     CancelationSignal* cancelation_signal) {
   return base::WrapUnique<HttpPostProviderFactory>(
-      new HttpBridgeFactory(baseline_context_getter,
+      new HttpBridgeFactory(std::move(url_loader_factory_info),
                             network_time_update_callback, cancelation_signal));
 }
 

components/sync/engine/net/http_bridge_network_resources.h

@@ -10,9 +10,9 @@
 #include "components/sync/engine/net/network_resources.h"
 #include "components/sync/engine/net/network_time_update_callback.h"
 
-namespace net {
-class URLRequestContextGetter;
-}  // namespace net
+namespace network {
+class SharedURLLoaderFactoryInfo;
+}  // namespace network
 
 namespace syncer {
 
@@ -25,8 +25,8 @@ class HttpBridgeNetworkResources : public NetworkResources {
 
   // NetworkResources
   std::unique_ptr<HttpPostProviderFactory> GetHttpPostProviderFactory(
-      const scoped_refptr<net::URLRequestContextGetter>&
-          baseline_context_getter,
+      std::unique_ptr<network::SharedURLLoaderFactoryInfo>
+          url_loader_factory_info,
       const NetworkTimeUpdateCallback& network_time_update_callback,
       CancelationSignal* cancelation_signal) override;
 };

components/sync/engine/net/network_resources.h

@@ -9,9 +9,9 @@
 
 #include "components/sync/engine/net/network_time_update_callback.h"
 
-namespace net {
-class URLRequestContextGetter;
-}  // namespace net
+namespace network {
+class SharedURLLoaderFactoryInfo;
+}  // namespace network
 
 namespace syncer {
 
@@ -23,8 +23,8 @@ class NetworkResources {
   virtual ~NetworkResources() {}
 
   virtual std::unique_ptr<HttpPostProviderFactory> GetHttpPostProviderFactory(
-      const scoped_refptr<net::URLRequestContextGetter>&
-          baseline_context_getter,
+      std::unique_ptr<network::SharedURLLoaderFactoryInfo>
+          url_loader_factory_info,
       const NetworkTimeUpdateCallback& network_time_update_callback,
       CancelationSignal* cancelation_signal) = 0;
 };

components/sync/test/DEPS

@@ -2,6 +2,7 @@ include_rules = [
   # Test files can include anything from sync.
   "+components/sync",
   "+net",
+  '+services/network/public/cpp',
 
   # For generated JNI code.
   "+jni",

components/sync/test/fake_server/fake_server_network_resources.cc

@@ -9,6 +9,7 @@
 #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;
@@ -25,7 +26,8 @@ FakeServerNetworkResources::~FakeServerNetworkResources() {}
 
 std::unique_ptr<syncer::HttpPostProviderFactory>
 FakeServerNetworkResources::GetHttpPostProviderFactory(
-    const scoped_refptr<net::URLRequestContextGetter>& baseline_context_getter,
+    std::unique_ptr<network::SharedURLLoaderFactoryInfo>
+        url_loader_factory_info,
     const NetworkTimeUpdateCallback& network_time_update_callback,
     CancelationSignal* cancelation_signal) {
   return std::make_unique<FakeServerHttpPostProviderFactory>(

components/sync/test/fake_server/fake_server_network_resources.h

@@ -13,9 +13,9 @@
 #include "components/sync/engine/net/network_resources.h"
 #include "components/sync/engine/net/network_time_update_callback.h"
 
-namespace net {
-class URLRequestContextGetter;
-}  // namespace net
+namespace network {
+class SharedURLLoaderFactoryInfo;
+}  // namespace network
 
 namespace fake_server {
 
@@ -30,8 +30,8 @@ class FakeServerNetworkResources : public syncer::NetworkResources {
 
   // NetworkResources
   std::unique_ptr<syncer::HttpPostProviderFactory> GetHttpPostProviderFactory(
-      const scoped_refptr<net::URLRequestContextGetter>&
-          baseline_context_getter,
+      std::unique_ptr<network::SharedURLLoaderFactoryInfo>
+          url_loader_factory_info,
       const syncer::NetworkTimeUpdateCallback& network_time_update_callback,
       syncer::CancelationSignal* cancelation_signal) override;
 

components/sync/tools/BUILD.gn

@@ -52,5 +52,6 @@ test("sync_client") {
     "//components/sync:test_support_engine",
     "//jingle:notifier",
     "//net:test_support",
+    "//services/network:test_support",
   ]
 }

components/sync/tools/DEPS

@@ -8,4 +8,5 @@ include_rules = [
   "+google/cacheinvalidation",
   "+jingle/notifier/base",
   "+net",
+  "+services/network/test",
 ]

components/sync/tools/sync_client.cc

@@ -51,6 +51,7 @@
 #include "net/dns/host_resolver.h"
 #include "net/http/transport_security_state.h"
 #include "net/url_request/url_request_test_util.h"
+#include "services/network/test/test_shared_url_loader_factory.h"
 #include "url/gurl.h"
 
 #if defined(OS_MACOSX)
@@ -393,10 +394,13 @@ int SyncClientMain(int argc, char* argv[]) {
   const char kUserAgent[] = "sync_client";
   // TODO(akalin): Replace this with just the context getter once
   // HttpPostProviderFactory is removed.
+  auto url_loader_factory =
+      base::MakeRefCounted<network::TestSharedURLLoaderFactory>();
   CancelationSignal factory_cancelation_signal;
-  std::unique_ptr<HttpPostProviderFactory> post_factory(new HttpBridgeFactory(
-      context_getter.get(), base::BindRepeating(&StubNetworkTimeUpdateCallback),
-      &factory_cancelation_signal));
+  std::unique_ptr<HttpPostProviderFactory> post_factory(
+      new HttpBridgeFactory(url_loader_factory->Clone(),
+                            base::BindRepeating(&StubNetworkTimeUpdateCallback),
+                            &factory_cancelation_signal));
   post_factory->Init(kUserAgent, BindToTrackerCallback());
   // Used only when committing bookmarks, so it's okay to leave this as null.
   ExtensionsActivity* extensions_activity = nullptr;