developerPrivate.repair: Skip not-corrupted and policy extensions

developerPrivate.repair: Skip not-corrupted and policy extensions.

Currently if we call repair() on a policy extension that is not
corrupted, it will:
1. First uninstall the extension
2. Then attempt to reinstall the extension. However this will fail b/c
  our code thinks the user is trying to manually install a policy forced
  extension (ExtensionInstallChecker::CheckManagementPolicy() -> 
  PolicyCheck::Start)
Simpler fix is to just disallow policy extensions.

Then, it we shouldn't attempt to repair any extension that is not corrupted

BUG=577959
Test=See bug description for repro.

Review-Url: https://codereview.chromium.org/2801583003
Cr-Commit-Position: refs/heads/master@{#463085}
Committed: https://chromium.googlesource.com/chromium/src/+/a00eafcea5becf94333233003d24f6881f5efcdb

[lazyboy, 2017-04-06 00:47:16]

Description was changed from

==========
developerPrivate.repair: Skip policy extensions that are not corrupted.

Currently if we call repair() on a policy extension that is not
corrupted, it will:
1. First uninstall the extension
2. Then attempt to reinstall the extension. However this will fail b/c
  our code thinks the user is trying to manually install a policy forced
  extension.

Simpler fix is to just disallow this case.

BUG=577959
Test=See bug description for repro.
==========

to

==========
developerPrivate.repair: Skip policy extensions that are not corrupted.

Currently if we call repair() on a policy extension that is not
corrupted, it will:
1. First uninstall the extension
2. Then attempt to reinstall the extension. However this will fail b/c
  our code thinks the user is trying to manually install a policy forced
  extension (ExtensionInstallChecker::CheckManagementPolicy() -> 
  PolicyCheck::Start)

Simpler fix is to just disallow this case.

BUG=577959
Test=See bug description for repro.
==========

[lazyboy, 2017-04-06 00:49:16]

lazyboy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[lazyboy, 2017-04-06 00:49:21]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 00:49:57]

Dry run: 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-04-06 00:53:22]

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

[commit-bot: I haz the power, 2017-04-06 00:53:23]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Devlin, 2017-04-06 00:54:06]

https://codereview.chromium.org/2801583003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/2801583003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1468:
if (extension_system->management_policy()->MustRemainEnabled(extension,
Should we also check MustRemainInstalled()?

https://codereview.chromium.org/2801583003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1473:
// reinstalled/repaired. Currently this would uninstall the extension but
Should we even allow this in the case of corruption?  If it's going to fail on
reinstall, it sounds like something we shouldn't do - especially now that we
should be repairing corrupt policy extensions.

[Devlin, 2017-04-06 00:55:24]

https://codereview.chromium.org/2801583003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/2801583003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1475:
if ((disable_reason & Extension::DISABLE_CORRUPTED) == 0)
And actually, we should probably have this check either here, in
WebstoreReinstaller, or both, for all extensions (regardless of policy or not)

[lazyboy, 2017-04-06 01:33:53]

https://codereview.chromium.org/2801583003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/2801583003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1468:
if (extension_system->management_policy()->MustRemainEnabled(extension,
On 2017/04/06 00:54:06, Devlin wrote:
> Should we also check MustRemainInstalled()?

I'm not entirely sure, I copied the logic from verifier:
https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_content...

Note that there's also !UserMayModifySettings checks somewhere.

https://codereview.chromium.org/2801583003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1473:
// reinstalled/repaired. Currently this would uninstall the extension but
On 2017/04/06 00:54:06, Devlin wrote:
> Should we even allow this in the case of corruption?  If it's going to fail on
> reinstall, it sounds like something we shouldn't do - especially now that we
> should be repairing corrupt policy extensions.

"Currently" is misleading here, I meant to put that in CL description to
describe what happens without the CL. With the CL, we do not uninstall the
extension at all. Updated the comment.

https://codereview.chromium.org/2801583003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1475:
if ((disable_reason & Extension::DISABLE_CORRUPTED) == 0)
On 2017/04/06 00:55:24, Devlin wrote:
> And actually, we should probably have this check either here, in
> WebstoreReinstaller, or both, for all extensions (regardless of policy or not)

Putting it in WebstoreReinstaller might other cases where we try to reinstall
from somewhere else than here?

Oh, this function is the only consumer of WebstoreReinstaller, so added the
check there.

[lazyboy, 2017-04-06 01:39:54]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 01:40:19]

Dry run: 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-04-06 02:12:23]

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

[commit-bot: I haz the power, 2017-04-06 02:12:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2017-04-06 15:12:32]

https://codereview.chromium.org/2801583003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/2801583003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1468:
if (extension_system->management_policy()->MustRemainEnabled(extension,
On 2017/04/06 01:33:53, lazyboy wrote:
> On 2017/04/06 00:54:06, Devlin wrote:
> > Should we also check MustRemainInstalled()?
> 
> I'm not entirely sure, I copied the logic from verifier:
>
https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_content...
> 
> Note that there's also !UserMayModifySettings checks somewhere.

Yeah, the differences differences between all of these possible policy settings
are pretty subtle, and sometimes unclear.  Since this inherently uninstalls the
extension (and only performs a one-off attempt), let's also check
MustRemainInstalled(), but we can probably avoid UserMayModifySettings (at least
for now).

https://codereview.chromium.org/2801583003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1473:
// reinstalled/repaired. Currently this would uninstall the extension but
On 2017/04/06 01:33:53, lazyboy wrote:
> On 2017/04/06 00:54:06, Devlin wrote:
> > Should we even allow this in the case of corruption?  If it's going to fail
on
> > reinstall, it sounds like something we shouldn't do - especially now that we
> > should be repairing corrupt policy extensions.
> 
> "Currently" is misleading here, I meant to put that in CL description to
> describe what happens without the CL. With the CL, we do not uninstall the
> extension at all. Updated the comment.

Yep, I figured. :)  But my main question still stands - it sounds like, even in
the case of corruption, the reinstall here will fail due to a policy check. 
This accounts for the case of a call to repair an uncorrupted extension, but
even with a corrupted extension, perhaps we shouldn't do this?  If I have a
corrupted policy extension and do this, won't it just uninstall it and fail?  Or
am I misunderstanding the policy check?

https://codereview.chromium.org/2801583003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/2801583003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1476:
if ((disable_reason & Extension::DISABLE_CORRUPTED) == 0)
To elaborate on a previous comment, I don't think we should be allowing repair()
for uncorrupted extensions at all.

What I'd see this looking like is:
if (!ExtensionPrefs::Get()->HasDisableReason(id, DISABLE_CORRUPTED)) {
  return RespondNow(Error(kCannot...));
}

// Depending on the answer to my other comment
if (management_policy()->MustRemainEnabled/Installed())
  return RespondNow(Error(kCannotRepairPolicyExtension));

There's no reason we should be trying to repair an uncorrupted extension, so we
may as well disallow it in all cases (even if it's not as much of a concern).

https://codereview.chromium.org/2801583003/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
(right):

https://codereview.chromium.org/2801583003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:86:
// TODO(lazyboy): ) Extract to a separate file.
Any chance we can do that in this CL?  How big of a churn is it?  Though
depending on if the test below changes, this may be unneeded.

https://codereview.chromium.org/2801583003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:933:
// Tests that developerPrivate.repair does not succeed for a non-corrupted
I think we can probably just make this a test for "developerPrivate.repair does
not succeed for a non-corrupted extension", and ignore policy vs not.

[lazyboy, 2017-04-06 17:55:38]

Description was changed from

==========
developerPrivate.repair: Skip policy extensions that are not corrupted.

Currently if we call repair() on a policy extension that is not
corrupted, it will:
1. First uninstall the extension
2. Then attempt to reinstall the extension. However this will fail b/c
  our code thinks the user is trying to manually install a policy forced
  extension (ExtensionInstallChecker::CheckManagementPolicy() -> 
  PolicyCheck::Start)

Simpler fix is to just disallow this case.

BUG=577959
Test=See bug description for repro.
==========

to

==========
developerPrivate.repair: Skip not-corrupted and policy extensions.

Currently if we call repair() on a policy extension that is not
corrupted, it will:
1. First uninstall the extension
2. Then attempt to reinstall the extension. However this will fail b/c
  our code thinks the user is trying to manually install a policy forced
  extension (ExtensionInstallChecker::CheckManagementPolicy() -> 
  PolicyCheck::Start)
Simpler fix is to just disallow policy extensions.

Then, it we shouldn't attempt to repair any extension that is not corrupted

BUG=577959
Test=See bug description for repro.
==========

[lazyboy, 2017-04-06 18:06:05]

https://codereview.chromium.org/2801583003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/2801583003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1468:
if (extension_system->management_policy()->MustRemainEnabled(extension,
On 2017/04/06 15:12:31, Devlin wrote:
> On 2017/04/06 01:33:53, lazyboy wrote:
> > On 2017/04/06 00:54:06, Devlin wrote:
> > > Should we also check MustRemainInstalled()?
> > 
> > I'm not entirely sure, I copied the logic from verifier:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_content...
> > 
> > Note that there's also !UserMayModifySettings checks somewhere.
> 
> Yeah, the differences differences between all of these possible policy
settings
> are pretty subtle, and sometimes unclear.  Since this inherently uninstalls
the
> extension (and only performs a one-off attempt), let's also check
> MustRemainInstalled(), but we can probably avoid UserMayModifySettings (at
least
> for now). 

OK, done MustRemainEnabled || MustRemainInstalled
As this should match our content verifier code in ChromeContentVerifierDelegate,
I've extracted this to a separate function
ContentVerifier::ShouldRepairIfCorrupted and used it in CCVDelegate.
I wanted the name to sound more tied to content verification than policy, but
not sure. WDYT?

https://codereview.chromium.org/2801583003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1473:
// reinstalled/repaired. Currently this would uninstall the extension but
On 2017/04/06 15:12:31, Devlin wrote:
> On 2017/04/06 01:33:53, lazyboy wrote:
> > On 2017/04/06 00:54:06, Devlin wrote:
> > > Should we even allow this in the case of corruption?  If it's going to
fail
> on
> > > reinstall, it sounds like something we shouldn't do - especially now that
we
> > > should be repairing corrupt policy extensions.
> > 
> > "Currently" is misleading here, I meant to put that in CL description to
> > describe what happens without the CL. With the CL, we do not uninstall the
> > extension at all. Updated the comment.
> 
> Yep, I figured. :)  But my main question still stands - it sounds like, even
in
> the case of corruption, the reinstall here will fail due to a policy check. 
> This accounts for the case of a call to repair an uncorrupted extension, but
> even with a corrupted extension, perhaps we shouldn't do this?  If I have a
> corrupted policy extension and do this, won't it just uninstall it and fail? 
Ah, that is correct. I've disallowed policy extension altogether.
 Or
> am I misunderstanding the policy check?

https://codereview.chromium.org/2801583003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/2801583003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1476:
if ((disable_reason & Extension::DISABLE_CORRUPTED) == 0)
On 2017/04/06 15:12:31, Devlin wrote:
> To elaborate on a previous comment, I don't think we should be allowing
repair()
> for uncorrupted extensions at all.
> 
> What I'd see this looking like is:
> if (!ExtensionPrefs::Get()->HasDisableReason(id, DISABLE_CORRUPTED)) {
>   return RespondNow(Error(kCannot...));
> }
> 
> // Depending on the answer to my other comment
> if (management_policy()->MustRemainEnabled/Installed())
>   return RespondNow(Error(kCannotRepairPolicyExtension));
> 
> There's no reason we should be trying to repair an uncorrupted extension, so
we
> may as well disallow it in all cases (even if it's not as much of a concern).

Done.

https://codereview.chromium.org/2801583003/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
(right):

https://codereview.chromium.org/2801583003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:86:
// TODO(lazyboy): ) Extract to a separate file.
On 2017/04/06 15:12:32, Devlin wrote:
> Any chance we can do that in this CL?  How big of a churn is it?  Though
> depending on if the test below changes, this may be unneeded.

Shouldn't be big, I'm holding on to it until we settle on the CL (as you mention
how it might change if the test below changes...), will do in the last patch.

https://codereview.chromium.org/2801583003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:933:
// Tests that developerPrivate.repair does not succeed for a non-corrupted
On 2017/04/06 15:12:32, Devlin wrote:
> I think we can probably just make this a test for "developerPrivate.repair
does
> not succeed for a non-corrupted extension", and ignore policy vs not.

Current patch set #5 has test for a non-corrupted case and a policy extension
case.

[Devlin, 2017-04-06 21:01:33]

Nice!  Just a few last nits, and ideally moving the duplicate code to a shared
space. :)

https://codereview.chromium.org/2801583003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/2801583003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:115:
"Cannot repair a not corrupted extension.";
"not corrupted" sounds a bit funny to me.  "valid"?  "healthy"?

https://codereview.chromium.org/2801583003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:116:
const char kCannotRepairPolicyExtension[] = "Cannot repair a policy extension.";
nit: s/policy/policy-installed

https://codereview.chromium.org/2801583003/diff/80001/chrome/browser/extensio...
File
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
(right):

https://codereview.chromium.org/2801583003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:969:
EXPECT_EQ("Cannot repair a policy extension.", function->GetError());
I'd slightly prefer two separate tests - one for uncorrupted, and one for policy
- so that we know we don't allow repair of uncorrupted extensions independent of
policy status.

https://codereview.chromium.org/2801583003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_reinstaller.cc (right):

https://codereview.chromium.org/2801583003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/webstore_reinstaller.cc:31:
->GetDisableReasons(extension_id) &
nit: Prefer HasDisableReason

https://codereview.chromium.org/2801583003/diff/80001/extensions/browser/cont...
File extensions/browser/content_verifier.h (right):

https://codereview.chromium.org/2801583003/diff/80001/extensions/browser/cont...
extensions/browser/content_verifier.h:50: const Extension* id);
\n

[lazyboy, 2017-04-06 22:32:56]

https://codereview.chromium.org/2801583003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/2801583003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:115:
"Cannot repair a not corrupted extension.";
On 2017/04/06 21:01:32, Devlin wrote:
> "not corrupted" sounds a bit funny to me.  "valid"?  "healthy"?

Done.

https://codereview.chromium.org/2801583003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:116:
const char kCannotRepairPolicyExtension[] = "Cannot repair a policy extension.";
On 2017/04/06 21:01:32, Devlin wrote:
> nit: s/policy/policy-installed

Done.

https://codereview.chromium.org/2801583003/diff/80001/chrome/browser/extensio...
File
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
(right):

https://codereview.chromium.org/2801583003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:969:
EXPECT_EQ("Cannot repair a policy extension.", function->GetError());
On 2017/04/06 21:01:32, Devlin wrote:
> I'd slightly prefer two separate tests - one for uncorrupted, and one for
policy
> - so that we know we don't allow repair of uncorrupted extensions independent
of
> policy status.

Done.

https://codereview.chromium.org/2801583003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_reinstaller.cc (right):

https://codereview.chromium.org/2801583003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/webstore_reinstaller.cc:31:
->GetDisableReasons(extension_id) &
On 2017/04/06 21:01:32, Devlin wrote:
> nit: Prefer HasDisableReason

Done.

https://codereview.chromium.org/2801583003/diff/80001/extensions/browser/cont...
File extensions/browser/content_verifier.h (right):

https://codereview.chromium.org/2801583003/diff/80001/extensions/browser/cont...
extensions/browser/content_verifier.h:50: const Extension* id);
On 2017/04/06 21:01:32, Devlin wrote:
> \n

Done.

https://codereview.chromium.org/2801583003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/2801583003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:160:
prefs->HasDisableReason(extension->id(),
Note: unrelated to this CL

https://codereview.chromium.org/2801583003/diff/100001/extensions/browser/moc...
File extensions/browser/mock_external_provider.h (right):

https://codereview.chromium.org/2801583003/diff/100001/extensions/browser/moc...
extensions/browser/mock_external_provider.h:1: // Copyright 2017 The Chromium
Authors. All rights reserved.
Moved test mock to this new file.

[Devlin, 2017-04-07 16:10:05]

lgtm % nits, assuming the external provider stuff is mostly just copy paste.

https://codereview.chromium.org/2801583003/diff/100001/chrome/browser/extensi...
File
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
(right):

https://codereview.chromium.org/2801583003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:62:
const char good_crx[] = "ldnnhddmnhbkjipkidpdiheffobcpfmf";
nit: I realize this is what it's called in extension_service_unittest.cc, but it
should be kGoodCrxId.

https://codereview.chromium.org/2801583003/diff/100001/extensions/browser/moc...
File extensions/browser/mock_external_provider.cc (right):

https://codereview.chromium.org/2801583003/diff/100001/extensions/browser/moc...
extensions/browser/mock_external_provider.cc:14:
MockExternalProvider::MockExternalProvider(VisitorInterface* visitor,
copy paste?

https://codereview.chromium.org/2801583003/diff/100001/extensions/browser/moc...
File extensions/browser/mock_external_provider.h (right):

https://codereview.chromium.org/2801583003/diff/100001/extensions/browser/moc...
extensions/browser/mock_external_provider.h:7: 
#include <map>
#include <memory>
#include <string>
#include <utility>

#include "base/macros.h"

https://codereview.chromium.org/2801583003/diff/100001/extensions/browser/moc...
extensions/browser/mock_external_provider.h:15: class MockExternalProvider :
public extensions::ExternalProviderInterface {
nit: no extensions:: prefix.

https://codereview.chromium.org/2801583003/diff/100001/extensions/browser/moc...
extensions/browser/mock_external_provider.h:32: std::unique_ptr<base::Version>*
version) const override;
forward declare version

https://codereview.chromium.org/2801583003/diff/100001/extensions/browser/moc...
extensions/browser/mock_external_provider.h:51: private:
delete this line (private scope started on line 39)

[lazyboy, 2017-04-07 17:30:27]

https://codereview.chromium.org/2801583003/diff/100001/chrome/browser/extensi...
File
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
(right):

https://codereview.chromium.org/2801583003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:62:
const char good_crx[] = "ldnnhddmnhbkjipkidpdiheffobcpfmf";
On 2017/04/07 16:10:04, Devlin wrote:
> nit: I realize this is what it's called in extension_service_unittest.cc, but
it
> should be kGoodCrxId.

Done.

https://codereview.chromium.org/2801583003/diff/100001/extensions/browser/moc...
File extensions/browser/mock_external_provider.cc (right):

https://codereview.chromium.org/2801583003/diff/100001/extensions/browser/moc...
extensions/browser/mock_external_provider.cc:14:
MockExternalProvider::MockExternalProvider(VisitorInterface* visitor,
On 2017/04/07 16:10:04, Devlin wrote:
> copy paste?

Yes, I've renamed MockExtensionProvider to MockExternalProvider as this class
implements ExternalProvider.

https://codereview.chromium.org/2801583003/diff/100001/extensions/browser/moc...
File extensions/browser/mock_external_provider.h (right):

https://codereview.chromium.org/2801583003/diff/100001/extensions/browser/moc...
extensions/browser/mock_external_provider.h:7: 
On 2017/04/07 16:10:04, Devlin wrote:
> #include <map>
> #include <memory>
> #include <string>
> #include <utility>
> 
> #include "base/macros.h"

Done.

https://codereview.chromium.org/2801583003/diff/100001/extensions/browser/moc...
extensions/browser/mock_external_provider.h:15: class MockExternalProvider :
public extensions::ExternalProviderInterface {
On 2017/04/07 16:10:04, Devlin wrote:
> nit: no extensions:: prefix.

Done.

https://codereview.chromium.org/2801583003/diff/100001/extensions/browser/moc...
extensions/browser/mock_external_provider.h:32: std::unique_ptr<base::Version>*
version) const override;
On 2017/04/07 16:10:04, Devlin wrote:
> forward declare version

Done.

https://codereview.chromium.org/2801583003/diff/100001/extensions/browser/moc...
extensions/browser/mock_external_provider.h:51: private:
On 2017/04/07 16:10:04, Devlin wrote:
> delete this line (private scope started on line 39)

Done.

[lazyboy, 2017-04-07 17:32:43]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 17:33:16]

Dry run: 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-04-07 18:27:06]

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

[commit-bot: I haz the power, 2017-04-07 18:27:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lazyboy, 2017-04-07 20:24:19]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 20:25:00]

Dry run: 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-04-07 21:34:17]

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

[commit-bot: I haz the power, 2017-04-07 21:34:17]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-04-08 00:43:46]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2017-04-08 00:43:46]

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/2801583003/#ps110011
(title: "fix WebstoreReinsallerBrowserTest")

[commit-bot: I haz the power, 2017-04-08 00:44:34]

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-04-08 00:57:26]

CQ is committing da patch.

Bot data: {"patchset_id": 110011, "attempt_start_ts": 1491612226152580,
"parent_rev": "10784779e17f9d143d5f57c7ff902ee7efc0cc81", "commit_rev":
"a00eafcea5becf94333233003d24f6881f5efcdb"}

[commit-bot: I haz the power, 2017-04-08 00:58:21]

Description was changed from

==========
developerPrivate.repair: Skip not-corrupted and policy extensions.

Currently if we call repair() on a policy extension that is not
corrupted, it will:
1. First uninstall the extension
2. Then attempt to reinstall the extension. However this will fail b/c
  our code thinks the user is trying to manually install a policy forced
  extension (ExtensionInstallChecker::CheckManagementPolicy() -> 
  PolicyCheck::Start)
Simpler fix is to just disallow policy extensions.

Then, it we shouldn't attempt to repair any extension that is not corrupted

BUG=577959
Test=See bug description for repro.
==========

to

==========
developerPrivate.repair: Skip not-corrupted and policy extensions.

Currently if we call repair() on a policy extension that is not
corrupted, it will:
1. First uninstall the extension
2. Then attempt to reinstall the extension. However this will fail b/c
  our code thinks the user is trying to manually install a policy forced
  extension (ExtensionInstallChecker::CheckManagementPolicy() -> 
  PolicyCheck::Start)
Simpler fix is to just disallow policy extensions.

Then, it we shouldn't attempt to repair any extension that is not corrupted

BUG=577959
Test=See bug description for repro.

Review-Url: https://codereview.chromium.org/2801583003
Cr-Commit-Position: refs/heads/master@{#463085}
Committed:
https://chromium.googlesource.com/chromium/src/+/a00eafcea5becf94333233003d24...
==========

[commit-bot: I haz the power, 2017-04-08 00:58:22]

Committed patchset #8 (id:110011) as
https://chromium.googlesource.com/chromium/src/+/a00eafcea5becf94333233003d24...