Update ExtensionInstallChecker to use PreloadCheck classes

chrome/browser/extensions/extension_install_checker.cc

Index: chrome/browser/extensions/extension_install_checker.cc
diff --git a/chrome/browser/extensions/extension_install_checker.cc b/chrome/browser/extensions/extension_install_checker.cc
index d027389d6e26b672969e8e8c84a9c644216ccfc1..ff294ed02fc57a5fcf4621e322bc47980200ae70 100644
--- a/chrome/browser/extensions/extension_install_checker.cc
+++ b/chrome/browser/extensions/extension_install_checker.cc
@@ -4,14 +4,17 @@
 
 #include "chrome/browser/extensions/extension_install_checker.h"
 
+#include "base/bind_helpers.h"
 #include "base/callback_helpers.h"
+#include "base/memory/ptr_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/extensions/blacklist.h"
+#include "chrome/browser/extensions/blacklist_check.h"
 #include "chrome/browser/extensions/chrome_requirements_checker.h"
 #include "chrome/browser/profiles/profile.h"
 #include "content/public/browser/browser_thread.h"
 #include "extensions/browser/extension_system.h"
-#include "extensions/browser/management_policy.h"
+#include "extensions/browser/policy_check.h"
 
 namespace extensions {
 
@@ -22,12 +25,10 @@ ExtensionInstallChecker::ExtensionInstallChecker(
     bool fail_fast)
     : profile_(profile),
       extension_(extension),
-      blacklist_state_(NOT_BLACKLISTED),
-      policy_allows_load_(true),
+      blacklist_error_(PreloadCheck::NONE),
       enabled_checks_(enabled_checks),
       running_checks_(0),
-      fail_fast_(fail_fast),
-      weak_ptr_factory_(this) {}
+      fail_fast_(fail_fast) {}
 
 ExtensionInstallChecker::~ExtensionInstallChecker() {
 }
@@ -71,18 +72,17 @@ void ExtensionInstallChecker::Start(const Callback& callback) {
 void ExtensionInstallChecker::CheckManagementPolicy() {
   DCHECK(extension_.get());
 
-  base::string16 error;
-  bool allow = ExtensionSystem::Get(profile_)->management_policy()->UserMayLoad(
-      extension_.get(), &error);
-  OnManagementPolicyCheckDone(allow, base::UTF16ToUTF8(error));
+  if (!policy_check_)

[Devlin, 2017/03/22 15:08:46]

As a single use class, when would policy_check_ already exist?

[michaelpg, 2017/03/24 02:34:16]

In tests; added comment.

+    policy_check_ = base::MakeUnique<PolicyCheck>(profile_, extension_.get());
+  policy_check_->Start(
+      base::BindOnce(&ExtensionInstallChecker::OnManagementPolicyCheckDone,
+                     base::Unretained(this)));

[Devlin, 2017/03/22 15:08:46]

document why base::Unretained is safe.

That said, in cases like these, it might almost just be better to use a weak
ptr.  It can sometimes require significant inner knowledge of the class to know
whether or not a use like this will be safe, since a possible
PreloadCheck::Start implementation could be something like:

namespace {

void TransformErrors(callback, std::string error) {
  callback.Run(StringErrorToPreloadCheckError(error));
}

}

SomePreloadCheck::Start(callback) {
  base::PostTaskAndReply(
      base::Bind(&GetSomeErrorsFromSomethingElse),
      base::Bind(&TransformErrors, callback));
}

If I were reviewing that code, it wouldn't immediately strike me as unsafe, and
isolating the TransformErrors call to an anonymous function would probably be
good practice.  But with this Unretained call, it means that if the
ExtensionInstallChecker was deleted, the callback would still be run and it
would be a UAF.

To avoid needing those audits (and needing to know when changing the preload
check impls), it's probably just nicer to use a weak ptr (guaranteed safe) here.

[michaelpg, 2017/03/24 02:34:16]

OK, converted to weak_ptr. You're right that base::Unretained makes an
assumption the PreloadChecks don't have to adhere to.

 }
 
 void ExtensionInstallChecker::OnManagementPolicyCheckDone(
-    bool allows_load,
-    const std::string& error) {
-  policy_allows_load_ = allows_load;
-  policy_error_ = error;
-  DCHECK(policy_allows_load_ || !policy_error_.empty());
+    PreloadCheck::Errors errors) {
+  if (errors.count(PreloadCheck::DISALLOWED_BY_POLICY))
+    policy_error_ = base::UTF16ToUTF8(policy_check_->GetErrorMessage());
 
   running_checks_ &= ~CHECK_MANAGEMENT_POLICY;
   MaybeInvokeCallback();
@@ -94,7 +94,7 @@ void ExtensionInstallChecker::CheckRequirements() {
   requirements_checker_ = base::MakeUnique<ChromeRequirementsChecker>();
   requirements_checker_->Check(
       extension_, base::Bind(&ExtensionInstallChecker::OnRequirementsCheckDone,
-                             weak_ptr_factory_.GetWeakPtr()));
+                             base::Unretained(this)));

[Devlin, 2017/03/22 15:08:46]

ditto re weak ptr

[michaelpg, 2017/03/24 02:34:16]

Done.

 }
 
 void ExtensionInstallChecker::OnRequirementsCheckDone(
@@ -110,17 +110,23 @@ void ExtensionInstallChecker::OnRequirementsCheckDone(
 void ExtensionInstallChecker::CheckBlacklistState() {
   DCHECK(extension_.get());
 
-  extensions::Blacklist* blacklist = Blacklist::Get(profile_);
-  blacklist->IsBlacklisted(
-      extension_->id(),
-      base::Bind(&ExtensionInstallChecker::OnBlacklistStateCheckDone,
-                 weak_ptr_factory_.GetWeakPtr()));
+  if (!blacklist_check_) {

[Devlin, 2017/03/22 15:08:46]

ditto re check initialization

[michaelpg, 2017/03/24 02:34:16]

Acknowledged.

+    blacklist_check_ = base::MakeUnique<BlacklistCheck>(
+        Blacklist::Get(profile_), extension_.get());
+  }
+  blacklist_check_->Start(
+      base::BindOnce(&ExtensionInstallChecker::OnBlacklistStateCheckDone,
+                     base::Unretained(this)));

[Devlin, 2017/03/22 15:08:46]

ditto re weak ptr

[michaelpg, 2017/03/24 02:34:16]

Done.

 }
 
-void ExtensionInstallChecker::OnBlacklistStateCheckDone(BlacklistState state) {
+void ExtensionInstallChecker::OnBlacklistStateCheckDone(
+    PreloadCheck::Errors errors) {
   DCHECK(is_running());
 
-  blacklist_state_ = state;
+  if (errors.empty())
+    blacklist_error_ = PreloadCheck::NONE;
+  else
+    blacklist_error_ = *errors.begin();

[Devlin, 2017/03/22 15:08:46]

Maybe DCHECK_EQ(1u, errors.size())?

[michaelpg, 2017/03/24 02:34:16]

Done.

 
   running_checks_ &= ~CHECK_BLACKLIST;
   MaybeInvokeCallback();
@@ -132,19 +138,22 @@ void ExtensionInstallChecker::MaybeInvokeCallback() {
 
   // Set bits for failed checks.
   int failed_mask = 0;
-  if (blacklist_state_ == BLACKLISTED_MALWARE)
+  if (blacklist_error_ == PreloadCheck::BLACKLISTED_ID)
     failed_mask |= CHECK_BLACKLIST;
   if (!requirement_errors_.empty())
     failed_mask |= CHECK_REQUIREMENTS;
-  if (!policy_allows_load_)
+  if (!policy_error_.empty())
     failed_mask |= CHECK_MANAGEMENT_POLICY;
 
   // Invoke callback if all checks are complete or there was at least one
   // failure and |fail_fast_| is true.
   if (!is_running() || (failed_mask && fail_fast_)) {
-    // If we are failing fast, discard any pending results.
-    weak_ptr_factory_.InvalidateWeakPtrs();
     running_checks_ = 0;
+
+    // If we are failing fast, discard any pending results.
+    blacklist_check_.reset();
+    policy_check_.reset();
+    requirements_checker_.reset();
     base::ResetAndReturn(&callback_).Run(failed_mask);
   }
 }