[Extensions] Report UMA metrics about verified contents fetch failure

For security reasons, resources of extensions from the Chrome Web Store
are verified when used. For that content verifier uses hashes of the
resources, signed by Chrome Web Store. These hashes might or might not
be included in the extension archive. Therefore sometimes they are
fetched from the Chrome Web Store only when needed.

If fetching of the hashes fails, extension is disabled because content
verifier cannot ensure that it is not corrupted. Unfortunately, this may
lead to false-positive disabling of the extension: if fetch of the
hashes failed, for example, because of bad network. To investigate
whether this happens frequently, this CL adds a new histogram:

 * Extensions.ContentVerification.FetchNetError : network error code
when fetch of the content hashes failed.

Bug: None
Change-Id: I03ff9c104a591579d713a5c939853645bea8ec71
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2426583
Commit-Queue: Oleg Davydov <burunduk@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820237}

extensions/browser/content_hash_fetcher.cc

@@ -26,6 +26,7 @@
 #include "extensions/common/file_util.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "net/base/load_flags.h"
+#include "net/base/net_errors.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
 #include "services/network/public/cpp/resource_request.h"
 #include "services/network/public/cpp/simple_url_loader.h"
@@ -45,10 +46,19 @@ void ContentHashFetcher::OnSimpleLoaderComplete(
           << " is_success:" << !!response_body << " "
           << fetch_key_.fetch_url.possibly_invalid_spec();
   DCHECK(hash_fetcher_callback_);
+  DCHECK(simple_loader_);
+  bool report_http_response_code =
+      (simple_loader_->NetError() == net::OK ||
+       simple_loader_->NetError() == net::ERR_HTTP_RESPONSE_CODE_FAILURE) &&
+      simple_loader_->ResponseInfo() && simple_loader_->ResponseInfo()->headers;
   response_task_runner_->PostTask(
       FROM_HERE,
-      base::BindOnce(std::move(hash_fetcher_callback_), std::move(fetch_key_),
-                     std::move(response_body)));
+      base::BindOnce(
+          std::move(hash_fetcher_callback_), std::move(fetch_key_),
+          std::move(response_body),
+          report_http_response_code
+              ? simple_loader_->ResponseInfo()->headers->response_code()
+              : simple_loader_->NetError()));
   delete this;
 }
 

extensions/browser/content_hash_fetcher.h

@@ -44,10 +44,12 @@ namespace internals {
 class ContentHashFetcher {
  public:
   // A callback for when fetch is complete.
-  // The response contents is passed through std::unique_ptr<std::string>.
+  // The response contents is passed through std::unique_ptr<std::string>. In
+  // case of failure the error code is passed as a last argument.
   using HashFetcherCallback =
       base::OnceCallback<void(ContentHash::FetchKey,
-                              std::unique_ptr<std::string>)>;
+                              std::unique_ptr<std::string>,
+                              ContentHash::FetchErrorCode)>;
 
   ContentHashFetcher(ContentHash::FetchKey fetch_key);
 

extensions/browser/content_verifier/content_hash.cc

@@ -10,6 +10,7 @@
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/json/json_reader.h"
+#include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sequence_checker.h"
 #include "base/task/post_task.h"
@@ -22,6 +23,7 @@
 #include "extensions/browser/extension_file_task_runner.h"
 #include "extensions/common/file_util.h"
 #include "net/base/load_flags.h"
+#include "net/base/net_errors.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 
@@ -103,7 +105,7 @@ void ContentHash::Create(
 
     GetComputedHashes(source_type, is_cancelled, std::move(created_callback),
                       std::move(key), /*verified_contents=*/nullptr,
-                      /*did_attempt_fetch=*/false);
+                      /*did_attempt_fetch=*/false, /*fetch_error=*/net::OK);
   }
 }
 
@@ -180,7 +182,7 @@ void ContentHash::GetVerifiedContents(
   if (verified_contents) {
     std::move(verified_contents_callback)
         .Run(std::move(key), std::move(verified_contents),
-             /*did_attempt_fetch=*/false);
+             /*did_attempt_fetch=*/false, /*fetch_error=*/net::OK);
     return;
   }
 
@@ -241,20 +243,22 @@ std::unique_ptr<VerifiedContents> ContentHash::StoreAndRetrieveVerifiedContents(
 void ContentHash::DidFetchVerifiedContents(
     GetVerifiedContentsCallback verified_contents_callback,
     FetchKey key,
-    std::unique_ptr<std::string> fetched_contents) {
+    std::unique_ptr<std::string> fetched_contents,
+    FetchErrorCode fetch_error) {
   std::unique_ptr<VerifiedContents> verified_contents =
       StoreAndRetrieveVerifiedContents(std::move(fetched_contents), key);
 
   if (!verified_contents) {
     std::move(verified_contents_callback)
-        .Run(std::move(key), nullptr, /*did_attempt_fetch=*/true);
+        .Run(std::move(key), nullptr, /*did_attempt_fetch=*/true,
+             /*fetch_error=*/fetch_error);
     return;
   }
 
-  RecordFetchResult(true);
+  RecordFetchResult(true, fetch_error);
   std::move(verified_contents_callback)
       .Run(std::move(key), std::move(verified_contents),
-           /*did_attempt_fetch=*/true);
+           /*did_attempt_fetch=*/true, /*fetch_error=*/fetch_error);
 }
 
 // static
