Retry reinstallation of corrupted policy extensions.

Retry reinstallation of corrupted policy extensions.

Introduce a new class PolicyExtensionReinstaller, that contains the
logic for retrying with a backoff.

Note that the backoff entries were per-extension before, I'm making
it global. Two reasons:
1) The per entry backoff was added primarily to stop
an extension from being continually reinstalling if the verification
for that extension kept failing. With the global backoff, it should
still hold. (crbug.com/661738)
2) Even if the entry is backoff, we call
ExtensionService::CheckForExternalUpdates() in the end, which applies
to *all* policy extensions, not just the one that corrupted.

BUG=703904
Test=Turn off network, corrupt a policy installed extension (easy to
modify a file/background script in extension's installation directory).
Turn the network back on, after a while extension should be reinstalled.

Review-Url: https://codereview.chromium.org/2790823004
Cr-Commit-Position: refs/heads/master@{#461748}
Committed: https://chromium.googlesource.com/chromium/src/+/77214d3c726115ef0205026fd78a70a521df0d6b

[lazyboy, 2017-03-31 19:01:41]

Description was changed from

==========
Retry reinstallation of corrupted policy extensions.

BUG=703904
Test=Turn off network, corrupt a policy installed extension (easy to
modify a file/background script in extension's installation directory).
Turn the network back on, after a while extension should be reinstalled.
==========

to

==========
Retry reinstallation of corrupted policy extensions.

Introduce a new class PolicyExtensionReinstaller, that contains the
logic for retrying with a backoff.

Note that the backoff entries were per-extension before, I'm making
it global. Two reasons:
1) The per entry backoff was added primarily to stop
an extension from being continually reinstalling if the verification
for that extension kept failing. With the global backoff, it should
still hold. (crbug.com/661738)
2) Even if the entry is backoff, we call
ExtensionService::CheckForExternalUpdates() in the end, which applies
to *all* policy extensions, not just the one that corrupted.

BUG=703904
Test=Turn off network, corrupt a policy installed extension (easy to
modify a file/background script in extension's installation directory).
Turn the network back on, after a while extension should be reinstalled.
==========

[lazyboy, 2017-03-31 19:03:35]

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

[lazyboy, 2017-03-31 19:03:51]

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

[commit-bot: I haz the power, 2017-03-31 19:04:42]

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-03-31 19:20:26]

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

[commit-bot: I haz the power, 2017-03-31 19:20:29]

Dry run: 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_...)

[lazyboy, 2017-03-31 20:49:56]

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

[commit-bot: I haz the power, 2017-03-31 20:50:25]

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-03-31 22:20:19]

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

[commit-bot: I haz the power, 2017-03-31 22:20:20]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-04-01 03:00:53]

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

https://codereview.chromium.org/2790823004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_verifier_delegate.cc:219:
policy_extension_reinstaller_.reset();
Document why this is important

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

https://codereview.chromium.org/2790823004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/content_verifier_browsertest.cc:802:
DCHECK(saved_callback_);
nit: ASSERTs?

https://codereview.chromium.org/2790823004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/content_verifier_browsertest.cc:850: // We will get
one more call than |iterations| because of how DelayTracker is
Can you elaborate on where this "one more" comes from?  Also, if it's exactly
one more, could we assert_eq(iterations, calls.size() - 1)?

https://codereview.chromium.org/2790823004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/content_verifier_browsertest.cc:861: // Tests that if
CheckForExternalUpdates() fails, then we retry reinstalling
Elaborate on when this can happen

https://codereview.chromium.org/2790823004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/content_verifier_browsertest.cc:880:
PolicyExtensionReinstaller::set_policy_reinstall_action_for_test(&action);
Doesn't DelayTracker's ctor do this?

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

https://codereview.chromium.org/2790823004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/policy_extension_reinstaller.cc:18:
base::Callback<void(const base::Closure& callback, base::TimeDelta delay)>*
This might warrant a typedef :)

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

https://codereview.chromium.org/2790823004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/policy_extension_reinstaller.h:11: #include
"base/gtest_prod_util.h"
needed?

https://codereview.chromium.org/2790823004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/policy_extension_reinstaller.h:45:
content::BrowserContext* context_;
nit: init to nullptr

[lazyboy, 2017-04-03 18:13:04]

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

https://codereview.chromium.org/2790823004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_verifier_delegate.cc:219:
policy_extension_reinstaller_.reset();
On 2017/04/01 03:00:53, Devlin wrote:
> Document why this is important

Done.

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

https://codereview.chromium.org/2790823004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/content_verifier_browsertest.cc:802:
DCHECK(saved_callback_);
On 2017/04/01 03:00:53, Devlin wrote:
> nit: ASSERTs?

Done.

https://codereview.chromium.org/2790823004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/content_verifier_browsertest.cc:839: // Setup to
intercept reinstall action, so we can see what the delay would
note: Incorrect copy-paste.

https://codereview.chromium.org/2790823004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/content_verifier_browsertest.cc:850: // We will get
one more call than |iterations| because of how DelayTracker is
On 2017/04/01 03:00:53, Devlin wrote:
> Can you elaborate on where this "one more" comes from?  Also, if it's exactly
> one more, could we assert_eq(iterations, calls.size() - 1)?

Added, also changed to iterations == calls.size()-1

https://codereview.chromium.org/2790823004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/content_verifier_browsertest.cc:861: // Tests that if
CheckForExternalUpdates() fails, then we retry reinstalling
On 2017/04/01 03:00:53, Devlin wrote:
> Elaborate on when this can happen

Done.

