[Extension]: Change CRX installer callback to report more error details.

These changes allow the new extension updater to capture more error details,
which would help investigate the significant discrepancies the update
results of the running Finch experiment.

Bug: 722942
Change-Id: I269bbadc71bf736a79e0aa95aed6d1fc78f4e288
Reviewed-on: https://chromium-review.googlesource.com/1065196Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Minh Nguyen <mxnguyen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561716}

chrome/browser/extensions/crx_installer.h

@@ -262,7 +262,7 @@ class CrxInstaller : public SandboxedUnpackerClient {
 
   // Called after OnUnpackSuccess as a last check to see whether the install
   // should complete.
-  CrxInstallError AllowInstall(const Extension* extension);
+  base::Optional<CrxInstallError> AllowInstall(const Extension* extension);
 
   // SandboxedUnpackerClient
   void OnUnpackFailure(const CrxInstallError& error) override;
@@ -302,7 +302,7 @@ class CrxInstaller : public SandboxedUnpackerClient {
   void ReportSuccessFromFileThread();
   void ReportSuccessFromUIThread();
   void NotifyCrxInstallBegin();
-  void NotifyCrxInstallComplete(bool success);
+  void NotifyCrxInstallComplete(const base::Optional<CrxInstallError>& error);
 
   // Deletes temporary directory and crx file if needed.
   void CleanupTempFiles();

chrome/browser/extensions/crx_installer_browsertest.cc

@@ -45,6 +45,7 @@
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/install/crx_install_error.h"
+#include "extensions/browser/install/sandboxed_unpacker_failure_reason.h"
 #include "extensions/browser/management_policy.h"
 #include "extensions/browser/notification_types.h"
 #include "extensions/common/extension.h"
@@ -296,9 +297,9 @@ class ExtensionCrxInstallerTest : public ExtensionBrowserTest {
 
   static void InstallerCallback(base::OnceClosure quit_closure,
                                 CrxInstaller::InstallerResultCallback callback,
-                                bool success) {
+                                const base::Optional<CrxInstallError>& error) {
     if (!callback.is_null())
-      std::move(callback).Run(success);
+      std::move(callback).Run(error);
     std::move(quit_closure).Run();
   }
 
@@ -546,7 +547,10 @@ IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTestWithExperimentalApis,
                        MAYBE_GrantScopes_WithCallback) {
   EXPECT_NO_FATAL_FAILURE(CheckHasEmptyScopesAfterInstall(
       "browsertest/scopes",
-      base::BindOnce([](bool success) { EXPECT_TRUE(success); }), true));
+      base::BindOnce([](const base::Optional<CrxInstallError>& error) {
+        EXPECT_EQ(base::nullopt, error);
+      }),
+      true));
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTestWithExperimentalApis,
@@ -559,7 +563,10 @@ IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTestWithExperimentalApis,
                        DoNotGrantScopes_WithCallback) {
   EXPECT_NO_FATAL_FAILURE(CheckHasEmptyScopesAfterInstall(
       "browsertest/scopes",
-      base::BindOnce([](bool success) { EXPECT_TRUE(success); }), false));
+      base::BindOnce([](const base::Optional<CrxInstallError>& error) {
+        EXPECT_EQ(base::nullopt, error);
+      }),
+      false));
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest, AllowOffStore) {
@@ -724,9 +731,12 @@ IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest,
   std::unique_ptr<WebstoreInstaller::Approval> approval =
       GetApproval("crx_installer/v2_no_permission_change/", id, false);
 
-  RunCrxInstaller(approval.get(), mock_prompt->CreatePrompt(),
-                  base::BindOnce([](bool success) { EXPECT_TRUE(success); }),
-                  test_data_dir_.AppendASCII("crx_installer/v1.crx"));
+  RunCrxInstaller(
+      approval.get(), mock_prompt->CreatePrompt(),
+      base::BindOnce([](const base::Optional<CrxInstallError>& error) {
+        EXPECT_EQ(base::nullopt, error);
+      }),
+      test_data_dir_.AppendASCII("crx_installer/v1.crx"));
 
   EXPECT_TRUE(mock_prompt->did_succeed());
 }
@@ -746,7 +756,13 @@ IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest,
   const std::string public_key = "123456";
   RunCrxInstallerFromUnpackedDirectory(
       mock_prompt->CreatePrompt(),
-      base::BindOnce([](bool success) { EXPECT_FALSE(success); }),
+      base::BindOnce([](const base::Optional<CrxInstallError>& error) {
+        ASSERT_NE(base::nullopt, error);
+        ASSERT_EQ(CrxInstallErrorType::SANDBOXED_UNPACKER_FAILURE,
+                  error->type());
+        EXPECT_EQ(SandboxedUnpackerFailureReason::DIRECTORY_MOVE_FAILED,
+                  error->sandbox_failure_detail());
+      }),
       std::string(), public_key, folder);
 
   EXPECT_FALSE(mock_prompt->did_succeed());
