Migrate SafeSearchURLChecker to SimpleURLLoader

Bug: 773295
Change-Id: I6be6adefab29b5d4e67013f45ba5ffe4e058dbdc
Reviewed-on: https://chromium-review.googlesource.com/1030062Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Mark Pilgrim <pilgrim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558725}

chrome/browser/safe_search_api/safe_search_url_checker.cc

@@ -20,15 +20,12 @@
 #include "google_apis/google_api_keys.h"
 #include "net/base/escape.h"
 #include "net/base/load_flags.h"
-#include "net/url_request/url_fetcher.h"
-#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_status.h"
+#include "services/network/public/cpp/resource_request.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/simple_url_loader.h"
 #include "url/url_constants.h"
 
-using net::URLFetcher;
-using net::URLFetcherDelegate;
-using net::URLRequestContextGetter;
-using net::URLRequestStatus;
-
 namespace {
 
 const char kSafeSearchApiUrl[] =
@@ -45,23 +42,6 @@ std::string BuildRequestData(const std::string& api_key, const GURL& url) {
   return base::StringPrintf(kDataFormat, api_key.c_str(), query.c_str());
 }
 
-// Creates a URLFetcher to call the SafeSearch API for |url|.
-std::unique_ptr<net::URLFetcher> CreateFetcher(
-    URLFetcherDelegate* delegate,
-    URLRequestContextGetter* context,
-    const std::string& api_key,
-    const GURL& url,
-    const net::NetworkTrafficAnnotationTag& traffic_annotation) {
-  std::unique_ptr<net::URLFetcher> fetcher =
-      URLFetcher::Create(0, GURL(kSafeSearchApiUrl), URLFetcher::POST, delegate,
-                         traffic_annotation);
-  fetcher->SetUploadData(kDataContentType, BuildRequestData(api_key, url));
-  fetcher->SetRequestContext(context);
-  fetcher->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
-                        net::LOAD_DO_NOT_SAVE_COOKIES);
-  return fetcher;
-}
-
 // Parses a SafeSearch API |response| and stores the result in |is_porn|.
 // On errors, returns false and doesn't set |is_porn|.
 bool ParseResponse(const std::string& response, bool* is_porn) {
@@ -93,21 +73,22 @@ bool ParseResponse(const std::string& response, bool* is_porn) {
 
 struct SafeSearchURLChecker::Check {
   Check(const GURL& url,
-        std::unique_ptr<net::URLFetcher> fetcher,
+        std::unique_ptr<network::SimpleURLLoader> simple_url_loader,
         CheckCallback callback);
   ~Check();
 
   GURL url;
-  std::unique_ptr<net::URLFetcher> fetcher;
+  std::unique_ptr<network::SimpleURLLoader> simple_url_loader;
   std::vector<CheckCallback> callbacks;
   base::TimeTicks start_time;
 };
 
-SafeSearchURLChecker::Check::Check(const GURL& url,
-                                   std::unique_ptr<net::URLFetcher> fetcher,
-                                   CheckCallback callback)
+SafeSearchURLChecker::Check::Check(
+    const GURL& url,
+    std::unique_ptr<network::SimpleURLLoader> simple_url_loader,
+    CheckCallback callback)
     : url(url),
-      fetcher(std::move(fetcher)),
+      simple_url_loader(std::move(simple_url_loader)),
       start_time(base::TimeTicks::Now()) {
   callbacks.push_back(std::move(callback));
 }
@@ -125,15 +106,17 @@ SafeSearchURLChecker::CheckResult::CheckResult(Classification classification,
       timestamp(base::TimeTicks::Now()) {}
 
 SafeSearchURLChecker::SafeSearchURLChecker(
-    URLRequestContextGetter* context,
+    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
     const net::NetworkTrafficAnnotationTag& traffic_annotation)
-    : SafeSearchURLChecker(context, traffic_annotation, kDefaultCacheSize) {}
+    : SafeSearchURLChecker(std::move(url_loader_factory),
+                           traffic_annotation,
+                           kDefaultCacheSize) {}
 
 SafeSearchURLChecker::SafeSearchURLChecker(
-    URLRequestContextGetter* context,
+    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
     const net::NetworkTrafficAnnotationTag& traffic_annotation,
     size_t cache_size)
-    : context_(context),
+    : url_loader_factory_(std::move(url_loader_factory)),
       traffic_annotation_(traffic_annotation),
       cache_(cache_size),
       cache_timeout_(
@@ -183,48 +166,56 @@ bool SafeSearchURLChecker::CheckURL(const GURL& url, CheckCallback callback) {
 
   DVLOG(1) << "Checking URL " << url;
   std::string api_key = google_apis::GetAPIKey();
-  std::unique_ptr<URLFetcher> fetcher(
-      CreateFetcher(this, context_, api_key, url, traffic_annotation_));
-  fetcher->Start();
-  checks_in_progress_.push_back(
-      std::make_unique<Check>(url, std::move(fetcher), std::move(callback)));
+  auto resource_request = std::make_unique<network::ResourceRequest>();
+  resource_request->url = GURL(kSafeSearchApiUrl);
+  resource_request->method = "POST";
+  resource_request->load_flags =
+      net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES;
+  std::unique_ptr<network::SimpleURLLoader> simple_url_loader =
+      network::SimpleURLLoader::Create(std::move(resource_request),
+                                       traffic_annotation_);
+  simple_url_loader->AttachStringForUpload(BuildRequestData(api_key, url),
+                                           kDataContentType);
+  auto it = checks_in_progress_.insert(
+      checks_in_progress_.begin(),
+      std::make_unique<Check>(url, std::move(simple_url_loader),
+                              std::move(callback)));
+  network::SimpleURLLoader* loader = it->get()->simple_url_loader.get();
+  loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+      url_loader_factory_.get(),
+      base::BindOnce(&SafeSearchURLChecker::OnSimpleLoaderComplete,
+                     base::Unretained(this), std::move(it)));
   return false;
 }
 
-void SafeSearchURLChecker::OnURLFetchComplete(const net::URLFetcher* source) {
-  auto it = checks_in_progress_.begin();
-  while (it != checks_in_progress_.end()) {
-    if (source == (*it)->fetcher.get())
-      break;
-    ++it;
-  }
-  DCHECK(it != checks_in_progress_.end());
+void SafeSearchURLChecker::OnSimpleLoaderComplete(
+    CheckList::iterator it,
+    std::unique_ptr<std::string> response_body) {
   Check* check = it->get();
 
-  const URLRequestStatus& status = source->GetStatus();
-  if (!status.is_success()) {
+  GURL url = check->url;
+  std::vector<CheckCallback> callbacks = std::move(check->callbacks);
+  base::TimeTicks start_time = check->start_time;
+  checks_in_progress_.erase(it);
+
+  if (!response_body) {
     DLOG(WARNING) << "URL request failed! Letting through...";
-    for (size_t i = 0; i < check->callbacks.size(); i++)
-      std::move(check->callbacks[i])
-          .Run(check->url, Classification::SAFE, true);
-    checks_in_progress_.erase(it);
+    for (size_t i = 0; i < callbacks.size(); i++)
+      std::move(callbacks[i]).Run(url, Classification::SAFE, true);
     return;
   }
 
-  std::string response_body;
-  source->GetResponseAsString(&response_body);
   bool is_porn = false;
-  bool uncertain = !ParseResponse(response_body, &is_porn);
+  bool uncertain = !ParseResponse(*response_body, &is_porn);
   Classification classification =
       is_porn ? Classification::UNSAFE : Classification::SAFE;
 
   // TODO(msramek): Consider moving this to SupervisedUserResourceThrottle.
   UMA_HISTOGRAM_TIMES("ManagedUsers.SafeSitesDelay",
-                      base::TimeTicks::Now() - check->start_time);
+                      base::TimeTicks::Now() - start_time);
 
-  cache_.Put(check->url, CheckResult(classification, uncertain));
+  cache_.Put(url, CheckResult(classification, uncertain));
 
-  for (size_t i = 0; i < check->callbacks.size(); i++)
-    std::move(check->callbacks[i]).Run(check->url, classification, uncertain);
-  checks_in_progress_.erase(it);
+  for (size_t i = 0; i < callbacks.size(); i++)
+    std::move(callbacks[i]).Run(url, classification, uncertain);
 }

chrome/browser/safe_search_api/safe_search_url_checker.h

@@ -13,20 +13,19 @@
 #include "base/callback_forward.h"
 #include "base/containers/mru_cache.h"
 #include "base/macros.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/time/time.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
-#include "net/url_request/url_fetcher_delegate.h"
 #include "url/gurl.h"
 
-namespace net {
-class URLFetcher;
-class URLRequestContextGetter;
-}
+namespace network {
+class SharedURLLoaderFactory;
+}  // namespace network
 
 // This class uses the SafeSearch API to check the SafeSearch classification
 // of the content on a given URL and returns the result asynchronously
 // via a callback.
-class SafeSearchURLChecker : net::URLFetcherDelegate {
+class SafeSearchURLChecker {
  public:
   enum class Classification { SAFE, UNSAFE };
 
@@ -35,13 +34,13 @@ class SafeSearchURLChecker : net::URLFetcherDelegate {
       void(const GURL&, Classification classification, bool /* uncertain */)>;
 
   explicit SafeSearchURLChecker(
-      net::URLRequestContextGetter* context,
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
       const net::NetworkTrafficAnnotationTag& traffic_annotation);
   SafeSearchURLChecker(
-      net::URLRequestContextGetter* context,
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
       const net::NetworkTrafficAnnotationTag& traffic_annotation,
       size_t cache_size);
-  ~SafeSearchURLChecker() override;
+  ~SafeSearchURLChecker();
 
   // Returns whether |callback| was run synchronously.
   bool CheckURL(const GURL& url, CheckCallback callback);
@@ -58,14 +57,15 @@ class SafeSearchURLChecker : net::URLFetcherDelegate {
     bool uncertain;
     base::TimeTicks timestamp;
   };
+  using CheckList = std::list<std::unique_ptr<Check>>;
 
-  // net::URLFetcherDelegate implementation.
-  void OnURLFetchComplete(const net::URLFetcher* source) override;
+  void OnSimpleLoaderComplete(CheckList::iterator it,
+                              std::unique_ptr<std::string> response_body);
 
-  net::URLRequestContextGetter* context_;
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
   const net::NetworkTrafficAnnotationTag traffic_annotation_;
 
-  std::vector<std::unique_ptr<Check>> checks_in_progress_;
+  CheckList checks_in_progress_;
 
   base::MRUCache<GURL, CheckResult> cache_;
   base::TimeDelta cache_timeout_;

chrome/browser/safe_search_api/safe_search_url_checker_unittest.cc

@@ -18,8 +18,9 @@
 #include "base/values.h"
 #include "net/base/net_errors.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
-#include "net/url_request/test_url_fetcher_factory.h"
-#include "net/url_request/url_request_test_util.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
+#include "services/network/test/test_url_loader_factory.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
@@ -31,8 +32,6 @@ namespace {
 
 const size_t kCacheSize = 2;
 
-const int kSafeSearchURLCheckerURLFetcherID = 0;
-
 const char* kURLs[] = {
     "http://www.randomsite1.com",
     "http://www.randomsite2.com",
@@ -45,6 +44,9 @@ const char* kURLs[] = {
     "http://www.randomsite9.com",
 };
 
+const char kSafeSearchApiUrl[] =
+    "https://safesearch.googleapis.com/v1:classify";
+
 std::string BuildResponse(bool is_porn) {
   base::DictionaryValue dict;
   std::unique_ptr<base::DictionaryValue> classification_dict(
@@ -66,9 +68,10 @@ class SafeSearchURLCheckerTest : public testing::Test {
  public:
   SafeSearchURLCheckerTest()
       : next_url_(0),
-        request_context_(new net::TestURLRequestContextGetter(
-            base::ThreadTaskRunnerHandle::Get())),
-        checker_(request_context_.get(),
+        test_shared_loader_factory_(
+            base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
+                &test_url_loader_factory_)),
+        checker_(test_shared_loader_factory_,
                  TRAFFIC_ANNOTATION_FOR_TESTS,
                  kCacheSize) {}
 
@@ -83,59 +86,62 @@ class SafeSearchURLCheckerTest : public testing::Test {
     return GURL(kURLs[next_url_++]);
   }
 
+  void SetupResponse(const GURL& url,
+                     net::Error error,
+                     const std::string& response) {
+    network::URLLoaderCompletionStatus status(error);
+    status.decoded_body_length = response.size();
+    test_url_loader_factory_.AddResponse(GURL(kSafeSearchApiUrl),
+                                         network::ResourceResponseHead(),
+                                         response, status);
+  }
+
   // Returns true if the result was returned synchronously (cache hit).
   bool CheckURL(const GURL& url) {
-    return checker_.CheckURL(url,
-                             base::Bind(&SafeSearchURLCheckerTest::OnCheckDone,
-                                        base::Unretained(this)));
+    bool cached = checker_.CheckURL(
+        url, base::BindOnce(&SafeSearchURLCheckerTest::OnCheckDone,
+                            base::Unretained(this)));
+    return cached;
   }
 
-  net::TestURLFetcher* GetURLFetcher() {
-    net::TestURLFetcher* url_fetcher =
-        url_fetcher_factory_.GetFetcherByID(kSafeSearchURLCheckerURLFetcherID);
-    EXPECT_TRUE(url_fetcher);
-    return url_fetcher;
-  }
+  void WaitForResponse() { base::RunLoop().RunUntilIdle(); }
 
-  void SendResponse(net::Error error, const std::string& response) {
-    net::TestURLFetcher* url_fetcher = GetURLFetcher();
-    url_fetcher->set_status(net::URLRequestStatus::FromError(error));
-    url_fetcher->set_response_code(net::HTTP_OK);
-    url_fetcher->SetResponseString(response);
-    url_fetcher->delegate()->OnURLFetchComplete(url_fetcher);
+  bool SendValidResponse(const GURL& url, bool is_porn) {
+    SetupResponse(url, net::OK, BuildResponse(is_porn));
+    bool result = CheckURL(url);
+    WaitForResponse();
+    return result;
   }
 
-  void SendValidResponse(bool is_porn) {
-    SendResponse(net::OK, BuildResponse(is_porn));
+  bool SendFailedResponse(const GURL& url) {
+    SetupResponse(url, net::ERR_ABORTED, std::string());
+    bool result = CheckURL(url);
+    WaitForResponse();
+    return result;
   }
 
-  void SendFailedResponse() { SendResponse(net::ERR_ABORTED, std::string()); }
-
   size_t next_url_;
   base::MessageLoop message_loop_;
-  scoped_refptr<net::TestURLRequestContextGetter> request_context_;
-  net::TestURLFetcherFactory url_fetcher_factory_;
+  network::TestURLLoaderFactory test_url_loader_factory_;
+  scoped_refptr<network::SharedURLLoaderFactory> test_shared_loader_factory_;
   SafeSearchURLChecker checker_;
 };
 
 TEST_F(SafeSearchURLCheckerTest, Simple) {
   {
     GURL url(GetNewURL());
-    ASSERT_FALSE(CheckURL(url));
     EXPECT_CALL(*this, OnCheckDone(url, Classification::SAFE, false));
-    SendValidResponse(false);
+    ASSERT_FALSE(SendValidResponse(url, false));
   }
   {
     GURL url(GetNewURL());
-    ASSERT_FALSE(CheckURL(url));
     EXPECT_CALL(*this, OnCheckDone(url, Classification::UNSAFE, false));
-    SendValidResponse(true);
+    ASSERT_FALSE(SendValidResponse(url, true));
   }
   {
     GURL url(GetNewURL());
-    ASSERT_FALSE(CheckURL(url));
     EXPECT_CALL(*this, OnCheckDone(url, Classification::SAFE, true));
-    SendFailedResponse();
+    ASSERT_FALSE(SendFailedResponse(url));
   }
 }
 
@@ -147,37 +153,35 @@ TEST_F(SafeSearchURLCheckerTest, Cache) {
   GURL url3(GetNewURL());
 
   // Populate the cache.
-  ASSERT_FALSE(CheckURL(url1));
   EXPECT_CALL(*this, OnCheckDone(url1, Classification::SAFE, false));
-  SendValidResponse(false);
-  ASSERT_FALSE(CheckURL(url2));
+  ASSERT_FALSE(SendValidResponse(url1, false));
   EXPECT_CALL(*this, OnCheckDone(url2, Classification::SAFE, false));
-  SendValidResponse(false);
+  ASSERT_FALSE(SendValidResponse(url2, false));
 
-  // Now we should get results synchronously.
+  // Now we should get results synchronously, without a network request.
+  test_url_loader_factory_.ClearResponses();
   EXPECT_CALL(*this, OnCheckDone(url2, Classification::SAFE, false));
   ASSERT_TRUE(CheckURL(url2));
   EXPECT_CALL(*this, OnCheckDone(url1, Classification::SAFE, false));
   ASSERT_TRUE(CheckURL(url1));
 
   // Now |url2| is the LRU and should be evicted on the next check.
-  ASSERT_FALSE(CheckURL(url3));
   EXPECT_CALL(*this, OnCheckDone(url3, Classification::SAFE, false));
-  SendValidResponse(false);
+  ASSERT_FALSE(SendValidResponse(url3, false));
 
-  ASSERT_FALSE(CheckURL(url2));
   EXPECT_CALL(*this, OnCheckDone(url2, Classification::SAFE, false));
-  SendValidResponse(false);
+  ASSERT_FALSE(SendValidResponse(url2, false));
 }
 
 TEST_F(SafeSearchURLCheckerTest, CoalesceRequestsToSameURL) {
   GURL url(GetNewURL());
   // Start two checks for the same URL.
+  SetupResponse(url, net::OK, BuildResponse(false));
   ASSERT_FALSE(CheckURL(url));
   ASSERT_FALSE(CheckURL(url));
-  // A single response should answer both checks.
+  // A single response should answer both of those checks
   EXPECT_CALL(*this, OnCheckDone(url, Classification::SAFE, false)).Times(2);
-  SendValidResponse(false);
+  WaitForResponse();
 }
 
 TEST_F(SafeSearchURLCheckerTest, CacheTimeout) {
@@ -185,13 +189,11 @@ TEST_F(SafeSearchURLCheckerTest, CacheTimeout) {
 
   checker_.SetCacheTimeoutForTesting(base::TimeDelta::FromSeconds(0));
 
-  ASSERT_FALSE(CheckURL(url));
   EXPECT_CALL(*this, OnCheckDone(url, Classification::SAFE, false));
-  SendValidResponse(false);
+  ASSERT_FALSE(SendValidResponse(url, false));
 
   // Since the cache timeout is zero, the cache entry should be invalidated
   // immediately.
-  ASSERT_FALSE(CheckURL(url));
   EXPECT_CALL(*this, OnCheckDone(url, Classification::UNSAFE, false));
-  SendValidResponse(true);
+  ASSERT_FALSE(SendValidResponse(url, true));
 }

chrome/browser/supervised_user/supervised_user_service.cc

@@ -45,6 +45,7 @@
 #include "content/public/browser/storage_partition.h"
 #include "extensions/buildflags/buildflags.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "ui/base/l10n/l10n_util.h"
 
 #if !defined(OS_ANDROID)
@@ -588,7 +589,9 @@ void SupervisedUserService::OnSafeSitesSettingChanged() {
       supervised_users::IsSafeSitesOnlineCheckEnabled(profile_);
   if (use_online_check != url_filter_.HasAsyncURLChecker()) {
     if (use_online_check)
-      url_filter_.InitAsyncURLChecker(profile_->GetRequestContext());
+      url_filter_.InitAsyncURLChecker(
+          content::BrowserContext::GetDefaultStoragePartition(profile_)
+              ->GetURLLoaderFactoryForBrowserProcess());
     else
       url_filter_.ClearAsyncURLChecker();
   }

chrome/browser/supervised_user/supervised_user_url_filter.cc

@@ -32,6 +32,7 @@
 #include "net/base/escape.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/base/url_util.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "url/gurl.h"
 #include "url/url_constants.h"
 
@@ -530,7 +531,7 @@ void SupervisedUserURLFilter::SetManualURLs(std::map<GURL, bool> url_map) {
 }
 
 void SupervisedUserURLFilter::InitAsyncURLChecker(
-    net::URLRequestContextGetter* context) {
+    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory) {
   net::NetworkTrafficAnnotationTag traffic_annotation =
       net::DefineNetworkTrafficAnnotation("supervised_user_url_filter", R"(
         semantics {
@@ -552,8 +553,8 @@ void SupervisedUserURLFilter::InitAsyncURLChecker(
             "family dashboard."
           policy_exception_justification: "Not implemented."
         })");
-  async_url_checker_.reset(
-      new SafeSearchURLChecker(context, traffic_annotation));
+  async_url_checker_.reset(new SafeSearchURLChecker(
+      std::move(url_loader_factory), traffic_annotation));
 }
 
 void SupervisedUserURLFilter::ClearAsyncURLChecker() {

chrome/browser/supervised_user/supervised_user_url_filter.h

@@ -28,9 +28,9 @@ namespace base {
 class TaskRunner;
 }
 
-namespace net {
-class URLRequestContextGetter;
-}
+namespace network {
+class SharedURLLoaderFactory;
+}  // namespace network
 
 // This class manages the filtering behavior for URLs, i.e. it tells callers
 // if a URL should be allowed, blocked or warned about. It uses information
@@ -160,7 +160,8 @@ class SupervisedUserURLFilter {
   void SetManualURLs(std::map<GURL, bool> url_map);
 
   // Initializes the experimental asynchronous checker.
-  void InitAsyncURLChecker(net::URLRequestContextGetter* context);
+  void InitAsyncURLChecker(
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
 
   // Clears any asynchronous checker.
   void ClearAsyncURLChecker();

chrome/browser/ui/webui/supervised_user_internals_message_handler.cc

@@ -27,6 +27,7 @@
 #include "components/supervised_user_error_page/supervised_user_error_page.h"
 #include "components/url_formatter/url_fixer.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/storage_partition.h"
 #include "content/public/browser/web_ui.h"
 
 using content::BrowserThread;