https://codereview.chromium.org/2790823004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/content_verifier_browsertest.cc:880:
PolicyExtensionReinstaller::set_policy_reinstall_action_for_test(&action);
On 2017/04/01 03:00:53, Devlin wrote:
> Doesn't DelayTracker's ctor do this?

Right, thanks.
Done.

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

https://codereview.chromium.org/2790823004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/policy_extension_reinstaller.cc:18:
base::Callback<void(const base::Closure& callback, base::TimeDelta delay)>*
On 2017/04/01 03:00:53, Devlin wrote:
> This might warrant a typedef :)

Done.

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

https://codereview.chromium.org/2790823004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/policy_extension_reinstaller.h:11: #include
"base/gtest_prod_util.h"
On 2017/04/01 03:00:53, Devlin wrote:
> needed?

Ah, was using FRIEND_*, not needed anymore, thanks

https://codereview.chromium.org/2790823004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/policy_extension_reinstaller.h:45:
content::BrowserContext* context_;
On 2017/04/01 03:00:53, Devlin wrote:
> nit: init to nullptr

Done.
Though this is always initialized in ctor initializer and hence this will always
be ignored.

[Devlin, 2017-04-03 21:12:42]

lgtm % the question in
chrome/browser/extensions/chrome_content_verifier_delegate.cc.

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

https://codereview.chromium.org/2790823004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/policy_extension_reinstaller.h:45:
content::BrowserContext* context_;
On 2017/04/03 18:13:04, lazyboy wrote:
> On 2017/04/01 03:00:53, Devlin wrote:
> > nit: init to nullptr
> 
> Done.
> Though this is always initialized in ctor initializer and hence this will
always
> be ignored.

Ack, but it's really easy to change a ctor and not realize that you suddenly
uninitialized a variable, so I prefer all POD members to be set, if any are
(with the possible exception if everything is defined inline where it's more
clear).

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

https://codereview.chromium.org/2790823004/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_verifier_delegate.cc:27: #include
"content/public/browser/browser_thread.h"
needed?

https://codereview.chromium.org/2790823004/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_verifier_delegate.cc:219: // Shut down
|policy_extension_reinstaller_| on its creation thread.
When would it not be?

[lazyboy, 2017-04-03 22:23:15]

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

https://codereview.chromium.org/2790823004/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_verifier_delegate.cc:27: #include
"content/public/browser/browser_thread.h"
On 2017/04/03 21:12:42, Devlin wrote:
> needed?

No, removed. Thanks.

https://codereview.chromium.org/2790823004/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_verifier_delegate.cc:219: // Shut down
|policy_extension_reinstaller_| on its creation thread.
On 2017/04/03 21:12:42, Devlin wrote:
> When would it not be?

CVDelegate (owner of |policy_extension_reinstaller_|) is created on UI
ExtensionSystemImpl::Shared::Init, but it has ref in InfoMap. When InfoMap
destroys on IO, it might delete CVDelegate there on IO that will destroy
|policy_extension_reinstaller_| on IO.
I do not see InfoMap::~InfoMap or CVDelegate::~CVDelegate in a browser run +
shutdown sequence though. But that always happens in browser_tests (e.g. when I
run ContentVerifierPolicyTest.*).

[lazyboy, 2017-04-03 22:24:28]

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

[commit-bot: I haz the power, 2017-04-03 22:25:14]

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-04 00:59:56]

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

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

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

[lazyboy, 2017-04-04 16:25:59]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2017-04-04 16:26:02]

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/2790823004/#ps80001
(title: "address comments")

[commit-bot: I haz the power, 2017-04-04 16:26:32]

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-04 16:46:16]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491323159747720,
"parent_rev": "c553fcf813fdfeaa5e94e01daa2cad2c7b71a6b3", "commit_rev":
"77214d3c726115ef0205026fd78a70a521df0d6b"}

[commit-bot: I haz the power, 2017-04-04 16:47:08]

Description was changed from

==========
Retry reinstallation of corrupted policy extensions.

Introduce a new class PolicyExtensionReinstaller, that contains the
logic for retrying with a backoff.

Note that the backoff entries were per-extension before, I'm making
it global. Two reasons:
1) The per entry backoff was added primarily to stop
an extension from being continually reinstalling if the verification
for that extension kept failing. With the global backoff, it should
still hold. (crbug.com/661738)
2) Even if the entry is backoff, we call
ExtensionService::CheckForExternalUpdates() in the end, which applies
to *all* policy extensions, not just the one that corrupted.

BUG=703904
Test=Turn off network, corrupt a policy installed extension (easy to
modify a file/background script in extension's installation directory).
Turn the network back on, after a while extension should be reinstalled.
==========

to

==========
Retry reinstallation of corrupted policy extensions.

Introduce a new class PolicyExtensionReinstaller, that contains the
logic for retrying with a backoff.

Note that the backoff entries were per-extension before, I'm making
it global. Two reasons:
1) The per entry backoff was added primarily to stop
an extension from being continually reinstalling if the verification
for that extension kept failing. With the global backoff, it should
still hold. (crbug.com/661738)
2) Even if the entry is backoff, we call
ExtensionService::CheckForExternalUpdates() in the end, which applies
to *all* policy extensions, not just the one that corrupted.

BUG=703904
Test=Turn off network, corrupt a policy installed extension (easy to
modify a file/background script in extension's installation directory).
Turn the network back on, after a while extension should be reinstalled.

Review-Url: https://codereview.chromium.org/2790823004
Cr-Commit-Position: refs/heads/master@{#461748}
Committed:
https://chromium.googlesource.com/chromium/src/+/77214d3c726115ef0205026fd78a...
==========

[commit-bot: I haz the power, 2017-04-04 16:47:12]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/77214d3c726115ef0205026fd78a...