@@ -765,7 +781,13 @@ IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest,
   const std::string public_key = "123456";
   RunCrxInstallerFromUnpackedDirectory(
       mock_prompt->CreatePrompt(),
-      base::BindOnce([](bool success) { EXPECT_FALSE(success); }),
+      base::BindOnce([](const base::Optional<CrxInstallError>& error) {
+        ASSERT_NE(base::nullopt, error);
+        ASSERT_EQ(CrxInstallErrorType::SANDBOXED_UNPACKER_FAILURE,
+                  error->type());
+        EXPECT_EQ(SandboxedUnpackerFailureReason::UNPACKER_CLIENT_FAILED,
+                  error->sandbox_failure_detail());
+      }),
       std::string(), public_key, temp_dir.GetPath());
 
   EXPECT_FALSE(mock_prompt->did_succeed());
@@ -790,7 +812,13 @@ IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest,
   const std::string public_key = "123456";
   RunCrxInstallerFromUnpackedDirectory(
       mock_prompt->CreatePrompt(),
-      base::BindOnce([](bool success) { EXPECT_FALSE(success); }),
+      base::BindOnce([](const base::Optional<CrxInstallError>& error) {
+        ASSERT_NE(base::nullopt, error);
+        ASSERT_EQ(CrxInstallErrorType::SANDBOXED_UNPACKER_FAILURE,
+                  error->type());
+        EXPECT_EQ(SandboxedUnpackerFailureReason::INVALID_MANIFEST,
+                  error->sandbox_failure_detail());
+      }),
       std::string(), public_key, temp_dir.GetPath());
 
   EXPECT_FALSE(mock_prompt->did_succeed());
@@ -818,8 +846,10 @@ IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest, InstallUnpackedCrx_Success) {
       "y0ury28n8jbN0PnInKKWcxpIXXmNQyC19HBuO3QIeUq9Dqc+7YFQIDAQAB";
   RunCrxInstallerFromUnpackedDirectory(
       mock_prompt->CreatePrompt(),
-      base::BindOnce([](bool success) { EXPECT_TRUE(success); }), std::string(),
-      public_key, temp_dir.GetPath());
+      base::BindOnce([](const base::Optional<CrxInstallError>& error) {
+        EXPECT_EQ(base::nullopt, error);
+      }),
+      std::string(), public_key, temp_dir.GetPath());
 
   EXPECT_TRUE(mock_prompt->did_succeed());
   EXPECT_FALSE(base::PathExists(temp_dir.GetPath()));
@@ -840,10 +870,15 @@ IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest,
       "y0ury28n8jbN0PnInKKWcxpIXXmNQyC19HBuO3QIeUq9Dqc+7YFQIDAQAB";
   ASSERT_EQ(nullptr, GetInstalledExtension(extension_id));
   auto temp_dir = UnpackedCrxTempDir();
-  RunUpdateExtension(mock_prompt->CreatePrompt(), extension_id, public_key,
-                     temp_dir->GetPath(), base::BindOnce([](bool success) {
-                       EXPECT_FALSE(success);
-                     }));
+  RunUpdateExtension(
+      mock_prompt->CreatePrompt(), extension_id, public_key,
+      temp_dir->GetPath(),
+      base::BindOnce([](const base::Optional<CrxInstallError>& error) {
+        ASSERT_NE(base::nullopt, error);
+        EXPECT_EQ(CrxInstallErrorType::OTHER, error->type());
+        EXPECT_EQ(CrxInstallErrorDetail::UPDATE_NON_EXISTING_EXTENSION,
+                  error->detail());
+      }));
 
   // The unpacked folder should be deleted.
   EXPECT_FALSE(mock_prompt->did_succeed());
@@ -868,10 +903,12 @@ IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest,
   AddExtension(extension_id, "0.0");
 
   auto temp_dir = UnpackedCrxTempDir();
-  RunUpdateExtension(mock_prompt->CreatePrompt(), extension_id, public_key,
-                     temp_dir->GetPath(), base::BindOnce([](bool success) {
-                       EXPECT_TRUE(success);
-                     }));
+  RunUpdateExtension(
+      mock_prompt->CreatePrompt(), extension_id, public_key,
+      temp_dir->GetPath(),
+      base::BindOnce([](const base::Optional<CrxInstallError>& error) {
+        EXPECT_EQ(base::nullopt, error);
+      }));
 
   EXPECT_TRUE(mock_prompt->did_succeed());
 
@@ -896,10 +933,16 @@ IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest,
   AddExtension(extension_id, "0.0");
 
   auto temp_dir = UnpackedCrxTempDir();
