Replace ExtensionInstallChecker with generic PreloadCheckGroup

chrome/browser/extensions/crx_installer.cc

 // Copyright (c) 2012 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 "chrome/browser/extensions/crx_installer.h"
 
 #include <map>
 #include <set>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sequenced_task_runner.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
 #include "base/version.h"
 #include "build/build_config.h"
+#include "chrome/browser/extensions/blacklist_check.h"
 #include "chrome/browser/extensions/convert_user_script.h"
 #include "chrome/browser/extensions/convert_web_app.h"
 #include "chrome/browser/extensions/extension_assets_manager.h"
 #include "chrome/browser/extensions/extension_error_reporter.h"
-#include "chrome/browser/extensions/extension_install_checker.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/install_tracker.h"
 #include "chrome/browser/extensions/install_tracker_factory.h"
 #include "chrome/browser/extensions/permissions_updater.h"
 #include "chrome/browser/extensions/webstore_installer.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/web_applications/web_app.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/grit/generated_resources.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/resource_dispatcher_host.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/install/crx_install_error.h"
 #include "extensions/browser/install/extension_install_ui.h"
 #include "extensions/browser/install_flag.h"
 #include "extensions/browser/notification_types.h"
-#include "extensions/browser/preload_check.h"
+#include "extensions/browser/policy_check.h"
+#include "extensions/browser/preload_check_group.h"
+#include "extensions/browser/requirements_checker.h"
 #include "extensions/common/extension_icon_set.h"
 #include "extensions/common/file_util.h"
 #include "extensions/common/manifest.h"
 #include "extensions/common/manifest_handlers/kiosk_mode_info.h"
 #include "extensions/common/manifest_handlers/shared_module_info.h"
 #include "extensions/common/manifest_url_handlers.h"
 #include "extensions/common/permissions/permission_message_provider.h"
 #include "extensions/common/permissions/permission_set.h"
 #include "extensions/common/permissions/permissions_data.h"
 #include "extensions/common/user_script.h"
@@ skipping 438 matching lines @@
             CrxInstallError::ERROR_DECLINED,
             l10n_util::GetStringFUTF16(
                 IDS_EXTENSION_INSTALL_DEPENDENCY_NOT_WHITELISTED,
                 base::UTF8ToUTF16(extension()->name()),
                 base::UTF8ToUTF16(imported_module->name()))));
         return;
       }
     }
   }
 
-  // Run the policy, requirements and blacklist checks in parallel. Skip the
-  // checks if the extension is a bookmark app.
+  // Skip the checks if the extension is a bookmark app.
   if (extension()->from_bookmark()) {
     ConfirmInstall();
-  } else {
-    install_checker_ = base::MakeUnique<ExtensionInstallChecker>(
-        profile_, extension_, ExtensionInstallChecker::CHECK_ALL,
-        false /* fail fast */);
-    install_checker_->Start(
-        base::Bind(&CrxInstaller::OnInstallChecksComplete, this));
+    return;
   }
+
+  // Run the policy, requirements and blacklist checks in parallel.
+  check_group_ = base::MakeUnique<PreloadCheckGroup>();
+
+  policy_check_ = base::MakeUnique<PolicyCheck>(profile_, extension());
+  requirements_check_ = base::MakeUnique<RequirementsChecker>(extension());
+  blacklist_check_ =
+      base::MakeUnique<BlacklistCheck>(Blacklist::Get(profile_), extension_);
+
+  check_group_->AddCheck(policy_check_.get());
+  check_group_->AddCheck(requirements_check_.get());
+  check_group_->AddCheck(blacklist_check_.get());
+
+  check_group_->Start(
+      base::BindOnce(&CrxInstaller::OnInstallChecksComplete, this));
 }
 
