Update ExtensionInstallChecker to use PreloadCheck classes

Update ExtensionInstallChecker to use PreloadCheck classes

Replaces some ad-hoc checks with their encapsulated equivalents.

Later, the individual OnFooCheckDone methods will be eliminated too.

BUG=679971
R=rdevlin.cronin@chromium.org

Review-Url: https://codereview.chromium.org/2768723002
Cr-Commit-Position: refs/heads/master@{#459621}
Committed: https://chromium.googlesource.com/chromium/src/+/2eb513ed005d6b23489f457d997d944b6fa089d8

[michaelpg, 2017-03-22 00:40:36]

Hopefully a more straightforward follow-up for you.

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

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_checker.cc (right):

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:75: if (!policy_check_)
As a single use class, when would policy_check_ already exist?

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:79:
base::Unretained(this)));
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.

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:97:
base::Unretained(this)));
ditto re weak ptr

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:113: if
(!blacklist_check_) {
ditto re check initialization

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:119:
base::Unretained(this)));
ditto re weak ptr

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:129: blacklist_error_ =
*errors.begin();
Maybe DCHECK_EQ(1u, errors.size())?

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_checker.h (right):

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.h:69: // BLACKLISTED_MALWARE
is considered to be a check failure.
BLACKLISTED_ID

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_checker_unittest.cc (right):

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker_unittest.cc:73:
policy_check_called_ = true;
(Mostly just thinking out loud) I'm a little dubious of how useful these checks
are.  Given we can supply the preload check now, I'd probably prefer removing
the virtualness of these functions and relying on the supplied check to do the
right thing, and inferring whether or not the check was called through other
ways (e.g., does a checker correctly pass or fail with running checks and
certain assumptions).  WDYT?

Regardless, not something that needs to be done in this CL.

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker_unittest.cc:92: class
ExtensionInstallCheckerAsync : public ExtensionInstallCheckerForTest {
Given how small this class is, I'd just expose a set_async method on
ExtensionInstallCheckerForTest.

https://codereview.chromium.org/2768723002/diff/20001/extensions/browser/prel...
File extensions/browser/preload_check_test_util.cc (right):

https://codereview.chromium.org/2768723002/diff/20001/extensions/browser/prel...
extensions/browser/preload_check_test_util.cc:69:
base::Bind(&PreloadCheckStub::StartInternal,
Another option would be to do
PostTask(FROM_HERE, base::Bind(callback, errors_));

Two potential concerns:
- Errors added after Start() don't get used.  That's probably fine (seems wrong
to do that).
- Calling the callback is no longer bound to the lifetime of this object.

If you don't care about those, I'd say go with the simpler approach and nix
StartInternal().  If you do, add a comment why.

https://codereview.chromium.org/2768723002/diff/20001/extensions/browser/prel...
extensions/browser/preload_check_test_util.cc:76: void
PreloadCheckStub::StartInternal(ResultCallback callback) {
StartInternal is a funny name for something that deterministically ends the flow
:P  Maybe Finish()?

https://codereview.chromium.org/2768723002/diff/20001/extensions/browser/prel...
File extensions/browser/preload_check_test_util.h (right):

https://codereview.chromium.org/2768723002/diff/20001/extensions/browser/prel...
extensions/browser/preload_check_test_util.h:60: explicit PreloadCheckStub(bool
is_async = false);
Maybe an enum, SYNCHRONOUS and ASYNCHRONOUS?  Makes the call sites much more
readable.

[michaelpg, 2017-03-24 02:34:17]

Thanks. PTAL.

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_checker.cc (right):

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:75: if (!policy_check_)
On 2017/03/22 15:08:46, Devlin wrote:
> As a single use class, when would policy_check_ already exist?

In tests; added comment.

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:79:
base::Unretained(this)));
On 2017/03/22 15:08:46, Devlin wrote:
> 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.

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

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:97:
base::Unretained(this)));
On 2017/03/22 15:08:46, Devlin wrote:
> ditto re weak ptr

Done.

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:113: if
(!blacklist_check_) {
On 2017/03/22 15:08:46, Devlin wrote:
> ditto re check initialization

Acknowledged.

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:119:
base::Unretained(this)));
On 2017/03/22 15:08:46, Devlin wrote:
> ditto re weak ptr

Done.

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:129: blacklist_error_ =
*errors.begin();
On 2017/03/22 15:08:46, Devlin wrote:
> Maybe DCHECK_EQ(1u, errors.size())?

Done.

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_checker.h (right):

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.h:69: // BLACKLISTED_MALWARE
is considered to be a check failure.
On 2017/03/22 15:08:46, Devlin wrote:
> BLACKLISTED_ID

