Expose error codes in the CrxUpdateItem.

This allows the clients of UpdateClient inspect the error codes when
an update is complete.

Bug: 843657

Change-Id: I667a58a2723846cdcc4070fc9b103419ea4859d1
Reviewed-on: https://chromium-review.googlesource.com/1060586Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559256}

components/update_client/component.cc

@@ -78,8 +78,7 @@ void InstallComplete(
              const CrxInstaller::Result& result) {
             base::DeleteFile(unpack_path, true);
             const ErrorCategory error_category =
-                result.error ? ErrorCategory::kInstallError
-                             : ErrorCategory::kErrorNone;
+                result.error ? ErrorCategory::kInstall : ErrorCategory::kNone;
             main_task_runner->PostTask(
                 FROM_HERE, base::BindOnce(std::move(callback),
                                           static_cast<int>(error_category),
@@ -110,7 +109,7 @@ void InstallOnBlockingTaskRunner(
     main_task_runner->PostTask(
         FROM_HERE,
         base::BindOnce(std::move(callback),
-                       static_cast<int>(ErrorCategory::kInstallError),
+                       static_cast<int>(ErrorCategory::kInstall),
                        static_cast<int>(result.error), result.extended_error));
     return;
   }
@@ -134,7 +133,7 @@ void UnpackCompleteOnBlockingTaskRunner(
     main_task_runner->PostTask(
         FROM_HERE,
         base::BindOnce(std::move(callback),
-                       static_cast<int>(ErrorCategory::kUnpackError),
+                       static_cast<int>(ErrorCategory::kUnpack),
                        static_cast<int>(result.error), result.extended_error));
     return;
   }
@@ -217,6 +216,9 @@ CrxUpdateItem Component::GetCrxUpdateItem() const {
   crx_update_item.last_check = last_check_;
   crx_update_item.next_version = next_version_;
   crx_update_item.next_fp = next_fp_;
+  crx_update_item.error_category = error_category_;
+  crx_update_item.error_code = error_code_;
+  crx_update_item.extra_code1 = extra_code1_;
 
   return crx_update_item;
 }
@@ -349,7 +351,7 @@ void Component::StateNew::DoHandle() {
     TransitionState(std::make_unique<StateChecking>(&component));
   } else {
     component.error_code_ = static_cast<int>(Error::CRX_NOT_FOUND);
-    component.error_category_ = static_cast<int>(ErrorCategory::kServiceError);
+    component.error_category_ = static_cast<int>(ErrorCategory::kService);
     TransitionState(std::make_unique<StateUpdateError>(&component));
   }
 }
@@ -383,7 +385,7 @@ void Component::StateChecking::DoHandle() {
 void Component::StateChecking::UpdateCheckComplete() {
   DCHECK(thread_checker_.CalledOnValidThread());
   auto& component = State::component();
-  if (!component.update_check_error_) {
+  if (!component.error_code_) {
     if (component.status_ == "ok") {
       TransitionState(std::make_unique<StateCanUpdate>(&component));
       return;
@@ -440,7 +442,7 @@ void Component::StateCanUpdate::DoHandle() {
   if (component.crx_component()
           ->supports_group_policy_enable_component_updates &&
       !component.update_context_.enabled_component_updates) {
-    component.error_category_ = static_cast<int>(ErrorCategory::kServiceError);
+    component.error_category_ = static_cast<int>(ErrorCategory::kService);
     component.error_code_ = static_cast<int>(ServiceError::UPDATE_DISABLED);
     component.extra_code1_ = 0;
     TransitionState(std::make_unique<StateUpdateError>(&component));
@@ -538,8 +540,7 @@ void Component::StateDownloadingDiff::DownloadComplete(
 
   if (download_result.error) {
     DCHECK(download_result.response.empty());
-    component.diff_error_category_ =
-        static_cast<int>(ErrorCategory::kNetworkError);
+    component.diff_error_category_ = static_cast<int>(ErrorCategory::kDownload);
     component.diff_error_code_ = download_result.error;
 
     TransitionState(std::make_unique<StateDownloading>(&component));
@@ -608,7 +609,7 @@ void Component::StateDownloading::DownloadComplete(
 
   if (download_result.error) {
     DCHECK(download_result.response.empty());
-    component.error_category_ = static_cast<int>(ErrorCategory::kNetworkError);
+    component.error_category_ = static_cast<int>(ErrorCategory::kDownload);
     component.error_code_ = download_result.error;
 
     TransitionState(std::make_unique<StateUpdateError>(&component));
@@ -670,13 +671,12 @@ void Component::StateUpdatingDiff::InstallComplete(int error_category,
     return;
   }
 
-  DCHECK_EQ(static_cast<int>(ErrorCategory::kErrorNone),
+  DCHECK_EQ(static_cast<int>(ErrorCategory::kNone),
             component.diff_error_category_);
   DCHECK_EQ(0, component.diff_error_code_);
   DCHECK_EQ(0, component.diff_extra_code1_);
 
-  DCHECK_EQ(static_cast<int>(ErrorCategory::kErrorNone),
-            component.error_category_);
+  DCHECK_EQ(static_cast<int>(ErrorCategory::kNone), component.error_category_);
   DCHECK_EQ(0, component.error_code_);
   DCHECK_EQ(0, component.extra_code1_);
 
@@ -735,8 +735,7 @@ void Component::StateUpdating::InstallComplete(int error_category,
     return;
   }
 
-  DCHECK_EQ(static_cast<int>(ErrorCategory::kErrorNone),
-            component.error_category_);
+  DCHECK_EQ(static_cast<int>(ErrorCategory::kNone), component.error_category_);
   DCHECK_EQ(0, component.error_code_);
   DCHECK_EQ(0, component.extra_code1_);
 

components/update_client/component.h

@@ -92,9 +92,10 @@ class Component {
   std::string next_fp() const { return next_fp_; }
   void set_next_fp(const std::string& next_fp) { next_fp_ = next_fp; }
 
-  int update_check_error() const { return update_check_error_; }
   void set_update_check_error(int update_check_error) {
-    update_check_error_ = update_check_error;
+    error_category_ = static_cast<int>(ErrorCategory::kUpdateCheck);
+    error_code_ = update_check_error;
+    extra_code1_ = 0;
   }
 
   bool is_foreground() const;

components/update_client/crx_update_item.h

@@ -33,6 +33,10 @@ struct CrxUpdateItem {
 
   base::Version next_version;
   std::string next_fp;
+
+  int error_category = 0;
+  int error_code = 0;
+  int extra_code1 = 0;
 };
 
 }  // namespace update_client

components/update_client/update_client_errors.h

@@ -23,11 +23,12 @@ enum class Error {
 // These errors are sent in pings. Add new values only to the bottom of
 // the enums below; the order must be kept stable.
 enum class ErrorCategory {
-  kErrorNone = 0,
-  kNetworkError,
-  kUnpackError,
-  kInstallError,
-  kServiceError,  // Runtime errors which occur in the service itself.
+  kNone = 0,
+  kDownload,
+  kUnpack,
+  kInstall,
+  kService,  // Runtime errors which occur in the service itself.
+  kUpdateCheck,
 };
 
 // These errors are returned with the |kNetworkError| error category. This
@@ -44,7 +45,7 @@ enum class CrxDownloaderError {
   GENERIC_ERROR = -1
 };
 
-// These errors are returned with the |kUnpackError| error category and
+// These errors are returned with the |kUnpack| error category and
 // indicate unpacker or patcher error.
 enum class UnpackerError {
   kNone = 0,
@@ -67,7 +68,7 @@ enum class UnpackerError {
   // kFingerprintWriteFailed = 17,    // Deprecated. Don't use.
 };
 
-// These errors are returned with the |kServiceError| error category and
+// These errors are returned with the |kService| error category and
 // are returned by the component installers.
 enum class InstallError {
   NONE = 0,
@@ -84,7 +85,7 @@ enum class InstallError {
   CUSTOM_ERROR_BASE = 100,  // Specific installer errors go above this value.
 };
 
-// These errors are returned with the |kInstallError| error category and
+// These errors are returned with the |kInstall| error category and
 // indicate critical or configuration errors in the update service.
 enum class ServiceError {
   NONE = 0,

components/update_client/update_client_unittest.cc

@@ -1064,7 +1064,15 @@ TEST_F(UpdateClientTest, TwoCrxUpdateDownloadTimeout) {
         .Times(AtLeast(1));
     EXPECT_CALL(observer, OnEvent(Events::COMPONENT_UPDATE_ERROR,
                                   "jebgalgnebhfojomionfpkfelancnnkf"))
-        .Times(1);
+        .Times(1)
+        .WillOnce(Invoke([&update_client](Events event, const std::string& id) {
+          CrxUpdateItem item;
+          update_client->GetCrxUpdateState(id, &item);
+          EXPECT_EQ(ComponentState::kUpdateError, item.state);
+          EXPECT_EQ(1, item.error_category);
+          EXPECT_EQ(-118, item.error_code);
+          EXPECT_EQ(0, item.extra_code1);
+        }));
   }
   {
     InSequence seq;
@@ -1588,7 +1596,7 @@ TEST_F(UpdateClientTest, OneCrxInstallError) {
       EXPECT_EQ("jebgalgnebhfojomionfpkfelancnnkf", ping_data[0].id);
       EXPECT_EQ(base::Version("0.9"), ping_data[0].previous_version);
       EXPECT_EQ(base::Version("1.0"), ping_data[0].next_version);
-      EXPECT_EQ(3, ping_data[0].error_category);  // kInstallError.
+      EXPECT_EQ(3, ping_data[0].error_category);  // kInstall.
       EXPECT_EQ(9, ping_data[0].error_code);      // kInstallerError.
     }
   };
@@ -3103,9 +3111,11 @@ TEST_F(UpdateClientTest, OneCrxUpdateCheckFails) {
       const std::string id = "jebgalgnebhfojomionfpkfelancnnkf";
       EXPECT_EQ(id, ids_to_check.front());
       EXPECT_EQ(1u, components.count(id));
-
+      constexpr int update_check_error = -1;
+      components.at(id)->set_update_check_error(update_check_error);
       base::ThreadTaskRunnerHandle::Get()->PostTask(
-          FROM_HERE, base::BindOnce(std::move(update_check_callback), -1, 0));
+          FROM_HERE, base::BindOnce(std::move(update_check_callback),
+                                    update_check_error, 0));
     }
   };
 
@@ -3149,6 +3159,9 @@ TEST_F(UpdateClientTest, OneCrxUpdateCheckFails) {
         CrxUpdateItem item;
         update_client->GetCrxUpdateState(id, &item);
         EXPECT_EQ(ComponentState::kUpdateError, item.state);
+        EXPECT_EQ(5, item.error_category);
+        EXPECT_EQ(-1, item.error_code);
+        EXPECT_EQ(0, item.extra_code1);
       }));
 
   update_client->AddObserver(&observer);

components/update_client/update_engine.h

@@ -164,7 +164,9 @@ struct UpdateContext : public base::RefCounted<UpdateContext> {
   // update check.
   std::vector<std::string> components_to_check_for_updates;
 
+  // The error reported by the update checker.
   int update_check_error = 0;
+
   size_t num_components_ready_to_check = 0;
   size_t num_components_checked = 0;