-void CrxInstaller::OnInstallChecksComplete(int failed_checks) {
+void CrxInstaller::OnInstallChecksComplete(PreloadCheck::Errors errors) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (!service_weak_)
     return;
 
+  if (errors.empty()) {
+    ConfirmInstall();
+    return;
+  }
+
   // Check for requirement errors.
-  if (!install_checker_->requirements_error_message().empty()) {
+  if (!requirements_check_->GetErrorMessage().empty()) {

[Devlin, 2017/04/06 02:03:53]

I know I mentioned before that it's weird to sometimes consult
GetErrorMessage(), other times consult |errors| in a different CL... do you have
any ideas in the pipeline to make it a bit less unusual?  (Not something that
needs to be done for this CL, but I do think it would be nice to have something
more clear).

[michaelpg, 2017/04/06 22:31:16]

:-\ Both pieces of info are useful. We could add a standard HadSomeError()
method (adding more redundancy), or we could explicitly check for every error
type (there are 3 requirements errors we'd check for). I see why it's weird, but
I haven't thought of anything clearly better.

The utility of PreloadCheck[Group] is outsourcing the boilerplate sync/async
check/check all code. At the end of the day we still have to allow for examining
each error and taking action individually.

[Devlin, 2017/04/07 14:33:57]

One thought I had is that we could make an Error struct
PreloadCheck::Error {
  ErrorCode code;
  base::string16 message;
}

That way, we still have all the utility have having code + error, and we don't
have the weirdness of sometimes checking errors.contains(foo) and sometimes
checking PreloadCheck::GetErrorMessage().

A few drawbacks:
- This doesn't work when we coalesce multiple errors from a preload check into a
single error message, like we do in RequirementsChecker.  Instead, that would be
the responsibility of the callback (or a utility method called by the callback).
- errors.count(SOME_ERROR) doesn't work, unless we have an operator==(ErrorCode)
on Error, or we make an ErrorSet class.

Overall, this might lead to overarchitecting.  For the time being, I'm fine with
this, but if/when we add more of these checks, it will get increasingly weird. 
Do you have an idea how many of these checks we'll have?

     if (error_on_unsupported_requirements_) {
       ReportFailureFromUIThread(
           CrxInstallError(CrxInstallError::ERROR_DECLINED,
-                          install_checker_->requirements_error_message()));
+                          requirements_check_->GetErrorMessage()));
       return;
     }
     install_flags_ |= kInstallFlagHasRequirementErrors;
   }
 
   // Check the blacklist state.
-  if (install_checker_->blacklist_error() == PreloadCheck::BLACKLISTED_ID) {
-    install_flags_ |= kInstallFlagIsBlacklistedForMalware;
+  if (errors.count(PreloadCheck::BLACKLISTED_ID) ||
+      errors.count(PreloadCheck::BLACKLISTED_UNKNOWN)) {
+    if (allow_silent_install_) {
+      // NOTE: extension may still be blacklisted, but we're forced to silently
+      // install it. In this case, ExtensionService::OnExtensionInstalled needs
+      // to deal with it.
+      if (errors.count(PreloadCheck::BLACKLISTED_ID))
+        install_flags_ |= kInstallFlagIsBlacklistedForMalware;
+    } else {
+      // User tried to install a blacklisted extension. Show an error and
+      // refuse to install it.
+      ReportFailureFromUIThread(CrxInstallError(
+          CrxInstallError::ERROR_DECLINED,
+          l10n_util::GetStringFUTF16(IDS_EXTENSION_IS_BLACKLISTED,
+                                     base::UTF8ToUTF16(extension()->name()))));
+      UMA_HISTOGRAM_ENUMERATION("ExtensionBlacklist.BlockCRX",
+                                extension()->location(),
+                                Manifest::NUM_LOCATIONS);
+      return;
+    }
   }
 
-  if ((install_checker_->blacklist_error() == PreloadCheck::BLACKLISTED_ID ||
-       install_checker_->blacklist_error() ==
-           PreloadCheck::BLACKLISTED_UNKNOWN) &&
-      !allow_silent_install_) {
-    // User tried to install a blacklisted extension. Show an error and
-    // refuse to install it.
-    ReportFailureFromUIThread(CrxInstallError(
-        CrxInstallError::ERROR_DECLINED,
-        l10n_util::GetStringFUTF16(IDS_EXTENSION_IS_BLACKLISTED,
-                                   base::UTF8ToUTF16(extension()->name()))));
-    UMA_HISTOGRAM_ENUMERATION("ExtensionBlacklist.BlockCRX",
-                              extension()->location(),
-                              Manifest::NUM_LOCATIONS);
-    return;
-  }
-
-  // NOTE: extension may still be blacklisted, but we're forced to silently
-  // install it. In this case, ExtensionService::OnExtensionInstalled needs to
-  // deal with it.
-
   // Check for policy errors.
-  if (!install_checker_->policy_error().empty()) {
+  if (errors.count(PreloadCheck::DISALLOWED_BY_POLICY)) {
     // We don't want to show the error infobar for installs from the WebStore,
     // because the WebStore already shows an error dialog itself.
     // Note: |client_| can be NULL in unit_tests!
     if (extension()->from_webstore() && client_)
       client_->install_ui()->SetSkipPostInstallUI(true);
+
     ReportFailureFromUIThread(CrxInstallError(
-        CrxInstallError::ERROR_DECLINED, install_checker_->policy_error()));
+        CrxInstallError::ERROR_DECLINED, policy_check_->GetErrorMessage()));
     return;
   }
 
   ConfirmInstall();
 }
 
 void CrxInstaller::ConfirmInstall() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   ExtensionService* service = service_weak_.get();
   if (!service || service->browser_terminating())
@@ skipping 322 matching lines @@
         ExtensionInstallPrompt::GetReEnablePromptTypeForExtension(
             service->profile(), extension());
     client_->ShowDialog(base::Bind(&CrxInstaller::OnInstallPromptDone, this),
                         extension(), nullptr,
                         base::MakeUnique<ExtensionInstallPrompt::Prompt>(type),
                         ExtensionInstallPrompt::GetDefaultShowDialogCallback());
   }
 }
 
 }  // namespace extensions