@@ -264,14 +268,15 @@ void ContentHash::GetComputedHashes(
     CreatedCallback created_callback,
     FetchKey key,
     std::unique_ptr<VerifiedContents> verified_contents,
-    bool did_attempt_fetch) {
+    bool did_attempt_fetch,
+    FetchErrorCode fetch_error) {
   if (source_type ==
           ContentVerifierDelegate::VerifierSourceType::SIGNED_HASHES &&
       !verified_contents) {
     DCHECK(did_attempt_fetch);
     ContentHash::DispatchFetchFailure(key.extension_id, key.extension_root,
                                       source_type, std::move(created_callback),
-                                      is_cancelled);
+                                      is_cancelled, fetch_error);
     return;
   }
   scoped_refptr<ContentHash> hash =
@@ -288,11 +293,12 @@ void ContentHash::DispatchFetchFailure(
     const base::FilePath& extension_root,
     ContentVerifierDelegate::VerifierSourceType source_type,
     CreatedCallback created_callback,
-    const IsCancelledCallback& is_cancelled) {
+    const IsCancelledCallback& is_cancelled,
+    FetchErrorCode fetch_error) {
   DCHECK_EQ(ContentVerifierDelegate::VerifierSourceType::SIGNED_HASHES,
             source_type)
       << "Only signed hashes should attempt fetching verified_contents.json";
-  RecordFetchResult(false);
+  RecordFetchResult(false, fetch_error);
   // NOTE: bare new because ContentHash constructor is private.
   scoped_refptr<ContentHash> content_hash =
       new ContentHash(extension_id, extension_root, source_type, nullptr);
@@ -301,8 +307,12 @@ void ContentHash::DispatchFetchFailure(
 }
 
 // static
-void ContentHash::RecordFetchResult(bool success) {
+void ContentHash::RecordFetchResult(bool success, int fetch_error) {
   UMA_HISTOGRAM_BOOLEAN("Extensions.ContentVerification.FetchResult", success);
+  if (!success) {
+    base::UmaHistogramSparse("Extensions.ContentVerification.FetchFailureError",
+                             fetch_error);
+  }
 }
 
 bool ContentHash::ShouldComputeHashesForResource(

extensions/browser/content_verifier/content_hash.h

@@ -50,6 +50,9 @@ namespace extensions {
 // take long time. This cancellation can be performed through |is_cancelled|.
 class ContentHash : public base::RefCountedThreadSafe<ContentHash> {
  public:
+  // The combined (network or http response) error code while fetching.
+  using FetchErrorCode = int;
+
   // Holds key to identify an extension for content verification, parameters to
   // fetch verified_contents.json and other supplementary info.
   struct FetchKey {
@@ -158,7 +161,8 @@ class ContentHash : public base::RefCountedThreadSafe<ContentHash> {
   using GetVerifiedContentsCallback = base::OnceCallback<void(
       FetchKey key,
       std::unique_ptr<VerifiedContents> verified_contents,
-      bool did_attempt_fetch)>;
+      bool did_attempt_fetch,
+      FetchErrorCode fetch_error)>;
 
   ContentHash(const ExtensionId& id,
               const base::FilePath& root,
@@ -181,7 +185,8 @@ class ContentHash : public base::RefCountedThreadSafe<ContentHash> {
   static void DidFetchVerifiedContents(
       GetVerifiedContentsCallback callback,
       FetchKey key,
-      std::unique_ptr<std::string> fetched_contents);
+      std::unique_ptr<std::string> fetched_contents,
+      FetchErrorCode fetch_error);
 
   // Step 2/2: computed_hashes.json.
   static void GetComputedHashes(
@@ -190,16 +195,18 @@ class ContentHash : public base::RefCountedThreadSafe<ContentHash> {
       CreatedCallback created_callback,
       FetchKey key,
       std::unique_ptr<VerifiedContents> verified_contents,
-      bool did_attempt_fetch);
+      bool did_attempt_fetch,
+      FetchErrorCode fetch_error);
 
   static void DispatchFetchFailure(
       const ExtensionId& extension_id,
       const base::FilePath& extension_root,
       ContentVerifierDelegate::VerifierSourceType source_type,
       CreatedCallback created_callback,
-      const IsCancelledCallback& is_cancelled);
+      const IsCancelledCallback& is_cancelled,
+      FetchErrorCode fetch_error);
 
-  static void RecordFetchResult(bool success);
+  static void RecordFetchResult(bool success, FetchErrorCode fetch_error);
 
   // Computes hashes for all files in |key_.extension_root|, and uses
   // a ComputedHashes::Writer to write that information into |hashes_file|.

tools/metrics/histograms/histograms_xml/extensions/histograms.xml

@@ -383,6 +383,20 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
   </summary>
 </histogram>
 
+<histogram name="Extensions.ContentVerification.FetchFailureError"
+    enum="CombinedHttpResponseAndNetErrorCode" expires_after="2021-03-30">
+  <owner>lazyboy@chromium.org</owner>
+  <owner>burunduk@chromium.org</owner>
+  <owner>rdevlin.cronin@chromium.org</owner>
+  <owner>extensions-core@chromium.org</owner>
+  <summary>
+    Error info for fetching verified_contents.json. Recorded when the file
+    wasn't available locally and we failed to fetch it from network. Note that
+    OK may also be reported in some cases, for example, when file was
+    successfully fetched, but is incorrect.
+  </summary>
+</histogram>
+
 <histogram name="Extensions.ContentVerification.FetchResult"
     enum="BooleanSuccess" expires_after="2021-03-28">
   <owner>lazyboy@chromium.org</owner>