I see why this is confusing, so I've changed the comment. The blacklist_error_
is a PreloadCheck::Error, not a BlacklistState. The error will be NONE,
BLACKLISTED_ID, or BLACKLISTED_UNKNOWN.

The error is set to BLACKLISTED_ID if the BlacklistState was
BLACKLISTED_MALWARE. That is, the "greylist" states like
BLACKLISTED_SECURITY_VULNERABILITY do not cause us to set a PreloadCheck::Error.
(This is how this class works today, so it's not a behavior change.)

The reason I didn't call the PreloadCheck::Error enumerator
"BLACKLISTED_MALWARE" instead of BLACKLISTED_ID is to avoid coupling the error
to the blacklist state. Should we decide the greylists should cause install
errors too, we can update the BlacklistCheck class to return BLACKLISTED_ID for
those too, and not have to update every client of BlacklistCheck (including
clients of ExtensionInstallCheck) to handle the additional errors.

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_checker_unittest.cc (right):

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker_unittest.cc:73:
policy_check_called_ = true;
On 2017/03/22 15:08:46, Devlin wrote:
> (Mostly just thinking out loud) I'm a little dubious of how useful these
checks
> are.  Given we can supply the preload check now, I'd probably prefer removing
> the virtualness of these functions and relying on the supplied check to do the
> right thing, and inferring whether or not the check was called through other
> ways (e.g., does a checker correctly pass or fail with running checks and
> certain assumptions).  WDYT?
> 
> Regardless, not something that needs to be done in this CL.

Makes sense; these primarily existed for mocking which we don't need anymore.
The test themselves should be enough to verify the checker behaves correctly.

https://codereview.chromium.org/2768723002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker_unittest.cc:92: class
ExtensionInstallCheckerAsync : public ExtensionInstallCheckerForTest {
On 2017/03/22 15:08:46, Devlin wrote:
> Given how small this class is, I'd just expose a set_async method on
> ExtensionInstallCheckerForTest.

Reasonable.

https://codereview.chromium.org/2768723002/diff/20001/extensions/browser/prel...
File extensions/browser/preload_check_test_util.cc (right):

https://codereview.chromium.org/2768723002/diff/20001/extensions/browser/prel...
extensions/browser/preload_check_test_util.cc:69:
base::Bind(&PreloadCheckStub::StartInternal,
On 2017/03/22 15:08:46, Devlin wrote:
> Another option would be to do
> PostTask(FROM_HERE, base::Bind(callback, errors_));
> 
> Two potential concerns:
> - Errors added after Start() don't get used.  That's probably fine (seems
wrong
> to do that).
> - Calling the callback is no longer bound to the lifetime of this object.
> 
> If you don't care about those, I'd say go with the simpler approach and nix
> StartInternal().  If you do, add a comment why.

Since I'm using a OnceCallback (new best practice), that doesn't compile. Added
comment.

https://codereview.chromium.org/2768723002/diff/20001/extensions/browser/prel...
extensions/browser/preload_check_test_util.cc:76: void
PreloadCheckStub::StartInternal(ResultCallback callback) {
On 2017/03/22 15:08:46, Devlin wrote:
> StartInternal is a funny name for something that deterministically ends the
flow
> :P  Maybe Finish()?

RunCallback sound good?

https://codereview.chromium.org/2768723002/diff/20001/extensions/browser/prel...
File extensions/browser/preload_check_test_util.h (right):

https://codereview.chromium.org/2768723002/diff/20001/extensions/browser/prel...
extensions/browser/preload_check_test_util.h:60: explicit PreloadCheckStub(bool
is_async = false);
On 2017/03/22 15:08:46, Devlin wrote:
> Maybe an enum, SYNCHRONOUS and ASYNCHRONOUS?  Makes the call sites much more
> readable.

I'll add set_is_async to mirror ExtensionInstallCheckerForTest. That may come in
handy soon....

https://codereview.chromium.org/2768723002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_checker_unittest.cc (right):

https://codereview.chromium.org/2768723002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker_unittest.cc:217: for (auto*
checker : {&sync_checker, &async_checker}) {
now these are inlined :-)

[Devlin, 2017-03-24 16:21:49]

LGTM % nits

https://codereview.chromium.org/2768723002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_checker.cc (right):

https://codereview.chromium.org/2768723002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:86: if
(errors.count(PreloadCheck::DISALLOWED_BY_POLICY))
would just `if (!errors.empty())` work?  It seems like in this case we shouldn't
need to specify a specific error so much as just know that it failed.

We could also just do
// GetErrorMessage() returns an empty string if there's no error.
policy_error_ = policy_check_->GetErrorMessage();

but that's perhaps a little less clear.

https://codereview.chromium.org/2768723002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_checker.h (right):

https://codereview.chromium.org/2768723002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.h:70: // error if the
blacklist state is BLACKLISTED_MALWARE or BLACKLISTED_UNKNOWN.
nit: Maybe s/blacklist state/BlacklistState to make it slightly more clear that
this is referring to a specific enum?

https://codereview.chromium.org/2768723002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_checker_unittest.cc (right):

https://codereview.chromium.org/2768723002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker_unittest.cc:106:
std::unique_ptr<PreloadCheckStub> blacklist_check =
nit: With base::MakeUnique<>(), auto is often cleaner and just as readable:
auto blacklist_check = base::MakeUnique<PreloadCheckStub>();

https://codereview.chromium.org/2768723002/diff/60001/extensions/browser/prel...
File extensions/browser/preload_check_test_util.h (right):

https://codereview.chromium.org/2768723002/diff/60001/extensions/browser/prel...
extensions/browser/preload_check_test_util.h:76: // Defaults to false.
optional: if you use c++11 fanciness and inline the defaults, this comment is
unnecessary :)

bool is_async_ = false;

[michaelpg, 2017-03-24 21:48:28]

thanks!

https://codereview.chromium.org/2768723002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_checker.cc (right):

https://codereview.chromium.org/2768723002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:86: if
(errors.count(PreloadCheck::DISALLOWED_BY_POLICY))
On 2017/03/24 16:21:48, Devlin wrote:
> would just `if (!errors.empty())` work?  It seems like in this case we
shouldn't
> need to specify a specific error so much as just know that it failed.

Went with this, with a DCHECK on the actual error.

> 
> We could also just do
> // GetErrorMessage() returns an empty string if there's no error.
> policy_error_ = policy_check_->GetErrorMessage();
> 
> but that's perhaps a little less clear.

https://codereview.chromium.org/2768723002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_checker.h (right):

https://codereview.chromium.org/2768723002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.h:70: // error if the
blacklist state is BLACKLISTED_MALWARE or BLACKLISTED_UNKNOWN.
On 2017/03/24 16:21:48, Devlin wrote:
> nit: Maybe s/blacklist state/BlacklistState to make it slightly more clear
that
> this is referring to a specific enum?

Done.

https://codereview.chromium.org/2768723002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_checker_unittest.cc (right):

https://codereview.chromium.org/2768723002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker_unittest.cc:106:
std::unique_ptr<PreloadCheckStub> blacklist_check =
On 2017/03/24 16:21:48, Devlin wrote:
> nit: With base::MakeUnique<>(), auto is often cleaner and just as readable:
> auto blacklist_check = base::MakeUnique<PreloadCheckStub>();

Done.

https://codereview.chromium.org/2768723002/diff/60001/extensions/browser/prel...
File extensions/browser/preload_check_test_util.h (right):

https://codereview.chromium.org/2768723002/diff/60001/extensions/browser/prel...
extensions/browser/preload_check_test_util.h:76: // Defaults to false.
On 2017/03/24 16:21:48, Devlin wrote:
> optional: if you use c++11 fanciness and inline the defaults, this comment is
> unnecessary :)
> 
> bool is_async_ = false;

Cool! Done in a couple other places.

[michaelpg, 2017-03-24 23:25:14]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2017-03-24 23:25:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2768723002/#ps100001
(title: "oops")

[commit-bot: I haz the power, 2017-03-24 23:25:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 23:51:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 23:51:14]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[michaelpg, 2017-03-25 01:21:25]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2017-03-25 01:21:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2768723002/#ps120001
(title: "remove redundant DCHECKs that fail in tests")

[commit-bot: I haz the power, 2017-03-25 01:21:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-25 02:02:55]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490404885100920,
"parent_rev": "827e3e9ad3709d6ff335ec9bafd1e536ea78b09f", "commit_rev":
"2eb513ed005d6b23489f457d997d944b6fa089d8"}

[commit-bot: I haz the power, 2017-03-25 02:03:57]

Description was changed from

==========
Update ExtensionInstallChecker to use PreloadCheck classes

Replaces some ad-hoc checks with their encapsulated equivalents.

Later, the individual OnFooCheckDone methods will be eliminated too.

BUG=679971
R=rdevlin.cronin@chromium.org
==========

to

==========
Update ExtensionInstallChecker to use PreloadCheck classes

Replaces some ad-hoc checks with their encapsulated equivalents.

Later, the individual OnFooCheckDone methods will be eliminated too.

BUG=679971
R=rdevlin.cronin@chromium.org

Review-Url: https://codereview.chromium.org/2768723002
Cr-Commit-Position: refs/heads/master@{#459621}
Committed:
https://chromium.googlesource.com/chromium/src/+/2eb513ed005d6b23489f457d997d...
==========

[commit-bot: I haz the power, 2017-03-25 02:03:58]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/2eb513ed005d6b23489f457d997d...