Add counters to measure network errors in the extension updater update check.

Note that these new histograms will help us understand more about the update
check error codes of the unified extension updater Finch experiment currently
running in Beta.


Bug: 722942
Change-Id: Ie20083437c8a06b7649c5ed01e91ac68357d84ca
Reviewed-on: https://chromium-review.googlesource.com/c/1271790
Commit-Queue: Minh Nguyen <mxnguyen@chromium.org>
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Robert Kaplow (sloooow) <rkaplow@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599278}

chrome/browser/extensions/updater/extension_updater_unittest.cc

@@ -2587,22 +2587,17 @@ TEST_F(ExtensionUpdaterTest, TestCheckSoon) {
 }
 
 TEST_F(ExtensionUpdaterTest, TestDisabledReasons1) {
-  std::vector<int> disabled;
-  disabled.push_back(disable_reason::DISABLE_USER_ACTION);
-  disabled.push_back(disable_reason::DISABLE_PERMISSIONS_INCREASE |
-                     disable_reason::DISABLE_CORRUPTED);
-  TestPingMetrics(1, disabled);
+  TestPingMetrics(1, {disable_reason::DISABLE_USER_ACTION,
+                      disable_reason::DISABLE_PERMISSIONS_INCREASE |
+                          disable_reason::DISABLE_CORRUPTED});
 }
 
 TEST_F(ExtensionUpdaterTest, TestDisabledReasons2) {
-  std::vector<int> disabled;
-  TestPingMetrics(1, disabled);
+  TestPingMetrics(1, {});
 }
 
 TEST_F(ExtensionUpdaterTest, TestDisabledReasons3) {
-  std::vector<int> disabled;
-  disabled.push_back(0);
-  TestPingMetrics(0, disabled);
+  TestPingMetrics(0, {0});
 }
 
 TEST_F(ExtensionUpdaterTest, TestUninstallWhileUpdateCheck) {
@@ -2630,6 +2625,11 @@ TEST_F(ExtensionUpdaterTest, TestUninstallWhileUpdateCheck) {
 
   service.set_extensions(ExtensionList(), ExtensionList());
   ASSERT_FALSE(service.GetExtensionById(id, false));
+
+  // RunUntilIdle is needed to make sure that the UpdateService instance that
+  // runs the extension update process has a chance to exit gracefully; without
+  // it, the test would crash.
+  RunUntilIdle();
 }
 
 // Tests that we don't get a DCHECK failure when the next check time saved in

extensions/browser/updater/extension_downloader.cc

@@ -111,7 +111,7 @@ ExtensionDownloaderTestDelegate* g_test_delegate = nullptr;
                                 kMaxRetries + 1);                         \
   }
 
-bool ShouldRetryRequest(network::SimpleURLLoader* loader) {
+bool ShouldRetryRequest(const network::SimpleURLLoader* loader) {
   DCHECK(loader);
 
   // Since HTTP errors are now presented as ERR_FAILED by default, this will
@@ -575,7 +575,9 @@ void ExtensionDownloader::CreateManifestLoader() {
 
 void ExtensionDownloader::OnManifestLoadComplete(
     std::unique_ptr<std::string> response_body) {
-  GURL url = manifest_loader_->GetFinalURL();
+  const GURL url = manifest_loader_->GetFinalURL();
+  DCHECK(manifests_queue_.active_request());
+
   int response_code = -1;
   if (manifest_loader_->ResponseInfo() &&
       manifest_loader_->ResponseInfo()->headers)
@@ -584,13 +586,13 @@ void ExtensionDownloader::OnManifestLoadComplete(
   VLOG(2) << response_code << " " << url;
 
   const base::TimeDelta& backoff_delay = base::TimeDelta::FromMilliseconds(0);
+  const int request_failure_count =
+      manifests_queue_.active_request_failure_count();
 
   // We want to try parsing the manifest, and if it indicates updates are
   // available, we want to fire off requests to fetch those updates.
   if (response_body && !response_body->empty()) {
-    RETRY_HISTOGRAM("ManifestFetchSuccess",
-                    manifests_queue_.active_request_failure_count(),
-                    url);
+    RETRY_HISTOGRAM("ManifestFetchSuccess", request_failure_count, url);
     VLOG(2) << "beginning manifest parse for " << url;
     auto callback = base::BindOnce(&ExtensionDownloader::HandleManifestResults,
                                    weak_ptr_factory_.GetWeakPtr(),
@@ -599,8 +601,28 @@ void ExtensionDownloader::OnManifestLoadComplete(
   } else {
     VLOG(1) << "Failed to fetch manifest '" << url.possibly_invalid_spec()
             << "' response code:" << response_code;
-    if (ShouldRetryRequest(manifest_loader_.get()) &&
-        manifests_queue_.active_request_failure_count() < kMaxRetries) {
+    const auto* loader = manifest_loader_.get();
+    if (request_failure_count == 0) {
+      DCHECK(loader);
+      // This is the first failure for this batch request, record the
+      // http/network error for each extension in the batch request.
+      const int error =
+          response_code == -1 ? loader->NetError() : response_code;
+      const std::string uma_histogram_name =
+          url.DomainIs(kGoogleDotCom)
+              ? std::string(
+                    "Extensions."
+                    "ExtensionUpdaterFirstUpdateCheckErrorsGoogleUrl")
+              : std::string(
+                    "Extensions."
+                    "ExtensionUpdaterFirstUpdateCheckErrorsNonGoogleUrl");
+      const auto& extension_ids =
+          manifests_queue_.active_request()->extension_ids();
+      for (auto it = extension_ids.begin(); it != extension_ids.end(); ++it) {
+        base::UmaHistogramSparse(uma_histogram_name, error);
+      }
+    }
+    if (ShouldRetryRequest(loader) && request_failure_count < kMaxRetries) {
       manifests_queue_.RetryRequest(backoff_delay);
     } else {
       RETRY_HISTOGRAM("ManifestFetchFailure",

tools/metrics/histograms/histograms.xml

@@ -30176,6 +30176,28 @@ uploading your change for review.
   <summary>An extension has been uninstalled.</summary>
 </histogram>
 
+<histogram name="Extensions.ExtensionUpdaterFirstUpdateCheckErrorsGoogleUrl"
+    enum="CombinedHttpResponseAndNetErrorCode" expires_after="M72">
+  <owner>mxnguyen@chromium.org</owner>
+  <summary>
+    Records the error codes of the extension updater update check errors. These
+    events are triggered only when the extension updater gets an error for the
+    first time (before any retry) in the update check phase for a google.com
+    domain.
+  </summary>
+</histogram>
+
+<histogram name="Extensions.ExtensionUpdaterFirstUpdateCheckErrorsNonGoogleUrl"
+    enum="CombinedHttpResponseAndNetErrorCode" expires_after="M72">
+  <owner>mxnguyen@chromium.org</owner>
+  <summary>
+    Records the error codes of the extension updater update check errors. These
+    events are triggered only when the extension updater gets an error for the
+    first time (before any retry) in the update check phase for a non google.com
+    domain.
+  </summary>
+</histogram>
+
 <histogram name="Extensions.ExtensionUpdaterRawUpdateCalls" units="extensions">
   <owner>mxnguyen@chromium.org</owner>
   <summary>