-  RunUpdateExtension(mock_prompt->CreatePrompt(), extension_id, public_key,
-                     temp_dir->GetPath(), base::BindOnce([](bool success) {
-                       EXPECT_FALSE(success);
-                     }));
+  RunUpdateExtension(
+      mock_prompt->CreatePrompt(), extension_id, public_key,
+      temp_dir->GetPath(),
+      base::BindOnce([](const base::Optional<CrxInstallError>& error) {
+        ASSERT_NE(base::nullopt, error);
+        ASSERT_EQ(CrxInstallErrorType::SANDBOXED_UNPACKER_FAILURE,
+                  error->type());
+        EXPECT_EQ(SandboxedUnpackerFailureReason::INVALID_MANIFEST,
+                  error->sandbox_failure_detail());
+      }));
 
   EXPECT_FALSE(mock_prompt->did_succeed());
 
@@ -928,10 +971,14 @@ IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest,
   AddExtension(extension_id, "0.0");
 
   auto temp_dir = UnpackedCrxTempDir();
-  RunUpdateExtension(mock_prompt->CreatePrompt(), extension_id, public_key,
-                     temp_dir->GetPath(), base::BindOnce([](bool success) {
-                       EXPECT_FALSE(success);
-                     }));
+  RunUpdateExtension(
+      mock_prompt->CreatePrompt(), extension_id, public_key,
+      temp_dir->GetPath(),
+      base::BindOnce([](const base::Optional<CrxInstallError>& error) {
+        ASSERT_NE(base::nullopt, error);
+        EXPECT_EQ(CrxInstallErrorType::OTHER, error->type());
+        EXPECT_EQ(CrxInstallErrorDetail::UNEXPECTED_ID, error->detail());
+      }));
 
   EXPECT_FALSE(mock_prompt->did_succeed());
 

chrome/browser/extensions/updater/extension_cache_impl.cc

@@ -22,6 +22,7 @@
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "extensions/browser/install/crx_install_error.h"
+#include "extensions/browser/install/sandboxed_unpacker_failure_reason.h"
 
 namespace extensions {
 
@@ -121,22 +122,28 @@ void ExtensionCacheImpl::Observe(int type,
   const std::string& hash = installer->expected_hash();
   const extensions::CrxInstallError* error =
       content::Details<const extensions::CrxInstallError>(details).ptr();
-  switch (error->type()) {
-    case extensions::CrxInstallError::ERROR_DECLINED:
-      DVLOG(2) << "Extension install was declined, file kept";
-      break;
-    case extensions::CrxInstallError::ERROR_HASH_MISMATCH:
-      if (cache_->ShouldRetryDownload(id, hash)) {
-        cache_->RemoveExtension(id, hash);
-        installer->set_hash_check_failed(true);
-      }
-      // We deliberately keep the file with incorrect hash sum, so that it
-      // will not be re-downloaded each time.
-      break;
-    default:
+  const auto error_type = error->type();
+
+  if (error_type == extensions::CrxInstallErrorType::DECLINED) {
+    DVLOG(2) << "Extension install was declined, file kept";
+    return;
+  }
+
+  if (error_type ==
+          extensions::CrxInstallErrorType::SANDBOXED_UNPACKER_FAILURE &&
+      error->sandbox_failure_detail() ==
+          extensions::SandboxedUnpackerFailureReason::
+              CRX_HASH_VERIFICATION_FAILED) {
+    if (cache_->ShouldRetryDownload(id, hash)) {
       cache_->RemoveExtension(id, hash);
-      break;
+      installer->set_hash_check_failed(true);
+    }
+    // We deliberately keep the file with incorrect hash sum, so that it
+    // will not be re-downloaded each time.
+    return;
   }
+
+  cache_->RemoveExtension(id, hash);
 }
 
 }  // namespace extensions

chrome/browser/extensions/updater/extension_updater_unittest.cc

@@ -1642,7 +1642,8 @@ class ExtensionUpdaterTest : public testing::Test {
 
       // Fake install notice.  This should start the second installation,
       // which will be checked below.
-      fake_crx1->NotifyCrxInstallComplete(false);
+      fake_crx1->NotifyCrxInstallComplete(CrxInstallError(
+          CrxInstallErrorType::OTHER, CrxInstallErrorDetail::NONE));
 
       EXPECT_TRUE(updater.crx_install_is_running_);
     }
@@ -1655,7 +1656,8 @@ class ExtensionUpdaterTest : public testing::Test {
 
     if (updates_start_running) {
       EXPECT_TRUE(updater.crx_install_is_running_);
-      fake_crx2->NotifyCrxInstallComplete(false);
+      fake_crx2->NotifyCrxInstallComplete(CrxInstallError(
+          CrxInstallErrorType::OTHER, CrxInstallErrorDetail::NONE));
     }
     EXPECT_FALSE(updater.crx_install_is_running_);
   }

chrome/browser/infobars/infobars_browsertest.cc

@@ -337,7 +337,9 @@ void InfoBarUiTest::ShowUi(const std::string& name) {
       InstallationErrorInfoBarDelegate::Create(
           GetInfoBarService(),
           extensions::CrxInstallError(
-              extensions::CrxInstallError::ERROR_OFF_STORE, msg));
+              extensions::CrxInstallErrorType::OTHER,
+              extensions::CrxInstallErrorDetail::OFFSTORE_INSTALL_DISALLOWED,
+              msg));
       break;
     }
 

