Retry to download the cleaner when network is available.

Bug: 773856
Change-Id: I1523381b4492b74b379ca1bd1801a59b2d447324
Reviewed-on: https://chromium-review.googlesource.com/714216
Commit-Queue: Fabio Tirelo <ftirelo@chromium.org>
Reviewed-by: Chris Sharp <csharp@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509566}

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc

@@ -24,6 +24,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/task_runner_util.h"
 #include "base/task_scheduler/post_task.h"
+#include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h"
 #include "chrome/install_static/install_details.h"
@@ -31,6 +32,7 @@
 #include "components/data_use_measurement/core/data_use_user_data.h"
 #include "components/version_info/version_info.h"
 #include "net/base/load_flags.h"
+#include "net/base/network_change_notifier.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/http_status_code.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
@@ -41,8 +43,24 @@
 
 namespace safe_browsing {
 
+const int kMaxCleanerDownloadAttempts = 3;
+
 namespace {
 
+constexpr char kDownloadStatusErrorCodeHistogramName[] =
+    "SoftwareReporter.Cleaner.DownloadStatusErrorCode";
+
+// Indicates the suffix to use for some histograms that depend on the final
+// download status. This is used because UMA histogram macros define static
+// constant strings to represent the name, so they can't be used when a name
+// is dynamically generated. The alternative would be to replicate the logic
+// of those macros, which is not ideal.
+enum class FetchCompletedReasonHistogramSuffix {
+  kDownloadSuccess,
+  kDownloadFailure,
+  kNetworkError,
+};
+
 base::FilePath::StringType CleanerTempDirectoryPrefix() {
   // Create a temporary directory name prefix like "ChromeCleaner_4_", where
   // "Chrome" is the product name and the 4 refers to the install mode of the
@@ -94,11 +112,21 @@ void RecordCleanerDownloadStatusHistogram(
 // Class that will attempt to download the Chrome Cleaner executable and call a
 // given callback when done. Instances of ChromeCleanerFetcher own themselves
 // and will self-delete if they encounter an error or when the network request
-// has completed.
-class ChromeCleanerFetcher : public net::URLFetcherDelegate {
+// has completed. On network errors due to connection not available, will retry
+// once connectivity is restablished.
+// NOTE: What we really want is to check if internet connectivity exists, but
+// since this information is not available, listening to network changes is
+// best we can do.
+class ChromeCleanerFetcher
+    : public net::URLFetcherDelegate,
+      public net::NetworkChangeNotifier::NetworkChangeObserver {
  public:
   explicit ChromeCleanerFetcher(ChromeCleanerFetchedCallback fetched_callback);
 
+  // NetworkChangeObserver overrides.
+  void OnNetworkChanged(
+      net::NetworkChangeNotifier::ConnectionType type) override;
+
  protected:
   ~ChromeCleanerFetcher() override;
 
@@ -110,12 +138,21 @@ class ChromeCleanerFetcher : public net::URLFetcherDelegate {
   void PostCallbackAndDeleteSelf(base::FilePath path,
                                  ChromeCleanerFetchStatus fetch_status);
 
+  // Retries up to |kMaxCleanerDownloadAttempts| times to download the cleaner
+  // when a network error prevents the connection to succeed.
+  void MaybeRetryDownloadingCleaner(int net_error);
+
   // Sends a histogram indicating an error and invokes the fetch callback if
   // the cleaner binary can't be downloaded or saved to the disk.
   void RecordDownloadStatusAndPostCallback(
       CleanerDownloadStatusHistogramValue histogram_value,
       ChromeCleanerFetchStatus fetch_status);
 
+  void RecordNumberOfDownloadAttempts(
+      FetchCompletedReasonHistogramSuffix suffix);
+
+  void RecordTimeToCompleteDownload(FetchCompletedReasonHistogramSuffix suffix);
+
   // net::URLFetcherDelegate overrides.
   void OnURLFetchComplete(const net::URLFetcher* source) override;
 
@@ -134,6 +171,11 @@ class ChromeCleanerFetcher : public net::URLFetcherDelegate {
       scoped_temp_dir_;
   base::FilePath temp_file_;
 
+  int attempts_to_download_ = 0;
+
+  // For metrics reporting.
+  base::Time time_fetching_started_;
+
   DISALLOW_COPY_AND_ASSIGN(ChromeCleanerFetcher);
 };
 
@@ -158,6 +200,23 @@ ChromeCleanerFetcher::ChromeCleanerFetcher(
                  base::Unretained(this)));
 }
 
+void ChromeCleanerFetcher::OnNetworkChanged(
+    net::NetworkChangeNotifier::ConnectionType type) {
+  DCHECK_GT(kMaxCleanerDownloadAttempts, attempts_to_download_);
+
+  // When network is reconnected, OnNetworkChanged will always be called
+  // with CONNECTION_NONE immediately prior to being called with an online
+  // state.
+  if (type == net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE)
+    return;
+
+  // Prevent from getting notified again of network changes unless needed.
+  net::NetworkChangeNotifier::RemoveNetworkChangeObserver(this);
+
+  // Retry to download the cleaner with the same parameters as before.
+  url_fetcher_->Start();
+}
+
 ChromeCleanerFetcher::~ChromeCleanerFetcher() = default;
 
 bool ChromeCleanerFetcher::CreateTemporaryDirectory() {
@@ -190,6 +249,8 @@ void ChromeCleanerFetcher::OnTemporaryDirectoryCreated(bool success) {
   url_fetcher_->SetMaxRetriesOn5xx(3);
   url_fetcher_->SaveResponseToFileAtPath(temp_file_, blocking_task_runner_);
   url_fetcher_->SetRequestContext(g_browser_process->system_request_context());
+
+  time_fetching_started_ = base::Time::Now();
   url_fetcher_->Start();
 }
 
@@ -209,27 +270,45 @@ void ChromeCleanerFetcher::PostCallbackAndDeleteSelf(
   delete this;
 }
 
+void ChromeCleanerFetcher::MaybeRetryDownloadingCleaner(int net_error) {
+  ++attempts_to_download_;
+  UMA_HISTOGRAM_SPARSE_SLOWLY(kDownloadStatusErrorCodeHistogramName, net_error);
+  if (attempts_to_download_ == kMaxCleanerDownloadAttempts) {
+    RecordTimeToCompleteDownload(
+        FetchCompletedReasonHistogramSuffix::kNetworkError);
+    RecordNumberOfDownloadAttempts(
+        FetchCompletedReasonHistogramSuffix::kNetworkError);
+    RecordDownloadStatusAndPostCallback(
+        CLEANER_DOWNLOAD_STATUS_OTHER_FAILURE,
+        ChromeCleanerFetchStatus::kOtherFailure);
+    return;
+  }
+
+  net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
+}
+
 void ChromeCleanerFetcher::OnURLFetchComplete(const net::URLFetcher* source) {
   // Take ownership of the fetcher in this scope (source == url_fetcher_).
   DCHECK_EQ(url_fetcher_.get(), source);
   DCHECK(!source->GetStatus().is_io_pending());
   DCHECK(fetched_callback_);
 
-  constexpr char kDownloadStatusErrorCodeHistogramName[] =
-      "SoftwareReporter.Cleaner.DownloadStatusErrorCode";
-
   if (!source->GetStatus().is_success()) {
-    UMA_HISTOGRAM_SPARSE_SLOWLY(kDownloadStatusErrorCodeHistogramName,
-                                source->GetStatus().error());
-    RecordDownloadStatusAndPostCallback(
-        CLEANER_DOWNLOAD_STATUS_OTHER_FAILURE,
-        ChromeCleanerFetchStatus::kOtherFailure);
+    // In case of network errors that prevent the connection to complete, retry
+    // to download once a connection becomes available.
+    MaybeRetryDownloadingCleaner(source->GetStatus().error());
     return;
   }
 
   const int response_code = source->GetResponseCode();
   UMA_HISTOGRAM_SPARSE_SLOWLY(kDownloadStatusErrorCodeHistogramName,
                               response_code);
+  const FetchCompletedReasonHistogramSuffix suffix =
+      response_code == net::HTTP_OK
+          ? FetchCompletedReasonHistogramSuffix::kDownloadSuccess
+          : FetchCompletedReasonHistogramSuffix::kDownloadFailure;
+  RecordTimeToCompleteDownload(suffix);
+  RecordNumberOfDownloadAttempts(suffix);
 
   if (response_code == net::HTTP_NOT_FOUND) {
     RecordDownloadStatusAndPostCallback(
@@ -271,6 +350,54 @@ void ChromeCleanerFetcher::RecordDownloadStatusAndPostCallback(
   PostCallbackAndDeleteSelf(base::FilePath(), fetch_status);
 }
 
+void ChromeCleanerFetcher::RecordNumberOfDownloadAttempts(
+    FetchCompletedReasonHistogramSuffix suffix) {
+  switch (suffix) {
+    case FetchCompletedReasonHistogramSuffix::kDownloadFailure:
+      UMA_HISTOGRAM_COUNTS_100(
+          "SoftwareReporter.Cleaner.NumberOfDownloadAttempts_DownloadFailure",
+          attempts_to_download_);
+      break;
+
+    case FetchCompletedReasonHistogramSuffix::kDownloadSuccess:
+      UMA_HISTOGRAM_COUNTS_100(
+          "SoftwareReporter.Cleaner.NumberOfDownloadAttempts_DownloadSuccess",
+          attempts_to_download_);
+      break;
+
+    case FetchCompletedReasonHistogramSuffix::kNetworkError:
+      UMA_HISTOGRAM_COUNTS_100(
+          "SoftwareReporter.Cleaner.NumberOfDownloadAttempts_NetworkError",
+          attempts_to_download_);
+      break;
+  }
+}
+
+void ChromeCleanerFetcher::RecordTimeToCompleteDownload(
+    FetchCompletedReasonHistogramSuffix suffix) {
+  const base::TimeDelta time_difference =
+      base::Time::Now() - time_fetching_started_;
+  switch (suffix) {
+    case FetchCompletedReasonHistogramSuffix::kDownloadFailure:
+      UMA_HISTOGRAM_LONG_TIMES_100(
+          "SoftwareReporter.Cleaner.TimeToCompleteDownload_DownloadFailure",
+          time_difference);
+      break;
+
+    case FetchCompletedReasonHistogramSuffix::kDownloadSuccess:
+      UMA_HISTOGRAM_LONG_TIMES_100(
+          "SoftwareReporter.Cleaner.TimeToCompleteDownload_DownloadSuccess",
+          time_difference);
+      break;
+
+    case FetchCompletedReasonHistogramSuffix::kNetworkError:
+      UMA_HISTOGRAM_LONG_TIMES_100(
+          "SoftwareReporter.Cleaner.TimeToCompleteDownload_NetworkError",
+          time_difference);
+      break;
+  }
+}
+
 }  // namespace
 
 void FetchChromeCleaner(ChromeCleanerFetchedCallback fetched_callback) {

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.h

@@ -13,6 +13,10 @@ class FilePath;
 
 namespace safe_browsing {
 
+// The maximum number of attempts to download the cleaner in case of network
+// errors.
+extern const int kMaxCleanerDownloadAttempts;
+
 enum class ChromeCleanerFetchStatus {
   // Fetch succeeded with a net::HTTP_OK response code.
   kSuccess,

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win_unittest.cc

@@ -14,6 +14,7 @@
 #include "chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "net/base/net_errors.h"
+#include "net/base/network_change_notifier.h"
 #include "net/http/http_status_code.h"
 #include "net/url_request/test_url_fetcher_factory.h"
 #include "net/url_request/url_fetcher_delegate.h"
@@ -70,6 +71,21 @@ class ChromeCleanerFetcherTest : public ::testing::Test,
   void OnChunkUpload(int fetcher_id) override {}
   void OnRequestEnd(int fetcher_id) override {}
 
+  void SetNetworkSuccess(int response_code) {
+    fetcher_->set_status(net::URLRequestStatus{});
+    fetcher_->set_response_code(response_code);
+    fetcher_->delegate()->OnURLFetchComplete(fetcher_);
+  }
+
+  void SetNetworkError() {
+    // Set up the fetcher to return failure other than HTTP_NOT_FOUND.
+    fetcher_->set_status(net::URLRequestStatus::FromError(net::ERR_FAILED));
+    // For this test, just use any http response code other than net::HTTP_OK
+    // and net::HTTP_NOT_FOUND.
+    fetcher_->set_response_code(net::HTTP_INTERNAL_SERVER_ERROR);
+    fetcher_->delegate()->OnURLFetchComplete(fetcher_);
+  }
+
  protected:
   // TestURLFetcher requires a MessageLoop and an IO thread to release
   // URLRequestContextGetter in URLFetcher::Core.
@@ -92,16 +108,15 @@ class ChromeCleanerFetcherTest : public ::testing::Test,
   base::FilePath downloaded_path_;
   ChromeCleanerFetchStatus fetch_status_ =
       ChromeCleanerFetchStatus::kOtherFailure;
+
+  std::unique_ptr<net::NetworkChangeNotifier> network_change_notifier_ =
+      base::WrapUnique(net::NetworkChangeNotifier::CreateMock());
 };
 
 TEST_F(ChromeCleanerFetcherTest, FetchSuccess) {
   EXPECT_EQ(GURL(fetcher_->GetOriginalURL()), GetSRTDownloadURL());
 
-  // Set up the fetcher to return success.
-  fetcher_->set_status(net::URLRequestStatus{});
-  fetcher_->set_response_code(net::HTTP_OK);
-
-  fetcher_->delegate()->OnURLFetchComplete(fetcher_);
+  SetNetworkSuccess(net::HTTP_OK);
 
   EXPECT_TRUE(callback_called_);
   EXPECT_EQ(downloaded_path_, response_path_);
@@ -112,24 +127,61 @@ TEST_F(ChromeCleanerFetcherTest, NotFoundOnServer) {
   // Set up the fetcher to return a HTTP_NOT_FOUND failure. Notice that the
   // net error in this case is OK, since there was no error preventing any
   // response (even 404) from being received.
-  fetcher_->set_status(net::URLRequestStatus{});
-  fetcher_->set_response_code(net::HTTP_NOT_FOUND);
+  SetNetworkSuccess(net::HTTP_NOT_FOUND);
+
+  EXPECT_TRUE(callback_called_);
+  EXPECT_TRUE(downloaded_path_.empty());
+  EXPECT_EQ(fetch_status_, ChromeCleanerFetchStatus::kNotFoundOnServer);
+}
+
+TEST_F(ChromeCleanerFetcherTest, NetworkError_SucceededAfterRetry) {
+  for (int attempt = 1; attempt < kMaxCleanerDownloadAttempts; ++attempt) {
+    SetNetworkError();
+
+    EXPECT_FALSE(callback_called_);
+
+    net::NetworkChangeNotifier::NotifyObserversOfNetworkChangeForTests(
+        net::NetworkChangeNotifier::CONNECTION_ETHERNET);
+    base::RunLoop().RunUntilIdle();
+  }
+
+  SetNetworkSuccess(net::HTTP_OK);
+
+  EXPECT_TRUE(callback_called_);
+  EXPECT_EQ(downloaded_path_, response_path_);
+  EXPECT_EQ(fetch_status_, ChromeCleanerFetchStatus::kSuccess);
+}
 
-  fetcher_->delegate()->OnURLFetchComplete(fetcher_);
+TEST_F(ChromeCleanerFetcherTest, NetworkError_NotFoundAfterRetry) {
+  for (int attempt = 1; attempt < kMaxCleanerDownloadAttempts; ++attempt) {
+    SetNetworkError();
+
+    EXPECT_FALSE(callback_called_);
+
+    net::NetworkChangeNotifier::NotifyObserversOfNetworkChangeForTests(
+        net::NetworkChangeNotifier::CONNECTION_ETHERNET);
+    base::RunLoop().RunUntilIdle();
+  }
+
+  SetNetworkSuccess(net::HTTP_NOT_FOUND);
 
   EXPECT_TRUE(callback_called_);
   EXPECT_TRUE(downloaded_path_.empty());
   EXPECT_EQ(fetch_status_, ChromeCleanerFetchStatus::kNotFoundOnServer);
 }
 
-TEST_F(ChromeCleanerFetcherTest, OtherFailure) {
-  // Set up the fetcher to return failure other than HTTP_NOT_FOUND.
-  fetcher_->set_status(net::URLRequestStatus::FromError(net::ERR_FAILED));
-  // For this test, just use any http response code other than net::HTTP_OK and
-  // net::HTTP_NOT_FOUND.
-  fetcher_->set_response_code(net::HTTP_INTERNAL_SERVER_ERROR);
+TEST_F(ChromeCleanerFetcherTest, NetworkError_AllAttemptsFailed) {
+  for (int attempt = 1; attempt < kMaxCleanerDownloadAttempts; ++attempt) {
+    SetNetworkError();
+
+    EXPECT_FALSE(callback_called_);
+
+    net::NetworkChangeNotifier::NotifyObserversOfNetworkChangeForTests(
+        net::NetworkChangeNotifier::CONNECTION_ETHERNET);
+    base::RunLoop().RunUntilIdle();
+  }
 
-  fetcher_->delegate()->OnURLFetchComplete(fetcher_);
+  SetNetworkError();
 
   EXPECT_TRUE(callback_called_);
   EXPECT_TRUE(downloaded_path_.empty());

tools/metrics/histograms/histograms.xml

@@ -79196,6 +79196,15 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="SoftwareReporter.Cleaner.NumberOfDownloadAttempts"
+    units="counts">
+  <owner>ftirelo@chromium.org</owner>
+  <summary>
+    The number of attempts to download the Chrome Cleanup tool until it either
+    succeeds or fails.
+  </summary>
+</histogram>
+
 <histogram name="SoftwareReporter.Cleaner.RebootResponse" enum="Boolean">
   <owner>ftirelo@chromium.org</owner>
   <summary>
@@ -79230,6 +79239,15 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="SoftwareReporter.Cleaner.TimeToCompleteDownload" units="ms">
+  <owner>ftirelo@chromium.org</owner>
+  <summary>
+    The time between the first attempt to download the Chrome Cleanup tool and a
+    successful download or the last unsuccessful attempt to download without
+    retrying.
+  </summary>
+</histogram>
+
 <histogram name="SoftwareReporter.Cleaner.Version">
   <owner>mad@chromium.org</owner>
   <summary>The build version of the software reporter cleaner tool.</summary>
@@ -96383,6 +96401,15 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   <affected-histogram name="SoftwareReporter.PromptDialog.TimeUntilDone"/>
 </histogram_suffixes>
 
+<histogram_suffixes name="ChromeCleanerFetchCompletedReason" separator="_">
+  <suffix name="DownloadFailure"
+      label="Download failed without an OK HTTP response code."/>
+  <suffix name="DownloadSuccess" label="Download succeeded."/>
+  <suffix name="NetworkError" label="Download failed due to a network error."/>
+  <affected-histogram name="SoftwareReporter.Cleaner.NumberOfDownloadAttempts"/>
+  <affected-histogram name="SoftwareReporter.Cleaner.TimeToCompleteDownload"/>
+</histogram_suffixes>
+
 <histogram_suffixes name="ChromeContentBrowserClientMetricSuffixes"
     separator=".">
   <suffix name="search"