Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2533873003:
Add throttling to corrupt policy extensions reinstall (Closed)
|
Created:
4 years ago by asargent_no_longer_on_chrome Modified:
4 years ago Reviewers:
Devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd throttling to corrupt policy extensions reinstall
For crbug.com/447040 (https://codereview.chromium.org/2299203004) we
made it so that enterprise policy force installed extensions would get
automatically reinstalled when we detected corruption. However, we
didn't add in any throttling, which was a bad idea because we turned out
to have a bug (crbug.com/643814) where particular extensions were always
incorrectly detected as corrupt right after install. So at least one
enterprise with one of these extensions in their force install list
ended up having chrome spin in an loop redownloading/reinstalling one of
the affected extensions, eating up tons of bandwidth and disk space.
This CL adds in throttling with an exponentially backed off delay on
each reinstall attempt, up to a maximum of 30 minutes. The delay value
is only maintained in memory during the course of a single run of chrome
and resets after 6 hours (or on browser restart), but should be
sufficient to drastically reduce the problem reported for this bug.
BUG=661738
Committed: https://crrev.com/6014bdfa1f211946627a7085c87fe922e1581fc5
Cr-Commit-Position: refs/heads/master@{#435430}
Patch Set 1 : ready for review #
Total comments: 10
Patch Set 2 : review feedback fixes #Patch Set 3 : merge latest origin/master #
Messages
Total messages: 29 (19 generated)
The CQ bit was checked by asargent@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
The CQ bit was checked by asargent@chromium.org to run a CQ dry run
asargent@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2533873003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/chrome_content_verifier_delegate.cc (right): https://codereview.chromium.org/2533873003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/chrome_content_verifier_delegate.cc:71: base::LazyInstance<base::Callback<void(base::TimeDelta delay)>> Any need for this to a be a lazy instance rather than just a raw ptr (and then make sure it doesn't go out of scope in the test)? https://codereview.chromium.org/2533873003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/chrome_content_verifier_delegate.cc:227: backoff_entry = policy_reinstall_backoff_[extension_id].get(); nit: you could avoid a double look-up with either: auto new_backoff_entry = base::MakeUnique<>(...); backoff_entry = new_backoff_entry.get(); policy_reinstall_backoff_[extension_id] = std::move(new_backoff_entry) or backoff_entry = policy_reinstall_backoff_.insert(std::make_pair( extension_id, base::MakeUnique<>(...))).first->second; https://codereview.chromium.org/2533873003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/content_verifier_browsertest.cc (right): https://codereview.chromium.org/2533873003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/content_verifier_browsertest.cc:759: class DelayTracker : public base::SupportsWeakPtr<DelayTracker> { probably safe to just use unretained here. https://codereview.chromium.org/2533873003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/content_verifier_browsertest.cc:779: // ExtensionPrefs* prefs = ExtensionPrefs::Get(profile()); delete? https://codereview.chromium.org/2533873003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/content_verifier_browsertest.cc:810: EXPECT_EQ(base::TimeDelta(), delay_tracker.calls()[0]); we should either ASSERT_EQ the size above or should add an ASSERT_FALSE(calls.empty()) to ensure we don't crash here.
https://codereview.chromium.org/2533873003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/chrome_content_verifier_delegate.cc (right): https://codereview.chromium.org/2533873003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/chrome_content_verifier_delegate.cc:71: base::LazyInstance<base::Callback<void(base::TimeDelta delay)>> On 2016/11/29 22:31:24, Devlin wrote: > Any need for this to a be a lazy instance rather than just a raw ptr (and then > make sure it doesn't go out of scope in the test)? I think I did it this way out of habit because we normally pass Callback's by value, but a raw pointer works too, so I've switched to that since it's a little simpler. https://codereview.chromium.org/2533873003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/chrome_content_verifier_delegate.cc:227: backoff_entry = policy_reinstall_backoff_[extension_id].get(); On 2016/11/29 22:31:24, Devlin wrote: > nit: you could avoid a double look-up with either: > auto new_backoff_entry = base::MakeUnique<>(...); > backoff_entry = new_backoff_entry.get(); > policy_reinstall_backoff_[extension_id] = std::move(new_backoff_entry) > > or > > backoff_entry = > policy_reinstall_backoff_.insert(std::make_pair( > extension_id, base::MakeUnique<>(...))).first->second; Done. https://codereview.chromium.org/2533873003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/content_verifier_browsertest.cc (right): https://codereview.chromium.org/2533873003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/content_verifier_browsertest.cc:759: class DelayTracker : public base::SupportsWeakPtr<DelayTracker> { On 2016/11/29 22:31:24, Devlin wrote: > probably safe to just use unretained here. Done. https://codereview.chromium.org/2533873003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/content_verifier_browsertest.cc:779: // ExtensionPrefs* prefs = ExtensionPrefs::Get(profile()); On 2016/11/29 22:31:24, Devlin wrote: > delete? Good catch, done. https://codereview.chromium.org/2533873003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/content_verifier_browsertest.cc:810: EXPECT_EQ(base::TimeDelta(), delay_tracker.calls()[0]); On 2016/11/29 22:31:25, Devlin wrote: > we should either ASSERT_EQ the size above or should add an > ASSERT_FALSE(calls.empty()) to ensure we don't crash here. Done.
The CQ bit was checked by asargent@chromium.org
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/2533873003/#ps40001 (title: "review feedback fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by asargent@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
chrome/browser/extensions/content_verifier_browsertest.cc:
While running git apply --index -p1;
error: patch failed:
chrome/browser/extensions/content_verifier_browsertest.cc:750
error: chrome/browser/extensions/content_verifier_browsertest.cc: patch does
not apply
Patch: chrome/browser/extensions/content_verifier_browsertest.cc
Index: chrome/browser/extensions/content_verifier_browsertest.cc
diff --git a/chrome/browser/extensions/content_verifier_browsertest.cc
b/chrome/browser/extensions/content_verifier_browsertest.cc
index
645875d85ea483c839fbfd1eef33e050467d10cb..fa799e76dc2b1e09959dfed61cf4edd67472ed8e
100644
--- a/chrome/browser/extensions/content_verifier_browsertest.cc
+++ b/chrome/browser/extensions/content_verifier_browsertest.cc
@@ -6,6 +6,7 @@
#include <set>
#include <string>
+#include "base/bind_helpers.h"
#include "base/callback_helpers.h"
#include "base/macros.h"
#include "base/memory/ptr_util.h"
@@ -14,6 +15,7 @@
#include "base/strings/string_split.h"
#include "base/threading/thread_task_runner_handle.h"
#include "chrome/browser/extensions/browsertest_util.h"
+#include "chrome/browser/extensions/chrome_content_verifier_delegate.h"
#include "chrome/browser/extensions/extension_browsertest.h"
#include "chrome/browser/extensions/extension_management_test_util.h"
#include "chrome/browser/extensions/extension_service.h"
@@ -750,4 +752,66 @@ IN_PROC_BROWSER_TEST_F(ContentVerifierPolicyTest,
EXPECT_FALSE(reasons & Extension::DISABLE_CORRUPTED);
}
+namespace {
+
+// A helper for intercepting the normal action that
+// ChromeContentVerifierDelegate would take on discovering corruption, letting
+// us track the delay for each consecutive reinstall.
+class DelayTracker {
+ public:
+ DelayTracker() {}
+
+ const std::vector<base::TimeDelta>& calls() { return calls_; }
+
+ void ReinstallAction(base::TimeDelta delay) { calls_.push_back(delay); }
+
+ private:
+ std::vector<base::TimeDelta> calls_;
+
+ DISALLOW_COPY_AND_ASSIGN(DelayTracker);
+};
+
+} // namespace
+
+IN_PROC_BROWSER_TEST_F(ContentVerifierPolicyTest, Backoff) {
+ ExtensionRegistry* registry = ExtensionRegistry::Get(profile());
+ ExtensionSystem* system = ExtensionSystem::Get(profile());
+ ExtensionService* service = system->extension_service();
+ ContentVerifier* verifier = system->content_verifier();
+
+ // Wait for the extension to be installed by the policy we set up in
+ // SetUpInProcessBrowserTestFixture.
+ if (!registry->GetInstalledExtension(id_)) {
+ RegistryObserver registry_observer(registry);
+ EXPECT_TRUE(registry_observer.WaitForInstall(id_));
+ }
+
+ // Setup to intercept reinstall action, so we can see what the delay would
+ // have been for the real action.
+ DelayTracker delay_tracker;
+ base::Callback<void(base::TimeDelta)> action = base::Bind(
+ &DelayTracker::ReinstallAction, base::Unretained(&delay_tracker));
+ ChromeContentVerifierDelegate::set_policy_reinstall_action_for_test(&action);
+
+ // Do 4 iterations of disabling followed by reinstall.
+ const size_t iterations = 4;
+ for (size_t i = 0; i < iterations; i++) {
+ RegistryObserver registry_observer(registry);
+ verifier->VerifyFailed(id_, ContentVerifyJob::HASH_MISMATCH);
+ EXPECT_TRUE(registry_observer.WaitForUnload(id_));
+ // Trigger reinstall manually (since we overrode default reinstall action).
+ service->CheckForExternalUpdates();
+ EXPECT_TRUE(registry_observer.WaitForInstall(id_));
+ }
+ const std::vector<base::TimeDelta>& calls = delay_tracker.calls();
+
+ // Assert that the first reinstall action happened with a delay of 0, and
+ // then kept growing each additional time.
+ ASSERT_EQ(iterations, calls.size());
+ EXPECT_EQ(base::TimeDelta(), delay_tracker.calls()[0]);
+ for (size_t i = 1; i < delay_tracker.calls().size(); i++) {
+ EXPECT_LT(calls[i - 1], calls[i]);
+ }
+}
+
} // namespace extensions
The CQ bit was checked by asargent@chromium.org
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/2533873003/#ps60001 (title: "merge latest origin/master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480539779489460,
"parent_rev": "1f0ea7ced687fb9d92db0e3017c23b08736efeec", "commit_rev":
"49071e4ca33b42825afbf792581302c15a3b9c2e"}
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add throttling to corrupt policy extensions reinstall For crbug.com/447040 (https://codereview.chromium.org/2299203004) we made it so that enterprise policy force installed extensions would get automatically reinstalled when we detected corruption. However, we didn't add in any throttling, which was a bad idea because we turned out to have a bug (crbug.com/643814) where particular extensions were always incorrectly detected as corrupt right after install. So at least one enterprise with one of these extensions in their force install list ended up having chrome spin in an loop redownloading/reinstalling one of the affected extensions, eating up tons of bandwidth and disk space. This CL adds in throttling with an exponentially backed off delay on each reinstall attempt, up to a maximum of 30 minutes. The delay value is only maintained in memory during the course of a single run of chrome and resets after 6 hours (or on browser restart), but should be sufficient to drastically reduce the problem reported for this bug. BUG=661738 ========== to ========== Add throttling to corrupt policy extensions reinstall For crbug.com/447040 (https://codereview.chromium.org/2299203004) we made it so that enterprise policy force installed extensions would get automatically reinstalled when we detected corruption. However, we didn't add in any throttling, which was a bad idea because we turned out to have a bug (crbug.com/643814) where particular extensions were always incorrectly detected as corrupt right after install. So at least one enterprise with one of these extensions in their force install list ended up having chrome spin in an loop redownloading/reinstalling one of the affected extensions, eating up tons of bandwidth and disk space. This CL adds in throttling with an exponentially backed off delay on each reinstall attempt, up to a maximum of 30 minutes. The delay value is only maintained in memory during the course of a single run of chrome and resets after 6 hours (or on browser restart), but should be sufficient to drastically reduce the problem reported for this bug. BUG=661738 Committed: https://crrev.com/6014bdfa1f211946627a7085c87fe922e1581fc5 Cr-Commit-Position: refs/heads/master@{#435430} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6014bdfa1f211946627a7085c87fe922e1581fc5 Cr-Commit-Position: refs/heads/master@{#435430} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