chrome/browser/ui/extensions/installation_error_infobar_delegate.cc

@@ -38,7 +38,9 @@ int InstallationErrorInfoBarDelegate::GetButtons() const {
 }
 
 base::string16 InstallationErrorInfoBarDelegate::GetLinkText() const {
-  return error_.type() == extensions::CrxInstallError::ERROR_OFF_STORE
+  return error_.type() == extensions::CrxInstallErrorType::OTHER &&
+                 error_.detail() == extensions::CrxInstallErrorDetail::
+                                        OFFSTORE_INSTALL_DISALLOWED
              ? l10n_util::GetStringUTF16(IDS_LEARN_MORE)
              : base::string16();
 }

extensions/browser/extension_system.h

@@ -9,8 +9,10 @@
 
 #include "base/callback_forward.h"
 #include "base/memory/ref_counted.h"
+#include "base/optional.h"
 #include "build/build_config.h"
 #include "components/keyed_service/core/keyed_service.h"
+#include "extensions/browser/install/crx_install_error.h"
 #include "extensions/buildflags/buildflags.h"
 #include "extensions/common/extension.h"
 
@@ -47,7 +49,8 @@ class ValueStoreFactory;
 class ExtensionSystem : public KeyedService {
  public:
   // A callback to be executed when InstallUpdate finishes.
-  using InstallUpdateCallback = base::OnceCallback<void(bool success)>;
+  using InstallUpdateCallback =
+      base::OnceCallback<void(const base::Optional<CrxInstallError>& result)>;
 
   ExtensionSystem();
   ~ExtensionSystem() override;

extensions/browser/install/BUILD.gn

@@ -9,9 +9,11 @@ assert(enable_extensions,
 
 source_set("install") {
   sources = [
+    "crx_install_error.cc",
     "crx_install_error.h",
     "extension_install_ui.cc",
     "extension_install_ui.h",
+    "sandboxed_unpacker_failure_reason.h",
   ]
 
   deps = [

extensions/browser/install/crx_install_error.cc

+// Copyright (c) 2018 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 "extensions/browser/install/crx_install_error.h"
+#include "extensions/browser/install/sandboxed_unpacker_failure_reason.h"
+
+namespace extensions {
+
+CrxInstallError::CrxInstallError(CrxInstallErrorType type,
+                                 CrxInstallErrorDetail detail,
+                                 const base::string16& message)
+    : type_(type), detail_(detail), message_(message) {
+  DCHECK_NE(CrxInstallErrorType::NONE, type);
+  DCHECK_NE(CrxInstallErrorType::SANDBOXED_UNPACKER_FAILURE, type);
+}
+
+CrxInstallError::CrxInstallError(CrxInstallErrorType type,
+                                 CrxInstallErrorDetail detail)
+    : CrxInstallError(type, detail, base::string16()) {}
+
+CrxInstallError::CrxInstallError(SandboxedUnpackerFailureReason reason,
+                                 const base::string16& message)
+    : type_(CrxInstallErrorType::SANDBOXED_UNPACKER_FAILURE),
+      detail_(CrxInstallErrorDetail::NONE),
+      sandbox_failure_detail_(reason),
+      message_(message) {}
+
+CrxInstallError::CrxInstallError(const CrxInstallError& other) = default;
+CrxInstallError::CrxInstallError(CrxInstallError&& other) = default;
+CrxInstallError& CrxInstallError::operator=(const CrxInstallError& other) =
+    default;
+CrxInstallError& CrxInstallError::operator=(CrxInstallError&& other) = default;
+
+CrxInstallError::~CrxInstallError() = default;
+
+// For SANDBOXED_UNPACKER_FAILURE type, use sandbox_failure_detail().
+CrxInstallErrorDetail CrxInstallError::detail() const {
+  DCHECK_NE(CrxInstallErrorType::SANDBOXED_UNPACKER_FAILURE, type_);
+  return detail_;
+}
+
+// sandbox_failure_detail() only returns a value when the error type is
+// SANDBOXED_UNPACKER_FAILURE.
+SandboxedUnpackerFailureReason CrxInstallError::sandbox_failure_detail() const {
+  DCHECK_EQ(CrxInstallErrorType::SANDBOXED_UNPACKER_FAILURE, type_);
+  DCHECK(sandbox_failure_detail_);
+  return sandbox_failure_detail_.value();
+}
+
+}  // namespace extensions

extensions/browser/install/crx_install_error.h

@@ -5,41 +5,82 @@
 #ifndef EXTENSIONS_BROWSER_INSTALL_CRX_INSTALL_ERROR_H_
 #define EXTENSIONS_BROWSER_INSTALL_CRX_INSTALL_ERROR_H_
 
+#include "base/optional.h"
 #include "base/strings/string16.h"
 
 namespace extensions {
 
-// Simple error class for CrxInstaller.
-class CrxInstallError {
- public:
-  // Typed errors that need to be handled specially by clients.
-  // ERROR_OFF_STORE for disallowed off-store installations.
-  // ERROR_DECLINED for situations when a .crx file seems to be OK, but there
+enum class SandboxedUnpackerFailureReason;
+
+// Typed errors that need to be handled specially by clients.
+// Do not change the order of the entries or remove entries in this list.
+enum class CrxInstallErrorType {
+  NONE = 0,
+  // DECLINED for situations when a .crx file seems to be OK, but there
   // are some policy restrictions or unmet dependencies that prevent it from
   // being installed.
-  // ERROR_HASH_MISMATCH if the expected extension SHA256 hash sum is different
-  // from the actual one.
-  enum Type {
-    ERROR_NONE,
-    ERROR_OFF_STORE,
-    ERROR_DECLINED,
-    ERROR_HASH_MISMATCH,
-    ERROR_OTHER
-  };
+  DECLINED = 1,
+  // SANDBOXED_UNPACKER_FAILURE for sandboxed unpacker error.
+  // CrxInstallErrorDetail will give more detail about the failure.
+  SANDBOXED_UNPACKER_FAILURE = 2,
+  OTHER = 3,
+};
 
-  CrxInstallError() : type_(ERROR_NONE) {}
+// Extended error code that may help explain the error type.
+// Do not change the order of the entries or remove entries in this list.
+enum class CrxInstallErrorDetail {
+  NONE,                                     // 0
+  CONVERT_USER_SCRIPT_TO_EXTENSION_FAILED,  // 1
+  UNEXPECTED_ID,                            // 2
+  UNEXPECTED_VERSION,                       // 3
+  MISMATCHED_VERSION,                       // 4
+  MANIFEST_INVALID,                         // 5
+  INSTALL_NOT_ENABLED,                      // 6
+  OFFSTORE_INSTALL_DISALLOWED,              // 7
+  INCORRECT_APP_CONTENT_TYPE,               // 8
+  NOT_INSTALLED_FROM_GALLERY,               // 9
+  INCORRECT_INSTALL_HOST,                   // 10
+  DEPENDENCY_NOT_SHARED_MODULE,             // 11
+  DEPENDENCY_OLD_VERSION,                   // 12
+  DEPENDENCY_NOT_ALLOWLISTED,               // 13
+  UNSUPPORTED_REQUIREMENTS,                 // 14
+  EXTENSION_IS_BLOCKLISTED,                 // 15
+  DISALLOWED_BY_POLICY,                     // 16
+  KIOSK_MODE_ONLY,                          // 17
+  OVERLAPPING_WEB_EXTENT,                   // 18
+  CANT_DOWNGRADE_VERSION,                   // 19
+  MOVE_DIRECTORY_TO_PROFILE_FAILED,         // 20
+  CANT_LOAD_EXTENSION,                      // 21
+  USER_CANCELED,                            // 22
+  USER_ABORTED,                             // 23
+  UPDATE_NON_EXISTING_EXTENSION,            // 24
+};
 
-  explicit CrxInstallError(const base::string16& message)
-      : type_(message.empty() ? ERROR_NONE : ERROR_OTHER), message_(message) {}
+// Simple error class for CrxInstaller.
+class CrxInstallError {
+ public:
+  CrxInstallError(CrxInstallErrorType type,
+                  CrxInstallErrorDetail detail,
+                  const base::string16& message);
+  CrxInstallError(CrxInstallErrorType type, CrxInstallErrorDetail detail);
+  CrxInstallError(SandboxedUnpackerFailureReason reason,
+                  const base::string16& message);
+  ~CrxInstallError();
 
-  CrxInstallError(Type type, const base::string16& message)
-      : type_(type), message_(message) {}
+  CrxInstallError(const CrxInstallError& other);
+  CrxInstallError(CrxInstallError&& other);
+  CrxInstallError& operator=(const CrxInstallError& other);
+  CrxInstallError& operator=(CrxInstallError&& other);
 
-  Type type() const { return type_; }
+  CrxInstallErrorType type() const { return type_; }
   const base::string16& message() const { return message_; }
+  CrxInstallErrorDetail detail() const;
+  SandboxedUnpackerFailureReason sandbox_failure_detail() const;
 
  private:
-  Type type_;
+  CrxInstallErrorType type_;
+  CrxInstallErrorDetail detail_;
+  base::Optional<SandboxedUnpackerFailureReason> sandbox_failure_detail_;
   base::string16 message_;
 };
 

extensions/browser/install/sandboxed_unpacker_failure_reason.h

+// Copyright (c) 2018 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.
+
+#ifndef EXTENSIONS_BROWSER_INSTALL_SANDBOXED_UNPACKER_FAILURE_REASON_H_
+#define EXTENSIONS_BROWSER_INSTALL_SANDBOXED_UNPACKER_FAILURE_REASON_H_
+
+namespace extensions {
+
+// Enumerate all the ways SandboxedUnpacker can fail.
+// Don't change the order or change the value of the enums.
+enum class SandboxedUnpackerFailureReason {
+  // SandboxedUnpacker::CreateTempDirectory()
+  COULD_NOT_GET_TEMP_DIRECTORY = 0,
+  COULD_NOT_CREATE_TEMP_DIRECTORY = 1,
+
+  // SandboxedUnpacker::Start()
+  FAILED_TO_COPY_EXTENSION_FILE_TO_TEMP_DIRECTORY = 2,
+  COULD_NOT_GET_SANDBOX_FRIENDLY_PATH = 3,
+
+  // SandboxedUnpacker::UnpackExtensionSucceeded()
+  COULD_NOT_LOCALIZE_EXTENSION = 4,
+  INVALID_MANIFEST = 5,
+
+  // SandboxedUnpacker::UnpackExtensionFailed()
+  UNPACKER_CLIENT_FAILED = 6,
+
+  // SandboxedUnpacker::UtilityProcessCrashed()
+  UTILITY_PROCESS_CRASHED_WHILE_TRYING_TO_INSTALL = 7,
+
+  // SandboxedUnpacker::ValidateSignature()
+  CRX_FILE_NOT_READABLE = 8,
+  CRX_HEADER_INVALID = 9,
+  CRX_MAGIC_NUMBER_INVALID = 10,
+  CRX_VERSION_NUMBER_INVALID = 11,
+  CRX_EXCESSIVELY_LARGE_KEY_OR_SIGNATURE = 12,
+  CRX_ZERO_KEY_LENGTH = 13,
+  CRX_ZERO_SIGNATURE_LENGTH = 14,
+  CRX_PUBLIC_KEY_INVALID = 15,
+  CRX_SIGNATURE_INVALID = 16,
+  CRX_SIGNATURE_VERIFICATION_INITIALIZATION_FAILED = 17,
+  CRX_SIGNATURE_VERIFICATION_FAILED = 18,
+
+  // SandboxedUnpacker::RewriteManifestFile()
+  ERROR_SERIALIZING_MANIFEST_JSON = 19,
+  ERROR_SAVING_MANIFEST_JSON = 20,
+
+  // SandboxedUnpacker::RewriteImageFiles()
+  COULD_NOT_READ_IMAGE_DATA_FROM_DISK_UNUSED = 21,
+  DECODED_IMAGES_DO_NOT_MATCH_THE_MANIFEST_UNUSED = 22,
+  INVALID_PATH_FOR_BROWSER_IMAGE = 23,
+  ERROR_REMOVING_OLD_IMAGE_FILE = 24,
+  INVALID_PATH_FOR_BITMAP_IMAGE = 25,
+  ERROR_RE_ENCODING_THEME_IMAGE = 26,
+  ERROR_SAVING_THEME_IMAGE = 27,
+  DEPRECATED_ABORTED_DUE_TO_SHUTDOWN = 28,  // No longer used; kept for UMA.
+
+  // SandboxedUnpacker::RewriteCatalogFiles()
+  COULD_NOT_READ_CATALOG_DATA_FROM_DISK_UNUSED = 29,
+  INVALID_CATALOG_DATA = 30,
+  INVALID_PATH_FOR_CATALOG_UNUSED = 31,
+  ERROR_SERIALIZING_CATALOG = 32,
+  ERROR_SAVING_CATALOG = 33,
+
+  // SandboxedUnpacker::ValidateSignature()
+  CRX_HASH_VERIFICATION_FAILED = 34,
+
+  UNZIP_FAILED = 35,
+  DIRECTORY_MOVE_FAILED = 36,
+
+  // SandboxedUnpacker::ValidateSignature()
+  CRX_FILE_IS_DELTA_UPDATE = 37,
+  CRX_EXPECTED_HASH_INVALID = 38,
+
+  // SandboxedUnpacker::IndexAndPersistRulesIfNeeded()
+  ERROR_PARSING_DNR_RULESET = 39,
+  ERROR_INDEXING_DNR_RULESET = 40,
+
+  NUM_FAILURE_REASONS
+};
+
+}  // namespace extensions
+
+#endif  // EXTENSIONS_BROWSER_INSTALL_SANDBOXED_UNPACKER_FAILURE_REASON_H_

extensions/browser/sandboxed_unpacker.h

@@ -37,6 +37,7 @@ class Connector;
 
 namespace extensions {
 class Extension;
+enum class SandboxedUnpackerFailureReason;
 
 class SandboxedUnpackerClient
     : public base::RefCountedDeleteOnSequence<SandboxedUnpackerClient> {
@@ -98,7 +99,7 @@ class SandboxedUnpackerClient
 //
 class SandboxedUnpacker : public base::RefCountedThreadSafe<SandboxedUnpacker> {
  public:
-  // Creates a SanboxedUnpacker that will do work to unpack an extension,
+  // Creates a SandboxedUnpacker that will do work to unpack an extension,
   // passing the |location| and |creation_flags| to Extension::Create. The
   // |extensions_dir| parameter should specify the directory under which we'll
   // create a subdirectory to write the unpacked extension contents.
@@ -132,79 +133,6 @@ class SandboxedUnpacker : public base::RefCountedThreadSafe<SandboxedUnpacker> {
  private:
   friend class base::RefCountedThreadSafe<SandboxedUnpacker>;
 
-  // Enumerate all the ways unpacking can fail.  Calls to ReportFailure()
-  // take a failure reason as an argument, and put it in histogram
-  // Extensions.SandboxUnpackFailureReason.
-  enum FailureReason {
-    // SandboxedUnpacker::CreateTempDirectory()
-    COULD_NOT_GET_TEMP_DIRECTORY,
-    COULD_NOT_CREATE_TEMP_DIRECTORY,
-
-    // SandboxedUnpacker::Start()
-    FAILED_TO_COPY_EXTENSION_FILE_TO_TEMP_DIRECTORY,
-    COULD_NOT_GET_SANDBOX_FRIENDLY_PATH,
-
-    // SandboxedUnpacker::UnpackExtensionSucceeded()
-    COULD_NOT_LOCALIZE_EXTENSION,
-    INVALID_MANIFEST,
-
-    // SandboxedUnpacker::UnpackExtensionFailed()
-    UNPACKER_CLIENT_FAILED,
-
-    // SandboxedUnpacker::UtilityProcessCrashed()
-    UTILITY_PROCESS_CRASHED_WHILE_TRYING_TO_INSTALL,
-
-    // SandboxedUnpacker::ValidateSignature()
-    CRX_FILE_NOT_READABLE,
-    CRX_HEADER_INVALID,
-    CRX_MAGIC_NUMBER_INVALID,
-    CRX_VERSION_NUMBER_INVALID,
-    CRX_EXCESSIVELY_LARGE_KEY_OR_SIGNATURE,
-    CRX_ZERO_KEY_LENGTH,
-    CRX_ZERO_SIGNATURE_LENGTH,
-    CRX_PUBLIC_KEY_INVALID,
-    CRX_SIGNATURE_INVALID,
-    CRX_SIGNATURE_VERIFICATION_INITIALIZATION_FAILED,
-    CRX_SIGNATURE_VERIFICATION_FAILED,
-
-    // SandboxedUnpacker::RewriteManifestFile()
-    ERROR_SERIALIZING_MANIFEST_JSON,
-    ERROR_SAVING_MANIFEST_JSON,
-
-    // SandboxedUnpacker::RewriteImageFiles()
-    COULD_NOT_READ_IMAGE_DATA_FROM_DISK_UNUSED,
-    DECODED_IMAGES_DO_NOT_MATCH_THE_MANIFEST_UNUSED,
-    INVALID_PATH_FOR_BROWSER_IMAGE,
-    ERROR_REMOVING_OLD_IMAGE_FILE,
-    INVALID_PATH_FOR_BITMAP_IMAGE,
-    ERROR_RE_ENCODING_THEME_IMAGE,
-    ERROR_SAVING_THEME_IMAGE,
-    DEPRECATED_ABORTED_DUE_TO_SHUTDOWN,  // No longer used; kept for UMA.
-
-    // SandboxedUnpacker::RewriteCatalogFiles()
-    COULD_NOT_READ_CATALOG_DATA_FROM_DISK_UNUSED,
-    INVALID_CATALOG_DATA,
-    INVALID_PATH_FOR_CATALOG_UNUSED,
-    ERROR_SERIALIZING_CATALOG,
-    ERROR_SAVING_CATALOG,
-
-    // SandboxedUnpacker::ValidateSignature()
-    CRX_HASH_VERIFICATION_FAILED,
-
-    UNZIP_FAILED,
-    DIRECTORY_MOVE_FAILED,
-
-    // SandboxedUnpacker::ValidateSignature()
-    CRX_FILE_IS_DELTA_UPDATE,
-    CRX_EXPECTED_HASH_INVALID,
-
-    // SandboxedUnpacker::IndexAndPersistRulesIfNeeded()
-    ERROR_PARSING_DNR_RULESET,
-    ERROR_INDEXING_DNR_RULESET,
-
-    NUM_FAILURE_REASONS
-  };
-
   friend class SandboxedUnpackerTest;
 
   ~SandboxedUnpacker();
@@ -213,8 +141,9 @@ class SandboxedUnpacker : public base::RefCountedThreadSafe<SandboxedUnpacker> {
   bool CreateTempDirectory();
 
   // Helper functions to simplify calling ReportFailure.
-  base::string16 FailureReasonToString16(FailureReason reason);
-  void FailWithPackageError(FailureReason reason);
+  base::string16 FailureReasonToString16(
+      const SandboxedUnpackerFailureReason reason);
+  void FailWithPackageError(const SandboxedUnpackerFailureReason reason);
 
   // Validates the signature of the extension and extract the key to
   // |public_key_|. True if the signature validates, false otherwise.
@@ -261,7 +190,11 @@ class SandboxedUnpacker : public base::RefCountedThreadSafe<SandboxedUnpacker> {
   // Reports unpack success or failure, or unzip failure.
   void ReportSuccess(std::unique_ptr<base::DictionaryValue> original_manifest,
                      const base::Optional<int>& dnr_ruleset_checksum);
-  void ReportFailure(FailureReason reason, const base::string16& error);
+
+  // Puts a sanboxed unpacker failure in histogram
+  // Extensions.SandboxUnpackFailureReason.
+  void ReportFailure(const SandboxedUnpackerFailureReason reason,
+                     const base::string16& error);
 
   // Overwrites original manifest with safe result from utility process.
   // Returns NULL on error. Caller owns the returned object.

extensions/browser/updater/update_data_provider.cc

@@ -10,9 +10,10 @@
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
+#include "base/optional.h"
 #include "base/strings/string_util.h"
 #include "base/task_scheduler/post_task.h"
-#include "components/update_client/update_client.h"
+#include "components/update_client/utils.h"
 #include "content/public/browser/browser_thread.h"
 #include "crypto/sha2.h"
 #include "extensions/browser/content_verifier.h"
@@ -20,6 +21,7 @@
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/extensions_browser_client.h"
+#include "extensions/browser/install/crx_install_error.h"
 #include "extensions/browser/updater/manifest_fetch_data.h"
 #include "extensions/common/extension.h"
 
@@ -34,17 +36,26 @@ void InstallUpdateCallback(content::BrowserContext* context,
                            const std::string& public_key,
                            const base::FilePath& unpacked_dir,
                            UpdateClientCallback update_client_callback) {
-  using InstallError = update_client::InstallError;
-  using Result = update_client::CrxInstaller::Result;
-
+  // Note that error codes are converted into custom error codes, which are all
+  // based on a constant (see ToInstallerResult). This means that custom codes
+  // from different embedders may collide. However, for any given extension ID,
+  // there should be only one embedder, so this should be OK from Omaha.
   ExtensionSystem::Get(context)->InstallUpdate(
       extension_id, public_key, unpacked_dir,
       base::BindOnce(
-          [](UpdateClientCallback update_client_callback, bool success) {
+          [](UpdateClientCallback callback,
+             const base::Optional<CrxInstallError>& error) {
             DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-            std::move(update_client_callback)
-                .Run(Result(success ? InstallError::NONE
-                                    : InstallError::GENERIC_ERROR));
+            update_client::CrxInstaller::Result result(0);
+            if (error.has_value()) {
+              int detail =
+                  error->type() ==
+                          CrxInstallErrorType::SANDBOXED_UNPACKER_FAILURE
+                      ? static_cast<int>(error->sandbox_failure_detail())
+                      : static_cast<int>(error->detail());
+              result = update_client::ToInstallerResult(error->type(), detail);
+            }
+            std::move(callback).Run(result);
           },
           std::move(update_client_callback)));
 }

extensions/browser/updater/update_service_unittest.cc

@@ -237,7 +237,7 @@ class FakeExtensionSystem : public MockExtensionSystem {
     if (!next_install_callback_.is_null()) {
       std::move(next_install_callback_).Run();
     }
-    std::move(install_update_callback).Run(true);
+    std::move(install_update_callback).Run(base::nullopt);
   }
 
  private: