Rate limit programmatic update checks for extensions (reland)
Previously extensions could call the runtime.requestUpdateCheck method
as often as every 5 seconds. This CL introduces a more restrictive
policy of around one extra check per autoupdate period, and changes the
implementation location of the rate limiting from within the
ExtensionUpdater code itself to the ChromeRuntimeAPIDelegate, and makes
it more flexible by using net::BackoffEntry.
This is a reland of https://codereview.chromium.org/1887253002, with
the removal of a static initializer.
BUG=599310
Review URL: https://codereview.chromium.org/1897293005
Cr-Commit-Position: refs/heads/master@{#388654}
diff --git a/chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.cc b/chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.cc
index 1368740..143337d 100644
--- a/chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.cc
+++ b/chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.cc
@@ -4,9 +4,12 @@
#include "chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.h"
+#include <memory>
#include <string>
#include <utility>
+#include <vector>
+#include "base/lazy_instance.h"
#include "base/location.h"
#include "base/metrics/histogram.h"
#include "base/single_thread_task_runner.h"
@@ -23,11 +26,14 @@
#include "chrome/browser/ui/browser_window.h"
#include "components/update_client/update_query_params.h"
#include "content/public/browser/notification_service.h"
+#include "extensions/browser/extension_registry.h"
#include "extensions/browser/extension_system.h"
#include "extensions/browser/notification_types.h"
#include "extensions/browser/warning_service.h"
#include "extensions/browser/warning_set.h"
#include "extensions/common/api/runtime.h"
+#include "extensions/common/constants.h"
+#include "net/base/backoff_entry.h"
#if defined(OS_CHROMEOS)
#include "chromeos/dbus/dbus_thread_manager.h"
@@ -55,19 +61,97 @@
// disabled.
const int kFastReloadCount = 5;
+// A holder class for the policy we use for exponential backoff of update check
+// requests.
+class BackoffPolicy {
+ public:
+ BackoffPolicy();
+ ~BackoffPolicy();
+
+ // Returns the actual policy to use.
+ static const net::BackoffEntry::Policy* Get();
+
+ private:
+ net::BackoffEntry::Policy policy_;
+};
+
+// We use a LazyInstance since one of the the policy values references an
+// extern symbol, which would cause a static initializer to be generated if we
+// just declared the policy struct as a static variable.
+base::LazyInstance<BackoffPolicy> g_backoff_policy = LAZY_INSTANCE_INITIALIZER;
+
+BackoffPolicy::BackoffPolicy() {
+ policy_ = {
+ // num_errors_to_ignore
+ 0,
+
+ // initial_delay_ms (note that we set 'always_use_initial_delay' to false
+ // below)
+ 1000 * extensions::kDefaultUpdateFrequencySeconds,
+
+ // multiply_factor
+ 1,
+
+ // jitter_factor
+ 0.1,
+
+ // maximum_backoff_ms (-1 means no maximum)
+ -1,
+
+ // entry_lifetime_ms (-1 means never discard)
+ -1,
+
+ // always_use_initial_delay
+ false,
+ };
+}
+
+BackoffPolicy::~BackoffPolicy() {}
+
+// static
+const net::BackoffEntry::Policy* BackoffPolicy::Get() {
+ return &g_backoff_policy.Get().policy_;
+}
+
+base::TickClock* g_test_clock = nullptr;
+
} // namespace
+struct ChromeRuntimeAPIDelegate::UpdateCheckInfo {
+ public:
+ UpdateCheckInfo() {
+ if (g_test_clock)
+ backoff.reset(
+ new net::BackoffEntry(BackoffPolicy::Get(), g_test_clock));
+ else
+ backoff.reset(new net::BackoffEntry(BackoffPolicy::Get()));
+ }
+
+ std::unique_ptr<net::BackoffEntry> backoff;
+ std::vector<UpdateCheckCallback> callbacks;
+};
+
ChromeRuntimeAPIDelegate::ChromeRuntimeAPIDelegate(
content::BrowserContext* context)
- : browser_context_(context), registered_for_updates_(false) {
+ : browser_context_(context),
+ registered_for_updates_(false),
+ extension_registry_observer_(this) {
registrar_.Add(this,
extensions::NOTIFICATION_EXTENSION_UPDATE_FOUND,
content::NotificationService::AllSources());
+ extension_registry_observer_.Add(
+ extensions::ExtensionRegistry::Get(browser_context_));
}
ChromeRuntimeAPIDelegate::~ChromeRuntimeAPIDelegate() {
}
+// static
+void ChromeRuntimeAPIDelegate::set_tick_clock_for_tests(
+ base::TickClock* clock) {
+ g_test_clock = clock;
+}
+
void ChromeRuntimeAPIDelegate::AddUpdateObserver(
extensions::UpdateObserver* observer) {
registered_for_updates_ = true;
@@ -152,17 +236,20 @@
if (!updater) {
return false;
}
- if (!updater->CheckExtensionSoon(
- extension_id,
- base::Bind(&ChromeRuntimeAPIDelegate::UpdateCheckComplete,
- base::Unretained(this),
- extension_id))) {
+
+ UpdateCheckInfo& info = update_check_info_[extension_id];
+
+ // If not enough time has elapsed, or we have 10 or more outstanding calls,
+ // return a status of throttled.
+ if (info.backoff->ShouldRejectRequest() || info.callbacks.size() >= 10) {
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE,
base::Bind(callback, UpdateCheckResult(true, kUpdateThrottled, "")));
} else {
- UpdateCallbackList& callbacks = pending_update_checks_[extension_id];
- callbacks.push_back(callback);
+ info.callbacks.push_back(callback);
+ updater->CheckExtensionSoon(
+ extension_id, base::Bind(&ChromeRuntimeAPIDelegate::UpdateCheckComplete,
+ base::Unretained(this), extension_id));
}
return true;
}
@@ -259,11 +346,31 @@
}
}
+void ChromeRuntimeAPIDelegate::OnExtensionInstalled(
+ content::BrowserContext* browser_context,
+ const Extension* extension,
+ bool is_update) {
+ if (!is_update)
+ return;
+ auto info = update_check_info_.find(extension->id());
+ if (info != update_check_info_.end()) {
+ info->second.backoff->Reset();
+ }
+}
+
void ChromeRuntimeAPIDelegate::UpdateCheckComplete(
const std::string& extension_id) {
ExtensionSystem* system = ExtensionSystem::Get(browser_context_);
ExtensionService* service = system->extension_service();
const Extension* update = service->GetPendingExtensionUpdate(extension_id);
+ UpdateCheckInfo& info = update_check_info_[extension_id];
+
+ // We always inform the BackoffEntry of a "failure" here, because we only
+ // want to consider an update check request a success from a throttling
+ // standpoint once the extension goes on to actually update to a new
+ // version. See OnExtensionInstalled for where we reset the BackoffEntry.
+ info.backoff->InformOfRequest(false);
+
if (update) {
CallUpdateCallbacks(
extension_id,
@@ -277,12 +384,12 @@
void ChromeRuntimeAPIDelegate::CallUpdateCallbacks(
const std::string& extension_id,
const UpdateCheckResult& result) {
- UpdateCallbackList callbacks = pending_update_checks_[extension_id];
- pending_update_checks_.erase(extension_id);
- for (UpdateCallbackList::const_iterator iter = callbacks.begin();
- iter != callbacks.end();
- ++iter) {
- const UpdateCheckCallback& callback = *iter;
+ auto it = update_check_info_.find(extension_id);
+ if (it == update_check_info_.end())
+ return;
+ std::vector<UpdateCheckCallback> callbacks;
+ it->second.callbacks.swap(callbacks);
+ for (const auto& callback : callbacks) {
callback.Run(result);
}
}
diff --git a/chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.h b/chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.h
index 934bcd8..6ff1a05 100644
--- a/chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.h
+++ b/chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.h
@@ -6,15 +6,19 @@
#define CHROME_BROWSER_EXTENSIONS_API_RUNTIME_CHROME_RUNTIME_API_DELEGATE_H_
#include <map>
-#include <vector>
+#include <string>
+#include <utility>
#include "base/macros.h"
+#include "base/scoped_observer.h"
#include "content/public/browser/notification_observer.h"
#include "content/public/browser/notification_registrar.h"
#include "extensions/browser/api/runtime/runtime_api.h"
#include "extensions/browser/api/runtime/runtime_api_delegate.h"
+#include "extensions/browser/extension_registry_observer.h"
namespace base {
+class TickClock;
class TimeTicks;
}
@@ -25,16 +29,21 @@
}
namespace extensions {
+class ExtensionRegistry;
class RuntimeAPI;
class UpdateObserver;
}
class ChromeRuntimeAPIDelegate : public extensions::RuntimeAPIDelegate,
- public content::NotificationObserver {
+ public content::NotificationObserver,
+ public extensions::ExtensionRegistryObserver {
public:
explicit ChromeRuntimeAPIDelegate(content::BrowserContext* context);
~ChromeRuntimeAPIDelegate() override;
+ // Sets a custom TickClock to use in tests.
+ static void set_tick_clock_for_tests(base::TickClock* clock);
+
private:
friend class extensions::RuntimeAPI;
@@ -56,6 +65,11 @@
const content::NotificationSource& source,
const content::NotificationDetails& details) override;
+ // ExtensionRegistryObserver implementation.
+ void OnExtensionInstalled(content::BrowserContext* browser_context,
+ const extensions::Extension* extension,
+ bool is_update) override;
+
void UpdateCheckComplete(const std::string& extension_id);
void CallUpdateCallbacks(const std::string& extension_id,
const UpdateCheckResult& result);
@@ -73,12 +87,14 @@
// it was reloaded with not enough time in between reloads.
std::map<std::string, std::pair<base::TimeTicks, int> > last_reload_time_;
- // Pending update checks.
- typedef std::vector<UpdateCheckCallback> UpdateCallbackList;
- typedef std::map<std::string, UpdateCallbackList> UpdateCallbackMap;
- UpdateCallbackMap pending_update_checks_;
+ // Information about update checks, keyed by extension id.
+ struct UpdateCheckInfo;
+ std::map<std::string, UpdateCheckInfo> update_check_info_;
- private:
+ ScopedObserver<extensions::ExtensionRegistry,
+ extensions::ExtensionRegistryObserver>
+ extension_registry_observer_;
+
DISALLOW_COPY_AND_ASSIGN(ChromeRuntimeAPIDelegate);
};
diff --git a/chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate_unittest.cc b/chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate_unittest.cc
new file mode 100644
index 0000000..0313064
--- /dev/null
+++ b/chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate_unittest.cc
@@ -0,0 +1,317 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <deque>
+#include <memory>
+#include <set>
+#include <utility>
+
+#include "base/callback.h"
+#include "base/files/file_path.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted.h"
+#include "base/test/simple_test_tick_clock.h"
+#include "chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_service_test_with_install.h"
+#include "chrome/browser/extensions/test_extension_system.h"
+#include "chrome/browser/extensions/update_install_gate.h"
+#include "chrome/browser/extensions/updater/extension_updater.h"
+#include "extensions/browser/event_router.h"
+#include "extensions/browser/event_router_factory.h"
+#include "extensions/browser/extension_prefs.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/updater/extension_downloader.h"
+#include "extensions/browser/updater/extension_downloader_test_delegate.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace extensions {
+
+namespace {
+
+// A fake EventRouter that lets us pretend an extension has a listener
+// registered for named events.
+class TestEventRouter : public EventRouter {
+ public:
+ explicit TestEventRouter(content::BrowserContext* context)
+ : EventRouter(context, ExtensionPrefs::Get(context)) {}
+ ~TestEventRouter() override {}
+
+ // An entry in our fake event registry.
+ using Entry = std::pair<std::string, std::string>;
+
+ bool ExtensionHasEventListener(const std::string& extension_id,
+ const std::string& event_name) override {
+ return fake_registry_.find(Entry(extension_id, event_name)) !=
+ fake_registry_.end();
+ }
+
+ // Pretend that |extension_id| is listening for |event_name|.
+ void AddFakeListener(const std::string& extension_id,
+ const std::string& event_name) {
+ fake_registry_.insert(Entry(extension_id, event_name));
+ }
+
+ private:
+ std::set<Entry> fake_registry_;
+
+ DISALLOW_COPY_AND_ASSIGN(TestEventRouter);
+};
+
+std::unique_ptr<KeyedService> TestEventRouterFactoryFunction(
+ content::BrowserContext* context) {
+ return base::WrapUnique(new TestEventRouter(context));
+}
+
+// This class lets us intercept extension update checks and respond as if
+// either no update was found, or one was (and it was downloaded).
+class DownloaderTestDelegate : public ExtensionDownloaderTestDelegate {
+ public:
+ DownloaderTestDelegate() {}
+
+ // On the next update check for extension |id|, we'll respond that no update
+ // is available.
+ void AddNoUpdateResponse(const std::string& id) {
+ no_updates_.insert(id);
+ if (updates_.find(id) != updates_.end())
+ updates_.erase(id);
+ }
+
+ // On the next update check for extension |id|, pretend that an update to
+ // version |version| has been downloaded to |path|.
+ void AddUpdateResponse(const std::string& id,
+ const base::FilePath& path,
+ const std::string& version) {
+ if (no_updates_.find(id) != no_updates_.end())
+ no_updates_.erase(id);
+ DownloadFinishedArgs args;
+ args.path = path;
+ args.version = version;
+ updates_[id] = std::move(args);
+ }
+
+ void StartUpdateCheck(
+ ExtensionDownloader* downloader,
+ ExtensionDownloaderDelegate* delegate,
+ std::unique_ptr<ManifestFetchData> fetch_data) override {
+ // Instead of immediately firing callbacks to the delegate in matching
+ // cases below, we instead post a task since the delegate typically isn't
+ // expecting a synchronous reply (the real code has to go do at least one
+ // network request before getting a response, so this is is a reasonable
+ // expectation by delegates).
+ for (const std::string& id : fetch_data->extension_ids()) {
+ auto no_update = no_updates_.find(id);
+ if (no_update != no_updates_.end()) {
+ no_updates_.erase(no_update);
+ base::MessageLoop::current()->task_runner()->PostTask(
+ FROM_HERE,
+ base::Bind(&ExtensionDownloaderDelegate::OnExtensionDownloadFailed,
+ base::Unretained(delegate), id,
+ ExtensionDownloaderDelegate::NO_UPDATE_AVAILABLE,
+ ExtensionDownloaderDelegate::PingResult(),
+ fetch_data->request_ids()));
+ continue;
+ }
+ auto update = updates_.find(id);
+ if (update != updates_.end()) {
+ CRXFileInfo info(id, update->second.path, "" /* no hash */);
+ std::string version = update->second.version;
+ updates_.erase(update);
+ base::MessageLoop::current()->task_runner()->PostTask(
+ FROM_HERE,
+ base::Bind(
+ &ExtensionDownloaderDelegate::OnExtensionDownloadFinished,
+ base::Unretained(delegate), info,
+ false /* file_ownership_passed */, GURL(), version,
+ ExtensionDownloaderDelegate::PingResult(),
+ fetch_data->request_ids(),
+ ExtensionDownloaderDelegate::InstallCallback()));
+ continue;
+ }
+ ADD_FAILURE() << "Unexpected extension id " << id;
+ }
+ }
+
+ private:
+ // Simple holder for the data passed in AddUpdateResponse calls.
+ struct DownloadFinishedArgs {
+ base::FilePath path;
+ std::string version;
+ };
+
+ // These keep track of what response we should give for update checks, keyed
+ // by extension id. A given extension id should only appear in one or the
+ // other.
+ std::set<std::string> no_updates_;
+ std::map<std::string, DownloadFinishedArgs> updates_;
+
+ DISALLOW_COPY_AND_ASSIGN(DownloaderTestDelegate);
+};
+
+// Helper to let test code wait for and return an update check result.
+class UpdateCheckResultCatcher {
+ public:
+ UpdateCheckResultCatcher() {}
+
+ void OnResult(const RuntimeAPIDelegate::UpdateCheckResult& result) {
+ EXPECT_EQ(nullptr, result_.get());
+ result_.reset(new RuntimeAPIDelegate::UpdateCheckResult(
+ result.success, result.response, result.version));
+ if (run_loop_)
+ run_loop_->Quit();
+ }
+
+ std::unique_ptr<RuntimeAPIDelegate::UpdateCheckResult> WaitForResult() {
+ if (!result_) {
+ run_loop_.reset(new base::RunLoop);
+ run_loop_->Run();
+ }
+ return std::move(result_);
+ }
+
+ private:
+ std::unique_ptr<RuntimeAPIDelegate::UpdateCheckResult> result_;
+ std::unique_ptr<base::RunLoop> run_loop_;
+
+ DISALLOW_COPY_AND_ASSIGN(UpdateCheckResultCatcher);
+};
+
+class ChromeRuntimeAPIDelegateTest : public ExtensionServiceTestWithInstall {
+ public:
+ ChromeRuntimeAPIDelegateTest() {}
+
+ void SetUp() override {
+ ExtensionServiceTestWithInstall::SetUp();
+ ExtensionDownloader::set_test_delegate(&downloader_test_delegate_);
+ ChromeRuntimeAPIDelegate::set_tick_clock_for_tests(&clock_);
+
+ InitializeExtensionServiceWithUpdater();
+ runtime_delegate_.reset(new ChromeRuntimeAPIDelegate(browser_context()));
+ service()->updater()->SetExtensionCacheForTesting(nullptr);
+ EventRouterFactory::GetInstance()->SetTestingFactory(
+ browser_context(), &TestEventRouterFactoryFunction);
+
+ // Setup the ExtensionService so that extension updates won't complete
+ // installation until the extension is idle.
+ update_install_gate_.reset(new UpdateInstallGate(service()));
+ service()->RegisterInstallGate(ExtensionPrefs::DELAY_REASON_WAIT_FOR_IDLE,
+ update_install_gate_.get());
+ static_cast<TestExtensionSystem*>(ExtensionSystem::Get(browser_context()))
+ ->SetReady();
+ }
+
+ void TearDown() override {
+ ExtensionDownloader::set_test_delegate(nullptr);
+ ChromeRuntimeAPIDelegate::set_tick_clock_for_tests(nullptr);
+ ExtensionServiceTestWithInstall::TearDown();
+ }
+
+ // Uses runtime_delegate_ to run an update check for |id|, expecting
+ // |expected_response| and (if an update was available) |expected_version|.
+ // The |expected_response| should be one of 'throttled', 'no_update', or
+ // 'update_available'.
+ void DoUpdateCheck(const std::string& id,
+ const std::string& expected_response,
+ const std::string& expected_version) {
+ UpdateCheckResultCatcher catcher;
+ EXPECT_TRUE(runtime_delegate_->CheckForUpdates(
+ id, base::Bind(&UpdateCheckResultCatcher::OnResult,
+ base::Unretained(&catcher))));
+ std::unique_ptr<RuntimeAPIDelegate::UpdateCheckResult> result =
+ catcher.WaitForResult();
+ ASSERT_NE(nullptr, result.get());
+ EXPECT_TRUE(result->success);
+ EXPECT_EQ(expected_response, result->response);
+ EXPECT_EQ(expected_version, result->version);
+ }
+
+ protected:
+ // A clock we pass to the code used for throttling, so that we can manually
+ // increment time to test various throttling scenarios.
+ base::SimpleTestTickClock clock_;
+
+ // Used for intercepting update check requests and possibly returning fake
+ // download results.
+ DownloaderTestDelegate downloader_test_delegate_;
+
+ // The object whose behavior we're testing.
+ std::unique_ptr<RuntimeAPIDelegate> runtime_delegate_;
+
+ // For preventing extensions from being updated immediately.
+ std::unique_ptr<UpdateInstallGate> update_install_gate_;
+
+ private:
+ DISALLOW_COPY_AND_ASSIGN(ChromeRuntimeAPIDelegateTest);
+};
+
+TEST_F(ChromeRuntimeAPIDelegateTest, RequestUpdateCheck) {
+ base::FilePath v1_path = data_dir().AppendASCII("autoupdate/v1.crx");
+ base::FilePath v2_path = data_dir().AppendASCII("autoupdate/v2.crx");
+
+ // Start by installing version 1.
+ scoped_refptr<const Extension> v1(InstallCRX(v1_path, INSTALL_NEW));
+ std::string id = v1->id();
+
+ // Make it look like our test extension listens for the
+ // runtime.onUpdateAvailable event, so that it won't be updated immediately
+ // when the ExtensionUpdater hands the new version to the ExtensionService.
+ TestEventRouter* event_router =
+ static_cast<TestEventRouter*>(EventRouter::Get(browser_context()));
+ event_router->AddFakeListener(id, "runtime.onUpdateAvailable");
+
+ // Run an update check that should get a "no_update" response.
+ downloader_test_delegate_.AddNoUpdateResponse(id);
+ DoUpdateCheck(id, "no_update", "");
+
+ // Check again after a short delay - we should be throttled because
+ // not enough time has passed.
+ clock_.Advance(base::TimeDelta::FromMinutes(15));
+ downloader_test_delegate_.AddNoUpdateResponse(id);
+ DoUpdateCheck(id, "throttled", "");
+
+ // Now simulate checking a few times at a 6 hour interval - none of these
+ // should be throttled.
+ for (int i = 0; i < 5; i++) {
+ clock_.Advance(base::TimeDelta::FromHours(6));
+ downloader_test_delegate_.AddNoUpdateResponse(id);
+ DoUpdateCheck(id, "no_update", "");
+ }
+
+ // Run an update check that should get an "update_available" response. This
+ // actually causes the new version to be downloaded/unpacked, but the install
+ // will not complete until we reload the extension.
+ clock_.Advance(base::TimeDelta::FromDays(1));
+ downloader_test_delegate_.AddUpdateResponse(id, v2_path, "2.0");
+ DoUpdateCheck(id, "update_available", "2.0");
+
+ // Call again after short delay - it should be throttled instead of getting
+ // another "update_available" response.
+ clock_.Advance(base::TimeDelta::FromMinutes(30));
+ downloader_test_delegate_.AddUpdateResponse(id, v2_path, "2.0");
+ DoUpdateCheck(id, "throttled", "");
+
+ // Reload the extension, causing the delayed update to v2 to happen, then do
+ // another update check - we should get a no_update instead of throttled.
+ service()->ReloadExtension(id);
+ const Extension* current =
+ ExtensionRegistry::Get(browser_context())->GetInstalledExtension(id);
+ ASSERT_NE(nullptr, current);
+ EXPECT_EQ("2.0", current->VersionString());
+ clock_.Advance(base::TimeDelta::FromSeconds(10));
+ downloader_test_delegate_.AddNoUpdateResponse(id);
+ DoUpdateCheck(id, "no_update", "");
+
+ // Check again after short delay; we should be throttled.
+ clock_.Advance(base::TimeDelta::FromMinutes(5));
+ DoUpdateCheck(id, "throttled", "");
+
+ // Call again after a longer delay, we should should be unthrottled.
+ clock_.Advance(base::TimeDelta::FromHours(8));
+ downloader_test_delegate_.AddNoUpdateResponse(id);
+ DoUpdateCheck(id, "no_update", "");
+}
+
+} // namespace
+
+} // namespace extensions
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 43f3ca0..a16b493 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -518,7 +518,8 @@
<< " because it is not installed or pending";
// Delete extension_path since we're not creating a CrxInstaller
// that would do it for us.
- if (!GetFileTaskRunner()->PostTask(
+ if (file_ownership_passed &&
+ !GetFileTaskRunner()->PostTask(
FROM_HERE,
base::Bind(&extensions::file_util::DeleteFile, file.path, false)))
NOTREACHED();
@@ -691,12 +692,12 @@
}
void ExtensionService::ReloadExtension(const std::string& extension_id) {
- ReloadExtensionImpl(extension_id, true); // be_noisy
+ ReloadExtensionImpl(extension_id, true); // be_noisy
}
void ExtensionService::ReloadExtensionWithQuietFailure(
const std::string& extension_id) {
- ReloadExtensionImpl(extension_id, false); // be_noisy
+ ReloadExtensionImpl(extension_id, false); // be_noisy
}
bool ExtensionService::UninstallExtension(
diff --git a/chrome/browser/extensions/updater/extension_updater.cc b/chrome/browser/extensions/updater/extension_updater.cc
index bd93ba3..484029f7 100644
--- a/chrome/browser/extensions/updater/extension_updater.cc
+++ b/chrome/browser/extensions/updater/extension_updater.cc
@@ -60,10 +60,6 @@
#endif
const int kMaxUpdateFrequencySeconds = 60 * 60 * 24 * 7; // 7 days
-// Require at least 5 seconds between consecutive non-succesful extension update
-// checks.
-const int kMinUpdateThrottleTime = 5;
-
// When we've computed a days value, we want to make sure we don't send a
// negative value (due to the system clock being set backwards, etc.), since -1
// is a special sentinel value that means "never pinged", and other negative
@@ -129,17 +125,6 @@
ExtensionUpdater::InProgressCheck::~InProgressCheck() {}
-struct ExtensionUpdater::ThrottleInfo {
- ThrottleInfo()
- : in_progress(true),
- throttle_delay(kMinUpdateThrottleTime),
- check_start(Time::Now()) {}
-
- bool in_progress;
- int throttle_delay;
- Time check_start;
-};
-
ExtensionUpdater::ExtensionUpdater(
ExtensionServiceInterface* service,
ExtensionPrefs* extension_prefs,
@@ -157,7 +142,6 @@
prefs_(prefs),
profile_(profile),
next_request_id_(0),
- extension_registry_observer_(this),
crx_install_is_running_(false),
extension_cache_(cache),
weak_ptr_factory_(this) {
@@ -168,8 +152,6 @@
frequency_seconds_ = std::max(frequency_seconds_, kMinUpdateFrequencySeconds);
#endif
frequency_seconds_ = std::min(frequency_seconds_, kMaxUpdateFrequencySeconds);
-
- extension_registry_observer_.Add(ExtensionRegistry::Get(profile));
}
ExtensionUpdater::~ExtensionUpdater() {
@@ -422,56 +404,12 @@
NotifyIfFinished(request_id);
}
-bool ExtensionUpdater::CheckExtensionSoon(const std::string& extension_id,
+void ExtensionUpdater::CheckExtensionSoon(const std::string& extension_id,
const FinishedCallback& callback) {
- bool have_throttle_info = ContainsKey(throttle_info_, extension_id);
- ThrottleInfo& info = throttle_info_[extension_id];
- if (have_throttle_info) {
- // We already had a ThrottleInfo object for this extension, check if the
- // update check request should be allowed.
-
- // If another check is in progress, don't start a new check.
- if (info.in_progress)
- return false;
-
- Time now = Time::Now();
- Time last = info.check_start;
- // If somehow time moved back, we don't want to infinitely keep throttling.
- if (now < last) {
- last = now;
- info.check_start = now;
- }
- Time earliest = last + TimeDelta::FromSeconds(info.throttle_delay);
- // If check is too soon, throttle.
- if (now < earliest)
- return false;
-
- // TODO(mek): Somehow increase time between allowing checks when checks
- // are repeatedly throttled and don't result in updates being installed.
-
- // It's okay to start a check, update values.
- info.check_start = now;
- info.in_progress = true;
- }
-
CheckParams params;
params.ids.push_back(extension_id);
- params.callback = base::Bind(&ExtensionUpdater::ExtensionCheckFinished,
- weak_ptr_factory_.GetWeakPtr(),
- extension_id, callback);
+ params.callback = callback;
CheckNow(params);
- return true;
-}
-
-void ExtensionUpdater::ExtensionCheckFinished(
- const std::string& extension_id,
- const FinishedCallback& callback) {
- std::map<std::string, ThrottleInfo>::iterator it =
- throttle_info_.find(extension_id);
- if (it != throttle_info_.end()) {
- it->second.in_progress = false;
- }
- callback.Run();
}
void ExtensionUpdater::OnExtensionDownloadFailed(
@@ -659,14 +597,6 @@
MaybeInstallCRXFile();
}
-void ExtensionUpdater::OnExtensionWillBeInstalled(
- content::BrowserContext* browser_context,
- const Extension* extension,
- bool is_update,
- const std::string& old_name) {
- throttle_info_.erase(extension->id());
-}
-
void ExtensionUpdater::NotifyStarted() {
content::NotificationService::current()->Notify(
extensions::NOTIFICATION_EXTENSION_UPDATING_STARTED,
diff --git a/chrome/browser/extensions/updater/extension_updater.h b/chrome/browser/extensions/updater/extension_updater.h
index d2568296..92851e8 100644
--- a/chrome/browser/extensions/updater/extension_updater.h
+++ b/chrome/browser/extensions/updater/extension_updater.h
@@ -56,7 +56,6 @@
// ....
// updater->Stop();
class ExtensionUpdater : public ExtensionDownloaderDelegate,
- public ExtensionRegistryObserver,
public content::NotificationObserver {
public:
typedef base::Closure FinishedCallback;
@@ -104,10 +103,8 @@
// already a pending task that has not yet run.
void CheckSoon();
- // Starts an update check for the specified extension soon. If a check
- // is already running, or finished too recently without an update being
- // installed, this method returns false and the check won't be scheduled.
- bool CheckExtensionSoon(const std::string& extension_id,
+ // Starts an update check for the specified extension soon.
+ void CheckExtensionSoon(const std::string& extension_id,
const FinishedCallback& callback);
// Starts an update check right now, instead of waiting for the next
@@ -158,8 +155,6 @@
std::list<std::string> in_progress_ids_;
};
- struct ThrottleInfo;
-
// Ensure that we have a valid ExtensionDownloader instance referenced by
// |downloader|.
void EnsureDownloaderCreated();
@@ -214,12 +209,6 @@
const content::NotificationSource& source,
const content::NotificationDetails& details) override;
- // Implementation of ExtensionRegistryObserver.
- void OnExtensionWillBeInstalled(content::BrowserContext* browser_context,
- const Extension* extension,
- bool is_update,
- const std::string& old_name) override;
-
// Send a notification that update checks are starting.
void NotifyStarted();
@@ -256,9 +245,6 @@
// Observes CRX installs we initiate.
content::NotificationRegistrar registrar_;
- ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver>
- extension_registry_observer_;
-
// True when a CrxInstaller is doing an install. Used in MaybeUpdateCrxFile()
// to keep more than one install from running at once.
bool crx_install_is_running_;
@@ -269,10 +255,6 @@
ExtensionCache* extension_cache_;
- // Keeps track of when an extension tried to update itself, so we can throttle
- // checks to prevent too many requests from being made.
- std::map<std::string, ThrottleInfo> throttle_info_;
-
base::WeakPtrFactory<ExtensionUpdater> weak_ptr_factory_;
DISALLOW_COPY_AND_ASSIGN(ExtensionUpdater);
