Convert BlacklistStateFetcher to use SimpleURLLoader instead of URLFetcher.

Bug: 825242
Change-Id: Ifb779d7bdc42dc98558803298576568d65a95bc5
Reviewed-on: https://chromium-review.googlesource.com/988806
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547437}

chrome/browser/extensions/blacklist_state_fetcher.cc

@@ -13,18 +13,16 @@
 #include "google_apis/google_api_keys.h"
 #include "net/base/escape.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
-#include "net/url_request/url_request_context.h"
-#include "net/url_request/url_request_context_getter.h"
 #include "net/url_request/url_request_status.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/simple_url_loader.h"
 #include "url/gurl.h"
 
 using content::BrowserThread;
 
 namespace extensions {
 
-BlacklistStateFetcher::BlacklistStateFetcher()
-    : url_fetcher_id_(0),
-      weak_ptr_factory_(this) {}
+BlacklistStateFetcher::BlacklistStateFetcher() : weak_ptr_factory_(this) {}
 
 BlacklistStateFetcher::~BlacklistStateFetcher() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ -49,10 +47,9 @@ void BlacklistStateFetcher::Request(const std::string& id,
   if (request_already_sent)
     return;
 
-  if (!url_request_context_getter_ && g_browser_process &&
-      g_browser_process->safe_browsing_service()) {
-    url_request_context_getter_ =
-        g_browser_process->safe_browsing_service()->url_request_context();
+  if (g_browser_process && g_browser_process->safe_browsing_service()) {
+    url_loader_factory_ =
+        g_browser_process->safe_browsing_service()->GetURLLoaderFactory();
   }
 
   SendRequest(id);
@@ -98,15 +95,19 @@ void BlacklistStateFetcher::SendRequest(const std::string& id) {
             }
           }
         })");
-  std::unique_ptr<net::URLFetcher> fetcher_ptr =
-      net::URLFetcher::Create(url_fetcher_id_++, request_url,
-                              net::URLFetcher::POST, this, traffic_annotation);
-  net::URLFetcher* fetcher = fetcher_ptr.get();
+  auto resource_request = std::make_unique<network::ResourceRequest>();
+  resource_request->url = request_url;
+  resource_request->method = "POST";
+  std::unique_ptr<network::SimpleURLLoader> fetcher_ptr =
+      network::SimpleURLLoader::Create(std::move(resource_request),
+                                       traffic_annotation);
+  auto* fetcher = fetcher_ptr.get();
+  fetcher->AttachStringForUpload(request_str, "application/octet-stream");
   requests_[fetcher] = {std::move(fetcher_ptr), id};
-  fetcher->SetAutomaticallyRetryOn5xx(false);  // Don't retry on error.
-  fetcher->SetRequestContext(url_request_context_getter_.get());
-  fetcher->SetUploadData("application/octet-stream", request_str);
-  fetcher->Start();
+  fetcher->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+      url_loader_factory_.get(),
+      base::BindOnce(&BlacklistStateFetcher::OnURLLoaderComplete,
+                     base::Unretained(this), fetcher));
 }
 
 void BlacklistStateFetcher::SetSafeBrowsingConfig(
@@ -115,11 +116,6 @@ void BlacklistStateFetcher::SetSafeBrowsingConfig(
       new safe_browsing::SafeBrowsingProtocolConfig(config));
 }
 
