[ads][CodeHealth] Network client (#32614)

components/brave_ads/core/browser/service/network_client.cc

@@ -12,7 +12,6 @@
 #include "base/strings/string_util.h"
 #include "brave/components/brave_ads/core/browser/service/network_client_util.h"
 #include "brave/components/brave_ads/core/mojom/brave_ads.mojom.h"
-#include "net/base/net_errors.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"
@@ -45,18 +44,21 @@ NetworkClient::~NetworkClient() = default;
 
 void NetworkClient::SendRequest(mojom::UrlRequestInfoPtr mojom_url_request,
                                 SendRequestCallback callback) {
+  CHECK(mojom_url_request);
+
   return HttpRequest(std::move(mojom_url_request), std::move(callback));
 }
 
 void NetworkClient::CancelRequests() {
   weak_ptr_factory_.InvalidateWeakPtrs();
-  url_loaders_.clear();
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 
 void NetworkClient::HttpRequest(mojom::UrlRequestInfoPtr mojom_url_request,
                                 SendRequestCallback callback) {
+  CHECK(mojom_url_request);
+
   auto resource_request = std::make_unique<network::ResourceRequest>();
   resource_request->url = mojom_url_request->url;
   resource_request->method = ToString(mojom_url_request->method);
@@ -68,49 +70,43 @@ void NetworkClient::HttpRequest(mojom::UrlRequestInfoPtr mojom_url_request,
 
   auto url_loader = network::SimpleURLLoader::Create(
       std::move(resource_request), GetNetworkTrafficAnnotationTag());
+  auto* raw_url_loader = url_loader.get();
 
-  url_loader->SetAllowHttpErrorResults(true);
+  raw_url_loader->SetAllowHttpErrorResults(true);
 
   if (!mojom_url_request->content.empty()) {
-    url_loader->AttachStringForUpload(mojom_url_request->content,
-                                      mojom_url_request->content_type);
+    raw_url_loader->AttachStringForUpload(mojom_url_request->content,
+                                          mojom_url_request->content_type);
   }
 
-  auto* url_loader_copy = url_loader.get();
-  url_loaders_.insert(std::move(url_loader));
-
-  url_loader_copy->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+  raw_url_loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
       url_loader_factory_.get(),
       base::BindOnce(&NetworkClient::HttpRequestCallback,
-                     weak_ptr_factory_.GetWeakPtr(), url_loader_copy,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(url_loader),
                      std::move(callback)));
 }
 
 void NetworkClient::HttpRequestCallback(
-    network::SimpleURLLoader* url_loader,
+    std::unique_ptr<network::SimpleURLLoader> url_loader,
     SendRequestCallback callback,
     std::optional<std::string> response_body) {
   CHECK(url_loader);
 
-  auto iter = url_loaders_.find(url_loader);
-  CHECK(iter != url_loaders_.cend());
-  auto owned_url_loader = std::move(*iter);
-  url_loaders_.erase(iter);
+  const GURL& url = url_loader->GetFinalURL();
 
-  const GURL& url = owned_url_loader->GetFinalURL();
-
-  const auto* response = owned_url_loader->ResponseInfo();
+  const auto* response = url_loader->ResponseInfo();
   if (!response || !response->headers) {
-    return ReportError(url, net::ERR_FAILED, std::move(callback));
+    // DNS failure, connection error, timeout etc.
+    return ReportError(url, url_loader->NetError(), std::move(callback));
   }
 
-  const auto response_headers = response->headers;
-
   auto mojom_url_response = mojom::UrlResponseInfo::New();
   mojom_url_response->url = url;
-  mojom_url_response->code = response_headers->response_code();
+  mojom_url_response->code = response->headers->response_code();
   mojom_url_response->body = response_body.value_or("");
-  mojom_url_response->headers = ExtractHttpResponseHeaders(response_headers);
+  mojom_url_response->headers = ExtractHttpResponseHeaders(response->headers);
+
+  // Forward the response to the original caller for handling.
   std::move(callback).Run(std::move(mojom_url_response));
 }
 

components/brave_ads/core/browser/service/network_client.h

@@ -10,8 +10,6 @@
 #include <optional>
 #include <string>
 
-#include "base/containers/flat_set.h"
-#include "base/containers/unique_ptr_adapters.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "brave/components/brave_ads/core/browser/service/network_client_callback.h"
@@ -25,7 +23,8 @@ class SharedURLLoaderFactory;
 
 namespace brave_ads {
 
-// This class is responsible for sending HTTP network requests.
+// Sends network requests, supporting standard HTTP. Standard HTTP requests are
+// issued via `SimpleURLLoader`.
 class NetworkClient {
  public:
   NetworkClient(
@@ -37,28 +36,26 @@ class NetworkClient {
 
   ~NetworkClient();
 
-  // Starts a network request for the given `mojom::UrlRequestInfo`. The
-  // provided `callback` will be invoked with a `mojom::UrlResponseInfo` unless
-  // the request is canceled or the `NetworkClient` is destroyed.
+  // Issues a network request described by `mojom::UrlRequestInfo`. When the
+  // request completes, `callback` is invoked with a `mojom::UrlResponseInfo`.
+  // The callback will not run if the request is canceled or if this instance
+  // is destroyed.
   void SendRequest(mojom::UrlRequestInfoPtr mojom_url_request,
                    SendRequestCallback callback);
 
-  // Cancels all ongoing network requests. Pending callbacks will not be
-  // invoked.
+  // Cancels all active requests. Any pending callbacks will be dropped.
   void CancelRequests();
 
  private:
+  // Sends the request using standard HTTP.
   void HttpRequest(mojom::UrlRequestInfoPtr mojom_url_request,
                    SendRequestCallback callback);
-  void HttpRequestCallback(network::SimpleURLLoader* url_loader,
+  void HttpRequestCallback(std::unique_ptr<network::SimpleURLLoader> url_loader,
                            SendRequestCallback callback,
                            std::optional<std::string> response_body);
 
   const scoped_refptr<network::SharedURLLoaderFactory>
       url_loader_factory_;  // Not owned.
-  base::flat_set<std::unique_ptr<network::SimpleURLLoader>,
-                 base::UniquePtrComparator>
-      url_loaders_;
 
   const network::NetworkContextGetter network_context_getter_;
 

components/brave_ads/core/browser/service/network_client_util.h

@@ -22,11 +22,12 @@ namespace brave_ads {
 // Converts `mojom::UrlRequestMethodType` to its string representation.
 std::string ToString(mojom::UrlRequestMethodType value);
 
-// Returns the network traffic annotation tag for network requests.
+// Returns the network traffic annotation tag used to identify and audit network
+// requests.
 net::NetworkTrafficAnnotationTag GetNetworkTrafficAnnotationTag();
 
-// Extracts all HTTP response headers from a `net::HttpResponseHeaders` object
-// and returns them as a flat map with lowercased keys.
+// Extracts all HTTP response headers from `net::HttpResponseHeaders` and
+// returns them as a flat map with lowercased keys.
 base::flat_map<std::string, std::string> ExtractHttpResponseHeaders(
     const scoped_refptr<net::HttpResponseHeaders>& http_response_headers);