New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 703904 link

Starred by 1 user
Status: Fixed
Owner:
lazyboy@chromium.org
Closed: Apr 2017
Cc:
devlin@chromium.org
█ asargent@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 699420



Sign in to add a comment

Corrupted policy extension reinstallation isn't retried

Project Member Reported by lazyboy@chromium.org, Mar 22 2017 
Two issues:

1) If an extension's content verification fails [1] because of corruption, then we reinstall the extension by calling CheckForExternalUpdates() [2]. However if that particular call to CheckForExternalUpdates failed (e.g. network unavailable) we wouldn't proactively attempt to reinstall the extension again. This is because this step would also disable the extension, I don't think we will ever try to repair it automatically: none of the disabled extension's resources will be requested that could result in further ChromeContentVerifierDelegate::VerifyFailed().

2) Even if ChromeContentVerifierDelegate::VerifyFailed() triggered somehow for that extension (an example could be that user explicitly trying to visit a resource in that extension), the early out [3]:
      if (pending_manager->IsPolicyReinstallForCorruptionExpected(extension_id))
        return;
will skip the reinstallation.


PendingExtensionManager::expected_policy_reinstalls_ holds these extension ids, we could think of reinstalling these with a timer + backoff. (The individual backoff timer policy_reinstall_backoff_ wouldn't apply anymore).

[1] https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_content_verifier_delegate.cc?rcl=99854454417e7c9ad57d2d2d441ef17cc481ba2d&l=197
[2] https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_content_verifier_delegate.cc?rcl=99854454417e7c9ad57d2d2d441ef17cc481ba2d&l=237
[3] https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_content_verifier_delegate.cc?rcl=c8cc93e27f2d38c39e8af5f79b8e69685348263e&l=201
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 4 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/77214d3c726115ef0205026fd78a70a521df0d6b

commit 77214d3c726115ef0205026fd78a70a521df0d6b
Author: lazyboy <lazyboy@chromium.org>
Date: Tue Apr 04 16:46:12 2017

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}

[modify] https://crrev.com/77214d3c726115ef0205026fd78a70a521df0d6b/chrome/browser/extensions/BUILD.gn
[modify] https://crrev.com/77214d3c726115ef0205026fd78a70a521df0d6b/chrome/browser/extensions/chrome_content_verifier_delegate.cc
[modify] https://crrev.com/77214d3c726115ef0205026fd78a70a521df0d6b/chrome/browser/extensions/chrome_content_verifier_delegate.h
[modify] https://crrev.com/77214d3c726115ef0205026fd78a70a521df0d6b/chrome/browser/extensions/content_verifier_browsertest.cc
[modify] https://crrev.com/77214d3c726115ef0205026fd78a70a521df0d6b/chrome/browser/extensions/extension_service.cc
[modify] https://crrev.com/77214d3c726115ef0205026fd78a70a521df0d6b/chrome/browser/extensions/extension_service.h
[modify] https://crrev.com/77214d3c726115ef0205026fd78a70a521df0d6b/chrome/browser/extensions/pending_extension_manager.cc
[modify] https://crrev.com/77214d3c726115ef0205026fd78a70a521df0d6b/chrome/browser/extensions/pending_extension_manager.h
[add] https://crrev.com/77214d3c726115ef0205026fd78a70a521df0d6b/chrome/browser/extensions/policy_extension_reinstaller.cc
[add] https://crrev.com/77214d3c726115ef0205026fd78a70a521df0d6b/chrome/browser/extensions/policy_extension_reinstaller.h
[add] https://crrev.com/77214d3c726115ef0205026fd78a70a521df0d6b/chrome/browser/extensions/policy_extension_reinstaller_unittest.cc
[modify] https://crrev.com/77214d3c726115ef0205026fd78a70a521df0d6b/chrome/test/BUILD.gn
[modify] https://crrev.com/77214d3c726115ef0205026fd78a70a521df0d6b/extensions/browser/content_hash_fetcher_unittest.cc
[modify] https://crrev.com/77214d3c726115ef0205026fd78a70a521df0d6b/extensions/browser/content_verifier.cc
[modify] https://crrev.com/77214d3c726115ef0205026fd78a70a521df0d6b/extensions/browser/content_verifier_delegate.h

Comment 2 by lazyboy@chromium.org, Apr 4 2017

Status: Fixed (was: Assigned)
►

Sign in to add a comment