-void BlacklistStateFetcher::SetURLRequestContextForTest(
-      net::URLRequestContextGetter* request_context) {
-  url_request_context_getter_ = request_context;
-}
-
 GURL BlacklistStateFetcher::RequestUrl() const {
   std::string url = base::StringPrintf(
       "%s/%s?client=%s&appver=%s&pver=2.2",
@@ -135,37 +131,54 @@ GURL BlacklistStateFetcher::RequestUrl() const {
   return GURL(url);
 }
 
-void BlacklistStateFetcher::OnURLFetchComplete(const net::URLFetcher* source) {
+void BlacklistStateFetcher::OnURLLoaderComplete(
+    network::SimpleURLLoader* url_loader,
+    std::unique_ptr<std::string> response_body) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  auto it = requests_.find(source);
+  int response_code = 0;
+  if (url_loader->ResponseInfo() && url_loader->ResponseInfo()->headers)
+    response_code = url_loader->ResponseInfo()->headers->response_code();
+
+  std::string response_body_str;
+  if (response_body.get())
+    response_body_str = std::move(*response_body.get());
+
+  OnURLLoaderCompleteInternal(url_loader, response_body_str, response_code,
+                              url_loader->NetError());
+}
+
+void BlacklistStateFetcher::OnURLLoaderCompleteInternal(
+    network::SimpleURLLoader* url_loader,
+    const std::string& response_body,
+    int response_code,
+    int net_error) {
+  auto it = requests_.find(url_loader);
   if (it == requests_.end()) {
     NOTREACHED();
     return;
   }
 
-  std::unique_ptr<net::URLFetcher> fetcher = std::move(it->second.first);
+  std::unique_ptr<network::SimpleURLLoader> loader =
+      std::move(it->second.first);
   std::string id = it->second.second;
   requests_.erase(it);
 
   BlacklistState state;
-
-  if (source->GetStatus().is_success() && source->GetResponseCode() == 200) {
-    std::string data;
-    source->GetResponseAsString(&data);
+  if (net_error == net::OK && response_code == 200) {
     ClientCRXListInfoResponse response;
-    if (response.ParseFromString(data)) {
+    if (response.ParseFromString(response_body)) {
       state = static_cast<BlacklistState>(response.verdict());
     } else {
       state = BLACKLISTED_UNKNOWN;
     }
   } else {
-    if (source->GetStatus().status() == net::URLRequestStatus::FAILED) {
+    if (net_error != net::OK) {
       VLOG(1) << "Blacklist request for: " << id
-              << " failed with error: " << source->GetStatus().error();
+              << " failed with error: " << net_error;
     } else {
       VLOG(1) << "Blacklist request for: " << id
-              << " failed with error: " << source->GetResponseCode();
+              << " failed with error: " << response_code;
     }
 
     state = BLACKLISTED_UNKNOWN;
@@ -174,8 +187,7 @@ void BlacklistStateFetcher::OnURLFetchComplete(const net::URLFetcher* source) {
   std::pair<CallbackMultiMap::iterator, CallbackMultiMap::iterator> range =
       callbacks_.equal_range(id);
   for (CallbackMultiMap::const_iterator callback_it = range.first;
-       callback_it != range.second;
-       ++callback_it) {
+       callback_it != range.second; ++callback_it) {
     callback_it->second.Run(state);
   }
 

chrome/browser/extensions/blacklist_state_fetcher.h

@@ -15,53 +15,53 @@
 #include "base/memory/weak_ptr.h"
 #include "components/safe_browsing/db/util.h"
 #include "extensions/browser/blacklist_state.h"
-#include "net/url_request/url_fetcher.h"
-#include "net/url_request/url_fetcher_delegate.h"
 
-namespace net {
-class URLRequestContextGetter;
-}
+namespace network {
+class SharedURLLoaderFactory;
+class SimpleURLLoader;
+}  // namespace network
 
 namespace extensions {
+class TestBlacklistStateFetcher;
 
-class BlacklistStateFetcher : public net::URLFetcherDelegate {
+class BlacklistStateFetcher {
  public:
   typedef base::Callback<void(BlacklistState)> RequestCallback;
 
   BlacklistStateFetcher();
 
-  ~BlacklistStateFetcher() override;
+  virtual ~BlacklistStateFetcher();
 
   virtual void Request(const std::string& id, const RequestCallback& callback);
 
   void SetSafeBrowsingConfig(
       const safe_browsing::SafeBrowsingProtocolConfig& config);
 
-  void SetURLRequestContextForTest(
-      net::URLRequestContextGetter* request_context);
-
  protected:
-  // net::URLFetcherDelegate interface.
-  void OnURLFetchComplete(const net::URLFetcher* source) override;
+  void OnURLLoaderComplete(network::SimpleURLLoader* url_loader,
+                           std::unique_ptr<std::string> response_body);
+
+  // Used for ease unit tests.
+  void OnURLLoaderCompleteInternal(network::SimpleURLLoader* url_loader,
+                                   const std::string& response_body,
+                                   int response_code,
+                                   int net_error);
 
  private:
+  friend class TestBlacklistStateFetcher;
   typedef std::multimap<std::string, RequestCallback> CallbackMultiMap;
 
   GURL RequestUrl() const;
 
   void SendRequest(const std::string& id);
 
-  // ID for URLFetchers for testing.
-  int url_fetcher_id_;
-
   std::unique_ptr<safe_browsing::SafeBrowsingProtocolConfig>
       safe_browsing_config_;
-  scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
-  scoped_refptr<net::URLRequestContextGetter> parent_request_context_for_test_;
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
 
-  // URLFetcher -> (owned fetcher, extension id).
-  std::map<const net::URLFetcher*,
-           std::pair<std::unique_ptr<net::URLFetcher>, std::string>>
+  // SimpleURLLoader -> (owned loader, extension id).
+  std::map<const network::SimpleURLLoader*,
+           std::pair<std::unique_ptr<network::SimpleURLLoader>, std::string>>
       requests_;
 
   // Callbacks by extension ID.

chrome/browser/extensions/blacklist_state_fetcher_unittest.cc

@@ -34,7 +34,7 @@ TEST_F(BlacklistStateFetcherTest, RequestBlacklistState) {
   BlacklistState result;
   fetcher.Request("a", base::Bind(&Assign, &result));
 
-  EXPECT_TRUE(tester.HandleFetcher(0));
+  EXPECT_TRUE(tester.HandleFetcher("a"));
   EXPECT_EQ(BLACKLISTED_SECURITY_VULNERABILITY, result);
 }
 
@@ -65,12 +65,12 @@ TEST_F(BlacklistStateFetcherTest, RequestMultipleBlacklistStates) {
   fetcher.Request("f", base::Bind(&Assign, &result[8]));
 
   // 6 fetchers should be created. Sending responses in shuffled order.
-  EXPECT_TRUE(tester.HandleFetcher(4));
-  EXPECT_TRUE(tester.HandleFetcher(2));
-  EXPECT_TRUE(tester.HandleFetcher(5));
-  EXPECT_TRUE(tester.HandleFetcher(1));
-  EXPECT_TRUE(tester.HandleFetcher(0));
-  EXPECT_TRUE(tester.HandleFetcher(3));
+  EXPECT_TRUE(tester.HandleFetcher("e"));
+  EXPECT_TRUE(tester.HandleFetcher("c"));
+  EXPECT_TRUE(tester.HandleFetcher("f"));
+  EXPECT_TRUE(tester.HandleFetcher("b"));
+  EXPECT_TRUE(tester.HandleFetcher("a"));
+  EXPECT_TRUE(tester.HandleFetcher("d"));
 
   EXPECT_EQ(NOT_BLACKLISTED, result[0]);
   EXPECT_EQ(NOT_BLACKLISTED, result[1]);

chrome/browser/extensions/blacklist_unittest.cc

@@ -169,8 +169,8 @@ TEST_F(BlacklistTest, FetchBlacklistStates) {
   base::RunLoop().RunUntilIdle();
 
    // Two fetchers should be created.
-  EXPECT_TRUE(fetcher_tester.HandleFetcher(0));
-  EXPECT_TRUE(fetcher_tester.HandleFetcher(1));
+  EXPECT_TRUE(fetcher_tester.HandleFetcher(a));
+  EXPECT_TRUE(fetcher_tester.HandleFetcher(b));
 
   EXPECT_EQ(BLACKLISTED_CWS_POLICY_VIOLATION, states[a]);
   EXPECT_EQ(BLACKLISTED_POTENTIALLY_UNWANTED, states[b]);
@@ -184,7 +184,7 @@ TEST_F(BlacklistTest, FetchBlacklistStates) {
   base::RunLoop().RunUntilIdle();
 
   // No new fetchers.
-  EXPECT_FALSE(fetcher_tester.HandleFetcher(2));
+  EXPECT_FALSE(fetcher_tester.HandleFetcher(c));
   EXPECT_EQ(BLACKLISTED_CWS_POLICY_VIOLATION, cached_states[a]);
   EXPECT_EQ(BLACKLISTED_POTENTIALLY_UNWANTED, cached_states[b]);
   EXPECT_EQ(0U, cached_states.count(c));

chrome/browser/extensions/test_blacklist_state_fetcher.cc

@@ -6,7 +6,7 @@
 
 #include "base/stl_util.h"
 #include "base/threading/thread_task_runner_handle.h"
-#include "net/url_request/url_request_test_util.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 
 namespace extensions {
 namespace {
@@ -29,15 +29,44 @@ safe_browsing::SafeBrowsingProtocolConfig CreateSafeBrowsingProtocolConfig() {
   return config;
 }
 
+class DummySharedURLLoaderFactory : public network::SharedURLLoaderFactory {
+ public:
+  DummySharedURLLoaderFactory() {}
+
+  std::unique_ptr<network::SharedURLLoaderFactoryInfo> Clone() override {
+    NOTREACHED();
+    return nullptr;
+  }
+
+  // network::URLLoaderFactory implementation:
+  void CreateLoaderAndStart(network::mojom::URLLoaderRequest loader,
+                            int32_t routing_id,
+                            int32_t request_id,
+                            uint32_t options,
+                            const network::ResourceRequest& request,
+                            network::mojom::URLLoaderClientPtr client,
+                            const net::MutableNetworkTrafficAnnotationTag&
+                                traffic_annotation) override {
+    // Ensure the client pipe doesn't get closed to avoid SimpleURLLoader seeing
+    // a connection error.
+    clients_.push_back(std::move(client));
+  }
+
+ private:
+  friend class base::RefCounted<DummySharedURLLoaderFactory>;
+  ~DummySharedURLLoaderFactory() override = default;
+
+  std::vector<network::mojom::URLLoaderClientPtr> clients_;
+};
+
 }  // namespace
 
 TestBlacklistStateFetcher::TestBlacklistStateFetcher(
     BlacklistStateFetcher* fetcher) : fetcher_(fetcher) {
   fetcher_->SetSafeBrowsingConfig(CreateSafeBrowsingProtocolConfig());
-  scoped_refptr<net::TestURLRequestContextGetter> context =
-        new net::TestURLRequestContextGetter(
-            base::ThreadTaskRunnerHandle::Get());
-  fetcher_->SetURLRequestContextForTest(context.get());
+
+  url_loader_factory_ = base::MakeRefCounted<DummySharedURLLoaderFactory>();
+  fetcher_->url_loader_factory_ = url_loader_factory_.get();
 }
 
 TestBlacklistStateFetcher::~TestBlacklistStateFetcher() {
@@ -48,19 +77,18 @@ void TestBlacklistStateFetcher::SetBlacklistVerdict(
   verdicts_[id] = state;
 }
 
-bool TestBlacklistStateFetcher::HandleFetcher(int fetcher_id) {
-  net::TestURLFetcher* url_fetcher = url_fetcher_factory_.GetFetcherByID(
-      fetcher_id);
-  if (!url_fetcher)
-    return false;
+bool TestBlacklistStateFetcher::HandleFetcher(const std::string& id) {
+  network::SimpleURLLoader* url_loader = nullptr;
+  for (auto& it : fetcher_->requests_) {
+    if (it.second.second == id) {
+      url_loader = it.second.first.get();
+      break;
+    }
+  }
 
-  const std::string& request_str = url_fetcher->upload_data();
-  ClientCRXListInfoRequest request;
-  if (!request.ParseFromString(request_str))
+  if (!url_loader)
     return false;
 
-  std::string id = request.id();
-
   ClientCRXListInfoResponse response;
   if (base::ContainsKey(verdicts_, id))
     response.set_verdict(verdicts_[id]);
@@ -70,10 +98,7 @@ bool TestBlacklistStateFetcher::HandleFetcher(int fetcher_id) {
   std::string response_str;
   response.SerializeToString(&response_str);
 
-  url_fetcher->set_status(net::URLRequestStatus());
-  url_fetcher->set_response_code(200);
-  url_fetcher->SetResponseString(response_str);
-  url_fetcher->delegate()->OnURLFetchComplete(url_fetcher);
+  fetcher_->OnURLLoaderCompleteInternal(url_loader, response_str, 200, net::OK);
 
   return true;
 }

chrome/browser/extensions/test_blacklist_state_fetcher.h

@@ -10,7 +10,6 @@
 #include "base/macros.h"
 #include "chrome/browser/extensions/blacklist_state_fetcher.h"
 #include "chrome/common/safe_browsing/crx_info.pb.h"
-#include "net/url_request/test_url_fetcher_factory.h"
 
 namespace extensions {
 
@@ -24,17 +23,18 @@ class TestBlacklistStateFetcher {
   void SetBlacklistVerdict(const std::string& id,
                            ClientCRXListInfoResponse_Verdict state);
 
-  // Get URLFetcher by its id from factory, send the appropriate response.
+  // Send the appropriate response for the request for extension with id |id|.
   // Return false, if fetcher with fiven id doesn't exist or in case of
   // incorrect request. Otherwise return true.
-  bool HandleFetcher(int id);
+  bool HandleFetcher(const std::string& id);
 
  private:
   BlacklistStateFetcher* fetcher_;
 
   std::map<std::string, ClientCRXListInfoResponse_Verdict> verdicts_;
 
-  net::TestURLFetcherFactory url_fetcher_factory_;
+  // Dummy URLLoaderFactory not used for responses but avoids crashes.
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(TestBlacklistStateFetcher);
 };