Simplify the update_client::NetworkFetcher interface.

Remove the public accessors, and bind the data in the callbacks instead.

Bug: 929167
Change-Id: Ib6b41bc33551d3a81f2226344e53325cace5cf87
Reviewed-on: https://chromium-review.googlesource.com/c/1479191Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#633868}

components/update_client/BUILD.gn

@@ -53,6 +53,7 @@ static_library("update_client") {
     "crx_downloader.cc",
     "crx_downloader.h",
     "crx_update_item.h",
+    "network.cc",
     "network.h",
     "persisted_data.cc",
     "persisted_data.h",

components/update_client/net/network_impl.cc

@@ -47,6 +47,37 @@ const net::NetworkTrafficAnnotationTag traffic_annotation =
           }
         })");
 
+// Returns the string value of a header of the server response or an empty
+// string if the header is not available. Only the first header is returned
+// if multiple instances of the same header are present.
+std::string GetStringHeader(const network::SimpleURLLoader* simple_url_loader,
+                            const char* header_name) {
+  DCHECK(simple_url_loader);
+
+  const auto* response_info = simple_url_loader->ResponseInfo();
+  if (!response_info || !response_info->headers)
+    return {};
+
+  std::string header_value;
+  return response_info->headers->EnumerateHeader(nullptr, header_name,
+                                                 &header_value)
+             ? header_value
+             : std::string{};
+}
+
+// Returns the integral value of a header of the server response or -1 if
+// if the header is not available or a conversion error has occured.
+int64_t GetInt64Header(const network::SimpleURLLoader* simple_url_loader,
+                       const char* header_name) {
+  DCHECK(simple_url_loader);
+
+  const auto* response_info = simple_url_loader->ResponseInfo();
+  if (!response_info || !response_info->headers)
+    return -1;
+
+  return response_info->headers->GetInt64HeaderValue(header_name);
+}
+
 }  // namespace
 
 namespace update_client {
@@ -83,7 +114,17 @@ void NetworkFetcherImpl::PostRequest(
   constexpr size_t kMaxResponseSize = 1024 * 1024;
   simple_url_loader_->DownloadToString(
       shared_url_network_factory_.get(),
-      std::move(post_request_complete_callback), kMaxResponseSize);
+      base::BindOnce(
+          [](const network::SimpleURLLoader* simple_url_loader,
+             PostRequestCompleteCallback post_request_complete_callback,
+             std::unique_ptr<std::string> response_body) {
+            std::move(post_request_complete_callback)
+                .Run(std::move(response_body), simple_url_loader->NetError(),
+                     GetStringHeader(simple_url_loader, kHeaderEtag),
+                     GetInt64Header(simple_url_loader, kHeaderXRetryAfter));
+          },
+          simple_url_loader_.get(), std::move(post_request_complete_callback)),
+      kMaxResponseSize);
 }
 
 void NetworkFetcherImpl::DownloadToFile(
@@ -109,42 +150,17 @@ void NetworkFetcherImpl::DownloadToFile(
       std::move(response_started_callback)));
   simple_url_loader_->DownloadToFile(
       shared_url_network_factory_.get(),
-      std::move(download_to_file_complete_callback), file_path);
-}
-
-int64_t NetworkFetcherImpl::GetContentSize() const {
-  DCHECK(simple_url_loader_);
-  return simple_url_loader_->GetContentSize();
-}
-
-int NetworkFetcherImpl::NetError() const {
-  DCHECK(simple_url_loader_);
-  return simple_url_loader_->NetError();
-}
-
-std::string NetworkFetcherImpl::GetStringHeaderValue(
-    const char* header_name) const {
-  DCHECK(simple_url_loader_);
-
-  const auto* response_info = simple_url_loader_->ResponseInfo();
-  if (!response_info || !response_info->headers)
-    return {};
-
-  std::string header_value;
-  return response_info->headers->EnumerateHeader(nullptr, header_name,
-                                                 &header_value)
-             ? header_value
-             : std::string{};
-}
-
-int64_t NetworkFetcherImpl::GetInt64HeaderValue(const char* header_name) const {
-  DCHECK(simple_url_loader_);
-
-  const auto* response_info = simple_url_loader_->ResponseInfo();
-  if (!response_info || !response_info->headers)
-    return -1;
-
-  return response_info->headers->GetInt64HeaderValue(header_name);
+      base::BindOnce(
+          [](const network::SimpleURLLoader* simple_url_loader,
+             DownloadToFileCompleteCallback download_to_file_complete_callback,
+             base::FilePath file_path) {
+            std::move(download_to_file_complete_callback)
+                .Run(file_path, simple_url_loader->NetError(),
+                     simple_url_loader->GetContentSize());
+          },
+          simple_url_loader_.get(),
+          std::move(download_to_file_complete_callback)),
+      file_path);
 }
 
 void NetworkFetcherImpl::OnResponseStartedCallback(

components/update_client/net/network_impl.h

@@ -40,11 +40,6 @@ class NetworkFetcherImpl : public NetworkFetcher {
                       ResponseStartedCallback response_started_callback,
                       DownloadToFileCompleteCallback
                           download_to_file_complete_callback) override;
-  int NetError() const override;
-  std::string GetStringHeaderValue(const char* header_name) const override;
-  int64_t GetInt64HeaderValue(const char* header_name) const override;
-  int64_t GetContentSize() const override;
-
  private:
   void OnResponseStartedCallback(
       ResponseStartedCallback response_started_callback,

components/update_client/network.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/update_client/network.h"
+
+namespace update_client {
+
+constexpr char NetworkFetcher::kHeaderEtag[];
+constexpr char NetworkFetcher::kHeaderXRetryAfter[];
+
+}  // namespace update_client

components/update_client/network.h

@@ -26,12 +26,27 @@ namespace update_client {
 class NetworkFetcher {
  public:
   using PostRequestCompleteCallback =
-      base::OnceCallback<void(std::unique_ptr<std::string> response_body)>;
-  using DownloadToFileCompleteCallback =
-      base::OnceCallback<void(base::FilePath path)>;
+      base::OnceCallback<void(std::unique_ptr<std::string> response_body,
+                              int net_error,
+                              const std::string& header_etag,
+                              int64_t xheader_retry_after_sec)>;
+  using DownloadToFileCompleteCallback = base::OnceCallback<
+      void(base::FilePath path, int net_error, int64_t content_size)>;
   using ResponseStartedCallback = base::OnceCallback<
       void(const GURL& final_url, int response_code, int64_t content_length)>;
 
+  // The ETag header carries the ECSDA signature of the POST response, if
+  // signing has been used.
+  static constexpr char kHeaderEtag[] = "ETag";
+
+  // The server uses the optional X-Retry-After header to indicate that the
+  // current request should not be attempted again.
+  //
+  // The value of the header is the number of seconds to wait before trying to
+  // do a subsequent update check. Only the values retrieved over HTTPS are
+  // trusted.
+  static constexpr char kHeaderXRetryAfter[] = "X-Retry-After";
+
   virtual ~NetworkFetcher() = default;
 
   virtual void PostRequest(
@@ -45,17 +60,6 @@ class NetworkFetcher {
       const base::FilePath& file_path,
       ResponseStartedCallback response_started_callback,
       DownloadToFileCompleteCallback download_to_file_complete_callback) = 0;
-  virtual int NetError() const = 0;
-
-  // Returns the string value of a header of the server response or an empty
-  // string if the header is not available. Only the first header is returned
-  // if multiple instances of the same header are present.
-  virtual std::string GetStringHeaderValue(const char* header_name) const = 0;
-
-  // Returns the integral value of a header of the server response or -1 if
-  // if the header is not available or a conversion error has occured.
-  virtual int64_t GetInt64HeaderValue(const char* header_name) const = 0;
-  virtual int64_t GetContentSize() const = 0;
 
  protected:
   NetworkFetcher() = default;

components/update_client/request_sender.cc

@@ -4,13 +4,13 @@
 
 #include "components/update_client/request_sender.h"
 
-#include <algorithm>
 #include <utility>
 
 #include "base/base64.h"
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/logging.h"
+#include "base/numerics/safe_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/client_update_protocol/ecdsa.h"
@@ -29,25 +29,6 @@ const char kKeyPubBytesBase64[] =
     "MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEsVwVMmIJaWBjktSx9m1JrZWYBvMm"
     "bsrGGQPhScDtao+DloD871YmEeunAaQvRMZgDh1nCaWkVG6wo75+yDbKDA==";
 
-// The ETag header carries the ECSDA signature of the protocol response, if
-// signing has been used.
-constexpr const char* kHeaderEtag = "ETag";
-
-// The server uses the optional X-Retry-After header to indicate that the
-// current request should not be attempted again. Any response received along
-// with the X-Retry-After header should be interpreted as it would have been
-// without the X-Retry-After header.
-//
-// In addition to the presence of the header, the value of the header is
-// used as a signal for when to do future update checks, but only when the
-// response is over https. Values over http are not trusted and are ignored.
-//
-// The value of the header is the number of seconds to wait before trying to do
-// a subsequent update check. The upper bound for the number of seconds to wait
-// before trying to do a subsequent update check is capped at 24 hours.
-constexpr const char* kHeaderXRetryAfter = "X-Retry-After";
-constexpr int64_t kMaxRetryAfterSec = 24 * 60 * 60;
-
 }  // namespace
 
 RequestSender::RequestSender(scoped_refptr<Configurator> config)
@@ -165,33 +146,33 @@ void RequestSender::OnResponseStarted(const GURL& final_url,
 
 void RequestSender::OnNetworkFetcherComplete(
     const GURL& original_url,
-    std::unique_ptr<std::string> response_body) {
+    std::unique_ptr<std::string> response_body,
+    int net_error,
+    const std::string& header_etag,
+    int64_t xheader_retry_after_sec) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   VLOG(1) << "request completed from url: " << original_url.spec();
 
-  int fetch_error = -1;
+  int error = -1;
   if (response_body && response_code_ == 200) {
-    fetch_error = 0;
+    DCHECK_EQ(0, net_error);
+    error = 0;
   } else if (response_code_ != -1) {
-    fetch_error = response_code_;
+    error = response_code_;
   } else {
-    fetch_error = network_fetcher_->NetError();
+    error = net_error;
   }
 
-  int64_t retry_after_sec(-1);
-  if (original_url.SchemeIsCryptographic() && fetch_error > 0) {
-    retry_after_sec = network_fetcher_->GetInt64HeaderValue(kHeaderXRetryAfter);
-    retry_after_sec = std::min(retry_after_sec, kMaxRetryAfterSec);
-  }
+  int retry_after_sec = -1;
+  if (original_url.SchemeIsCryptographic() && error > 0)
+    retry_after_sec = base::saturated_cast<int>(xheader_retry_after_sec);
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::BindOnce(&RequestSender::SendInternalComplete,
-                     base::Unretained(this), fetch_error,
-                     response_body ? *response_body : std::string(),
-                     network_fetcher_->GetStringHeaderValue(kHeaderEtag),
-                     static_cast<int>(retry_after_sec)));
+      FROM_HERE, base::BindOnce(&RequestSender::SendInternalComplete,
+                                base::Unretained(this), error,
+                                response_body ? *response_body : std::string(),
+                                header_etag, retry_after_sec));
 }
 
 void RequestSender::HandleSendError(int error, int retry_after_sec) {

components/update_client/request_sender.h

@@ -70,7 +70,10 @@ class RequestSender {
                          int64_t content_length);
 
   void OnNetworkFetcherComplete(const GURL& original_url,
-                                std::unique_ptr<std::string> response_body);
+                                std::unique_ptr<std::string> response_body,
+                                int net_error,
+                                const std::string& header_etag,
+                                int64_t xheader_retry_after_sec);
 
   // Implements the error handling and url fallback mechanism.
   void SendInternal();

components/update_client/update_engine.cc

@@ -4,6 +4,7 @@
 
 #include "components/update_client/update_engine.h"
 
+#include <algorithm>
 #include <memory>
 #include <utility>
 
@@ -199,12 +200,12 @@ void UpdateEngine::UpdateCheckResultsAvailable(
 
   update_context->retry_after_sec = retry_after_sec;
 
-  const int throttle_sec(update_context->retry_after_sec);
-  DCHECK_LE(throttle_sec, 24 * 60 * 60);
-
   // Only positive values for throttle_sec are effective. 0 means that no
-  // throttling occurs and has the effect of resetting the member.
+  // throttling occurs and it resets |throttle_updates_until_|.
   // Negative values are not trusted and are ignored.
+  constexpr int kMaxRetryAfterSec = 24 * 60 * 60;  // 24 hours.
+  const int throttle_sec =
+      std::min(update_context->retry_after_sec, kMaxRetryAfterSec);
   if (throttle_sec >= 0) {
     throttle_updates_until_ =
         throttle_sec ? base::TimeTicks::Now() +

components/update_client/url_fetcher_downloader.cc

@@ -90,7 +90,9 @@ void UrlFetcherDownloader::StartURLFetch(const GURL& url) {
   download_start_time_ = base::TimeTicks::Now();
 }
 
-void UrlFetcherDownloader::OnNetworkFetcherComplete(base::FilePath file_path) {
+void UrlFetcherDownloader::OnNetworkFetcherComplete(base::FilePath file_path,
+                                                    int net_error,
+                                                    int64_t content_size) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
 
   const base::TimeTicks download_end_time(base::TimeTicks::Now());
@@ -102,41 +104,39 @@ void UrlFetcherDownloader::OnNetworkFetcherComplete(base::FilePath file_path) {
   // Consider a 5xx response from the server as an indication to terminate
   // the request and avoid overloading the server in this case.
   // is not accepting requests for the moment.
-  int fetch_error = -1;
+  int error = -1;
   if (!file_path.empty() && response_code_ == 200) {
-    fetch_error = 0;
+    DCHECK_EQ(0, net_error);
+    error = 0;
   } else if (response_code_ != -1) {
-    fetch_error = response_code_;
+    error = response_code_;
   } else {
-    fetch_error = network_fetcher_->NetError();
+    error = net_error;
   }
 
-  const bool is_handled = fetch_error == 0 || IsHttpServerError(fetch_error);
+  const bool is_handled = error == 0 || IsHttpServerError(error);
 
   Result result;
-  result.error = fetch_error;
-  if (!fetch_error) {
+  result.error = error;
+  if (!error) {
     result.response = file_path;
   }
 
   DownloadMetrics download_metrics;
   download_metrics.url = url();
   download_metrics.downloader = DownloadMetrics::kUrlFetcher;
-  download_metrics.error = fetch_error;
+  download_metrics.error = error;
   // Tests expected -1, in case of failures and no content is available.
-  download_metrics.downloaded_bytes =
-      fetch_error && !network_fetcher_->GetContentSize()
-          ? -1
-          : network_fetcher_->GetContentSize();
+  download_metrics.downloaded_bytes = error ? -1 : content_size;
   download_metrics.total_bytes = total_bytes_;
   download_metrics.download_time_ms = download_time.InMilliseconds();
 
-  VLOG(1) << "Downloaded " << network_fetcher_->GetContentSize() << " bytes in "
+  VLOG(1) << "Downloaded " << content_size << " bytes in "
           << download_time.InMilliseconds() << "ms from " << final_url_.spec()
           << " to " << result.response.value();
 
   // Delete the download directory in the error cases.
-  if (fetch_error && !download_dir_.empty())
+  if (error && !download_dir_.empty())
     base::PostTaskWithTraits(
         FROM_HERE, kTaskTraits,
         base::BindOnce(IgnoreResult(&base::DeleteFile), download_dir_, true));

components/update_client/url_fetcher_downloader.h

@@ -35,7 +35,9 @@ class UrlFetcherDownloader : public CrxDownloader {
 
   void CreateDownloadDir();
   void StartURLFetch(const GURL& url);
-  void OnNetworkFetcherComplete(base::FilePath file_path);
+  void OnNetworkFetcherComplete(base::FilePath file_path,
+                                int net_error,
+                                int64_t content_size);
   void OnResponseStarted(const GURL& final_url,
                          int response_code,
                          int64_t content_length);