diff options
author | sorin <sorin@chromium.org> | 2015-05-26 12:59:09 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-05-26 20:00:01 +0000 |
commit | 7c7176234e553c35a2d8ec014f2caa29f7278065 (patch) | |
tree | 70eb3c58c82369cc13c939c17ae63344644c4b31 /components/update_client | |
parent | 92780e77b6e0755e1d4bacbd493032969d04293b (diff) | |
download | chromium_src-7c7176234e553c35a2d8ec014f2caa29f7278065.zip chromium_src-7c7176234e553c35a2d8ec014f2caa29f7278065.tar.gz chromium_src-7c7176234e553c35a2d8ec014f2caa29f7278065.tar.bz2 |
Rewrite component update service in terms of components/update_client.
The goal of this change is to re-implement the component updater by
reusing the common code in components/update_client while keeping
the its public interface the same as before, in order to minimize
changes in its existing clients.
BUG=450337
Review URL: https://codereview.chromium.org/1133443002
Cr-Commit-Position: refs/heads/master@{#331412}
Diffstat (limited to 'components/update_client')
-rw-r--r-- | components/update_client/action.cc | 2 | ||||
-rw-r--r-- | components/update_client/action_update.cc | 26 | ||||
-rw-r--r-- | components/update_client/action_update.h | 8 | ||||
-rw-r--r-- | components/update_client/action_update_check.cc | 6 | ||||
-rw-r--r-- | components/update_client/configurator.h | 11 | ||||
-rw-r--r-- | components/update_client/crx_update_item.h | 6 | ||||
-rw-r--r-- | components/update_client/task.h | 3 | ||||
-rw-r--r-- | components/update_client/task_update.cc | 14 | ||||
-rw-r--r-- | components/update_client/task_update.h | 21 | ||||
-rw-r--r-- | components/update_client/test_configurator.cc | 34 | ||||
-rw-r--r-- | components/update_client/test_configurator.h | 12 | ||||
-rw-r--r-- | components/update_client/update_client.cc | 43 | ||||
-rw-r--r-- | components/update_client/update_client.h | 13 | ||||
-rw-r--r-- | components/update_client/update_client_internal.h | 5 | ||||
-rw-r--r-- | components/update_client/update_client_unittest.cc | 361 | ||||
-rw-r--r-- | components/update_client/update_engine.cc | 8 | ||||
-rw-r--r-- | components/update_client/update_engine.h | 7 |
17 files changed, 439 insertions, 141 deletions
diff --git a/components/update_client/action.cc b/components/update_client/action.cc index 4a11efe..3cdce79 100644 --- a/components/update_client/action.cc +++ b/components/update_client/action.cc @@ -20,6 +20,8 @@ namespace update_client { +using Events = UpdateClient::Observer::Events; + namespace { // Returns true if a differential update is available, it has not failed yet, diff --git a/components/update_client/action_update.cc b/components/update_client/action_update.cc index 7aefb43..ed912c8 100644 --- a/components/update_client/action_update.cc +++ b/components/update_client/action_update.cc @@ -88,22 +88,22 @@ void ActionUpdate::Run(UpdateContext* update_context, Callback callback) { } void ActionUpdate::DownloadProgress( - const std::string& crx_id, + const std::string& id, const CrxDownloader::Result& download_result) { DCHECK(thread_checker_.CalledOnValidThread()); - DCHECK(crx_id == update_context_->queue.front()); + DCHECK(id == update_context_->queue.front()); using Events = UpdateClient::Observer::Events; - NotifyObservers(Events::COMPONENT_UPDATE_DOWNLOADING, crx_id); + NotifyObservers(Events::COMPONENT_UPDATE_DOWNLOADING, id); } void ActionUpdate::DownloadComplete( - const std::string& crx_id, + const std::string& id, const CrxDownloader::Result& download_result) { DCHECK(thread_checker_.CalledOnValidThread()); - DCHECK(crx_id == update_context_->queue.front()); + DCHECK(id == update_context_->queue.front()); - CrxUpdateItem* item = FindUpdateItemById(crx_id); + CrxUpdateItem* item = FindUpdateItemById(id); DCHECK(item); AppendDownloadMetrics(update_context_->crx_downloader->download_metrics(), @@ -115,7 +115,7 @@ void ActionUpdate::DownloadComplete( OnDownloadSuccess(item, download_result); update_context_->main_task_runner->PostDelayedTask( FROM_HERE, base::Bind(&ActionUpdate::Install, base::Unretained(this), - crx_id, download_result.response), + id, download_result.response), base::TimeDelta::FromMilliseconds( update_context_->config->StepDelay())); } @@ -123,12 +123,12 @@ void ActionUpdate::DownloadComplete( update_context_->crx_downloader.reset(); } -void ActionUpdate::Install(const std::string& crx_id, +void ActionUpdate::Install(const std::string& id, const base::FilePath& crx_path) { DCHECK(thread_checker_.CalledOnValidThread()); - DCHECK(crx_id == update_context_->queue.front()); + DCHECK(id == update_context_->queue.front()); - CrxUpdateItem* item = FindUpdateItemById(crx_id); + CrxUpdateItem* item = FindUpdateItemById(id); DCHECK(item); OnInstallStart(item); @@ -168,13 +168,13 @@ void ActionUpdate::EndUnpackingOnBlockingTaskRunner( base::TimeDelta::FromMilliseconds(update_context->config->StepDelay())); } -void ActionUpdate::DoneInstalling(const std::string& crx_id, +void ActionUpdate::DoneInstalling(const std::string& id, ComponentUnpacker::Error error, int extended_error) { DCHECK(thread_checker_.CalledOnValidThread()); - DCHECK(crx_id == update_context_->queue.front()); + DCHECK(id == update_context_->queue.front()); - CrxUpdateItem* item = FindUpdateItemById(crx_id); + CrxUpdateItem* item = FindUpdateItemById(id); DCHECK(item); if (error == ComponentUnpacker::kNone) diff --git a/components/update_client/action_update.h b/components/update_client/action_update.h index 8b33444..ddee94b 100644 --- a/components/update_client/action_update.h +++ b/components/update_client/action_update.h @@ -53,14 +53,14 @@ class ActionUpdate : public Action, protected ActionImpl { // Called when progress is being made downloading a CRX. The progress may // not monotonically increase due to how the CRX downloader switches between // different downloaders and fallback urls. - void DownloadProgress(const std::string& crx_id, + void DownloadProgress(const std::string& id, const CrxDownloader::Result& download_result); // Called when the CRX package has been downloaded to a temporary location. - void DownloadComplete(const std::string& crx_id, + void DownloadComplete(const std::string& id, const CrxDownloader::Result& download_result); - void Install(const std::string& crx_id, const base::FilePath& crx_path); + void Install(const std::string& id, const base::FilePath& crx_path); // TODO(sorin): refactor the public interface of ComponentUnpacker so // that these calls can run on the main thread. @@ -74,7 +74,7 @@ class ActionUpdate : public Action, protected ActionImpl { ComponentUnpacker::Error error, int extended_error); - void DoneInstalling(const std::string& crx_id, + void DoneInstalling(const std::string& id, ComponentUnpacker::Error error, int extended_error); diff --git a/components/update_client/action_update_check.cc b/components/update_client/action_update_check.cc index c480de8..d68940e 100644 --- a/components/update_client/action_update_check.cc +++ b/components/update_client/action_update_check.cc @@ -71,6 +71,7 @@ void ActionUpdateCheck::Run(UpdateContext* update_context, Callback callback) { item->next_version = Version(); item->previous_fp = crx_component.fingerprint; item->next_fp.clear(); + item->on_demand = update_context->is_foreground; item->diff_update_failed = false; item->error_category = 0; item->error_code = 0; @@ -80,9 +81,10 @@ void ActionUpdateCheck::Run(UpdateContext* update_context, Callback callback) { item->diff_extra_code1 = 0; item->download_metrics.clear(); - ChangeItemState(item.get(), CrxUpdateItem::State::kChecking); + update_context_->update_items.push_back(item.get()); - update_context_->update_items.push_back(item.release()); + ChangeItemState(item.get(), CrxUpdateItem::State::kChecking); + ignore_result(item.release()); } update_checker_->CheckForUpdates( diff --git a/components/update_client/configurator.h b/components/update_client/configurator.h index c4c763f..4bb103e 100644 --- a/components/update_client/configurator.h +++ b/components/update_client/configurator.h @@ -37,20 +37,11 @@ class Configurator : public base::RefCountedThreadSafe<Configurator> { virtual int InitialDelay() const = 0; // Delay in seconds to every subsequent update check. 0 means don't check. - // This function is a mutator for testing purposes. - virtual int NextCheckDelay() = 0; + virtual int NextCheckDelay() const = 0; // Delay in seconds from each task step. Used to smooth out CPU/IO usage. virtual int StepDelay() const = 0; - // Delay in seconds between applying updates for different components, if - // several updates are available at a given time. This function is a mutator - // for testing purposes. - virtual int StepDelayMedium() = 0; - - // Minimum delta time in seconds before checking again the same component. - virtual int MinimumReCheckWait() const = 0; - // Minimum delta time in seconds before an on-demand check is allowed // for the same component. virtual int OnDemandDelay() const = 0; diff --git a/components/update_client/crx_update_item.h b/components/update_client/crx_update_item.h index f066ca2..a9cf171 100644 --- a/components/update_client/crx_update_item.h +++ b/components/update_client/crx_update_item.h @@ -75,10 +75,6 @@ struct CrxUpdateItem { // enforce conditions or notify observers of the change. State state; - // True if the component was recently unregistered and will be uninstalled - // soon (after the currently operation is finished, if there is one). - bool unregistered; - std::string id; CrxComponent component; @@ -115,8 +111,6 @@ struct CrxUpdateItem { std::vector<CrxDownloader::DownloadMetrics> download_metrics; - std::vector<base::Closure> ready_callbacks; - CrxUpdateItem(); ~CrxUpdateItem(); diff --git a/components/update_client/task.h b/components/update_client/task.h index 51b6890..52af752 100644 --- a/components/update_client/task.h +++ b/components/update_client/task.h @@ -19,10 +19,9 @@ class Task; // together. class Task { public: - using Callback = base::Callback<void(Task* task, int error)>; virtual ~Task() {} - virtual void Run(const Callback& callback) = 0; + virtual void Run() = 0; }; } // namespace update_client diff --git a/components/update_client/task_update.cc b/components/update_client/task_update.cc index 2cbf001..ca8e35f 100644 --- a/components/update_client/task_update.cc +++ b/components/update_client/task_update.cc @@ -13,27 +13,29 @@ namespace update_client { TaskUpdate::TaskUpdate(UpdateEngine* update_engine, + bool is_foreground, const std::vector<std::string>& ids, - const UpdateClient::CrxDataCallback& crx_data_callback) + const UpdateClient::CrxDataCallback& crx_data_callback, + const Callback& callback) : update_engine_(update_engine), + is_foreground_(is_foreground), ids_(ids), - crx_data_callback_(crx_data_callback) { + crx_data_callback_(crx_data_callback), + callback_(callback) { } TaskUpdate::~TaskUpdate() { DCHECK(thread_checker_.CalledOnValidThread()); } -void TaskUpdate::Run(const Callback& callback) { +void TaskUpdate::Run() { DCHECK(thread_checker_.CalledOnValidThread()); - callback_ = callback; - if (ids_.empty()) RunComplete(-1); update_engine_->Update( - ids_, crx_data_callback_, + is_foreground_, ids_, crx_data_callback_, base::Bind(&TaskUpdate::RunComplete, base::Unretained(this))); } diff --git a/components/update_client/task_update.h b/components/update_client/task_update.h index 7a91c28..2cfac4f 100644 --- a/components/update_client/task_update.h +++ b/components/update_client/task_update.h @@ -22,12 +22,23 @@ class UpdateEngine; // Defines a specialized task for updating a group of CRXs. class TaskUpdate : public Task { public: + using Callback = base::Callback<void(Task* task, int error)>; + + // |update_engine| is injected here to handle the task. + // |is_foreground| is true when the update task is initiated by the user, + // most likely as a result of an on-demand call. + // |ids| represents the CRXs to be updated by this task. + // |crx_data_callback| is called to get update data for the these CRXs. + // |callback| is called to return the execution flow back to creator of + // this task when the task is done. TaskUpdate(UpdateEngine* update_engine, + bool is_foreground, const std::vector<std::string>& ids, - const UpdateClient::CrxDataCallback& crx_data_callback); + const UpdateClient::CrxDataCallback& crx_data_callback, + const Callback& callback); ~TaskUpdate() override; - void Run(const Callback& callback) override; + void Run() override; private: // Called when the Run function associated with this task has completed. @@ -37,12 +48,10 @@ class TaskUpdate : public Task { UpdateEngine* update_engine_; // Not owned by this class. + const bool is_foreground_; const std::vector<std::string> ids_; const UpdateClient::CrxDataCallback crx_data_callback_; - - // Called to return the execution flow back to the caller of the - // Run function when this task has completed . - Callback callback_; + const Callback callback_; DISALLOW_COPY_AND_ASSIGN(TaskUpdate); }; diff --git a/components/update_client/test_configurator.cc b/components/update_client/test_configurator.cc index 9cd59e4..a1dc0e5 100644 --- a/components/update_client/test_configurator.cc +++ b/components/update_client/test_configurator.cc @@ -4,7 +4,6 @@ #include "components/update_client/test_configurator.h" -#include "base/run_loop.h" #include "base/version.h" #include "components/update_client/component_patcher_operation.h" #include "url/gurl.h" @@ -27,8 +26,6 @@ TestConfigurator::TestConfigurator( const scoped_refptr<base::SingleThreadTaskRunner>& network_task_runner) : worker_task_runner_(worker_task_runner), initial_time_(0), - times_(1), - recheck_time_(0), ondemand_time_(0), context_(new net::TestURLRequestContextGetter(network_task_runner)) { } @@ -40,15 +37,7 @@ int TestConfigurator::InitialDelay() const { return initial_time_; } -int TestConfigurator::NextCheckDelay() { - // This is called when a new full cycle of checking for updates is going - // to happen. In test we normally only test one cycle so it is a good - // time to break from the test messageloop Run() method so the test can - // finish. - if (--times_ <= 0) { - quit_closure_.Run(); - return 0; - } +int TestConfigurator::NextCheckDelay() const { return 1; } @@ -56,14 +45,6 @@ int TestConfigurator::StepDelay() const { return 0; } -int TestConfigurator::StepDelayMedium() { - return NextCheckDelay(); -} - -int TestConfigurator::MinimumReCheckWait() const { - return recheck_time_; -} - int TestConfigurator::OnDemandDelay() const { return ondemand_time_; } @@ -122,23 +103,10 @@ bool TestConfigurator::UseBackgroundDownloader() const { return false; } -// Set how many update checks are called, the default value is just once. -void TestConfigurator::SetLoopCount(int times) { - times_ = times; -} - -void TestConfigurator::SetRecheckTime(int seconds) { - recheck_time_ = seconds; -} - void TestConfigurator::SetOnDemandTime(int seconds) { ondemand_time_ = seconds; } -void TestConfigurator::SetQuitClosure(const base::Closure& quit_closure) { - quit_closure_ = quit_closure; -} - void TestConfigurator::SetInitialDelay(int seconds) { initial_time_ = seconds; } diff --git a/components/update_client/test_configurator.h b/components/update_client/test_configurator.h index 6e441e7..8d67046 100644 --- a/components/update_client/test_configurator.h +++ b/components/update_client/test_configurator.h @@ -9,8 +9,6 @@ #include <utility> #include <vector> -#include "base/callback.h" -#include "base/compiler_specific.h" #include "base/macros.h" #include "base/memory/ref_counted.h" #include "components/update_client/configurator.h" @@ -58,10 +56,8 @@ class TestConfigurator : public Configurator { // Overrrides for Configurator. int InitialDelay() const override; - int NextCheckDelay() override; + int NextCheckDelay() const override; int StepDelay() const override; - int StepDelayMedium() override; - int MinimumReCheckWait() const override; int OnDemandDelay() const override; int UpdateDelay() const override; std::vector<GURL> UpdateUrl() const override; @@ -81,10 +77,7 @@ class TestConfigurator : public Configurator { scoped_refptr<base::SingleThreadTaskRunner> GetSingleThreadTaskRunner() const override; - void SetLoopCount(int times); - void SetRecheckTime(int seconds); void SetOnDemandTime(int seconds); - void SetQuitClosure(const base::Closure& quit_closure); void SetInitialDelay(int seconds); private: @@ -96,12 +89,9 @@ class TestConfigurator : public Configurator { scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_; int initial_time_; - int times_; - int recheck_time_; int ondemand_time_; scoped_refptr<net::TestURLRequestContextGetter> context_; - base::Closure quit_closure_; DISALLOW_COPY_AND_ASSIGN(TestConfigurator); }; diff --git a/components/update_client/update_client.cc b/components/update_client/update_client.cc index ce857e1..55b096e 100644 --- a/components/update_client/update_client.cc +++ b/components/update_client/update_client.cc @@ -38,7 +38,6 @@ namespace update_client { CrxUpdateItem::CrxUpdateItem() : state(State::kNew), - unregistered(false), on_demand(false), diff_update_failed(false), error_category(0), @@ -58,6 +57,12 @@ CrxComponent::CrxComponent() : allow_background_download(true) { CrxComponent::~CrxComponent() { } +// It is important that an instance of the UpdateClient binds an unretained +// pointer to itself. Otherwise, a life time circular dependency between this +// instance and its inner members prevents the destruction of this instance. +// Using unretained references is allowed in this case since the life time of +// the UpdateClient instance exceeds the life time of its inner members, +// including any thread objects that might execute callbacks bound to it. UpdateClientImpl::UpdateClientImpl( const scoped_refptr<Configurator>& config, scoped_ptr<PingManager> ping_manager, @@ -92,11 +97,15 @@ void UpdateClientImpl::Install(const std::string& id, std::vector<std::string> ids; ids.push_back(id); - scoped_ptr<TaskUpdate> task( - new TaskUpdate(update_engine_.get(), ids, crx_data_callback)); + // Partially applies |completion_callback| to OnTaskComplete, so this + // argument is available when the task completes, along with the task itself. + const auto callback = + base::Bind(&UpdateClientImpl::OnTaskComplete, this, completion_callback); + scoped_ptr<TaskUpdate> task(new TaskUpdate(update_engine_.get(), true, ids, + crx_data_callback, callback)); auto it = tasks_.insert(task.release()).first; - RunTask(*it, completion_callback); + RunTask(*it); } void UpdateClientImpl::Update(const std::vector<std::string>& ids, @@ -104,24 +113,23 @@ void UpdateClientImpl::Update(const std::vector<std::string>& ids, const CompletionCallback& completion_callback) { DCHECK(thread_checker_.CalledOnValidThread()); - scoped_ptr<TaskUpdate> task( - new TaskUpdate(update_engine_.get(), ids, crx_data_callback)); + const auto callback = + base::Bind(&UpdateClientImpl::OnTaskComplete, this, completion_callback); + scoped_ptr<TaskUpdate> task(new TaskUpdate(update_engine_.get(), false, ids, + crx_data_callback, callback)); + if (tasks_.empty()) { auto it = tasks_.insert(task.release()).first; - RunTask(*it, completion_callback); + RunTask(*it); } else { task_queue_.push(task.release()); } } -void UpdateClientImpl::RunTask(Task* task, - const CompletionCallback& completion_callback) { +void UpdateClientImpl::RunTask(Task* task) { DCHECK(thread_checker_.CalledOnValidThread()); base::ThreadTaskRunnerHandle::Get()->PostTask( - FROM_HERE, - base::Bind(&Task::Run, base::Unretained(task), - base::Bind(&UpdateClientImpl::OnTaskComplete, - base::Unretained(this), completion_callback))); + FROM_HERE, base::Bind(&Task::Run, base::Unretained(task))); } void UpdateClientImpl::OnTaskComplete( @@ -138,7 +146,7 @@ void UpdateClientImpl::OnTaskComplete( delete task; if (!task_queue_.empty()) { - RunTask(task_queue_.front(), completion_callback); + RunTask(task_queue_.front()); task_queue_.pop(); } } @@ -168,12 +176,11 @@ bool UpdateClientImpl::IsUpdating(const std::string& id) const { return update_engine_->IsUpdating(id); } -scoped_ptr<UpdateClient> UpdateClientFactory( +scoped_refptr<UpdateClient> UpdateClientFactory( const scoped_refptr<Configurator>& config) { scoped_ptr<PingManager> ping_manager(new PingManager(*config)); - return scoped_ptr<UpdateClient>( - new UpdateClientImpl(config, ping_manager.Pass(), &UpdateChecker::Create, - &CrxDownloader::Create)); + return new UpdateClientImpl(config, ping_manager.Pass(), + &UpdateChecker::Create, &CrxDownloader::Create); } } // namespace update_client diff --git a/components/update_client/update_client.h b/components/update_client/update_client.h index 100bbcc..29b80e0 100644 --- a/components/update_client/update_client.h +++ b/components/update_client/update_client.h @@ -200,8 +200,11 @@ struct CrxComponent { bool allow_background_download; }; -// All methods are safe to call only from the browser's main thread. -class UpdateClient { +// All methods are safe to call only from the browser's main thread. Once an +// instance of this class is created, the reference to it must be released +// only after the thread pools of the browser process have been destroyed and +// the browser process has gone single-threaded. +class UpdateClient : public base::RefCounted<UpdateClient> { public: using CrxDataCallback = base::Callback<void(const std::vector<std::string>& ids, @@ -283,12 +286,14 @@ class UpdateClient { virtual bool IsUpdating(const std::string& id) const = 0; + protected: + friend class base::RefCounted<UpdateClient>; + virtual ~UpdateClient() {} }; // Creates an instance of the update client. -// TODO(sorin): make UpdateClient a ref counted type. -scoped_ptr<UpdateClient> UpdateClientFactory( +scoped_refptr<UpdateClient> UpdateClientFactory( const scoped_refptr<Configurator>& config); } // namespace update_client diff --git a/components/update_client/update_client_internal.h b/components/update_client/update_client_internal.h index 94caf04..0ace882 100644 --- a/components/update_client/update_client_internal.h +++ b/components/update_client/update_client_internal.h @@ -37,7 +37,6 @@ class UpdateClientImpl : public UpdateClient { scoped_ptr<PingManager> ping_manager, UpdateChecker::Factory update_checker_factory, CrxDownloader::Factory crx_downloader_factory); - ~UpdateClientImpl() override; // Overrides for UpdateClient. void AddObserver(Observer* observer) override; @@ -53,7 +52,9 @@ class UpdateClientImpl : public UpdateClient { bool IsUpdating(const std::string& id) const override; private: - void RunTask(Task* task, const CompletionCallback& completion_callback); + ~UpdateClientImpl() override; + + void RunTask(Task* task); void OnTaskComplete(const CompletionCallback& completion_callback, Task* task, int error); diff --git a/components/update_client/update_client_unittest.cc b/components/update_client/update_client_unittest.cc index 52f43fa..bda4008 100644 --- a/components/update_client/update_client_unittest.cc +++ b/components/update_client/update_client_unittest.cc @@ -12,9 +12,8 @@ #include "base/message_loop/message_loop.h" #include "base/path_service.h" #include "base/run_loop.h" -#include "base/test/sequenced_worker_pool_owner.h" #include "base/thread_task_runner_handle.h" -#include "base/threading/thread.h" +#include "base/threading/sequenced_worker_pool.h" #include "base/values.h" #include "base/version.h" #include "components/update_client/crx_update_item.h" @@ -65,6 +64,31 @@ class MockObserver : public UpdateClient::Observer { MOCK_METHOD2(OnEvent, void(Events event, const std::string&)); }; +class OnDemandTester { + public: + OnDemandTester(const scoped_refptr<UpdateClient>& update_client, + bool expected_value); + + void CheckOnDemand(Events event, const std::string&); + + private: + const scoped_refptr<UpdateClient> update_client_; + const bool expected_value_; +}; + +OnDemandTester::OnDemandTester(const scoped_refptr<UpdateClient>& update_client, + bool expected_value) + : update_client_(update_client), expected_value_(expected_value) { +} + +void OnDemandTester::CheckOnDemand(Events event, const std::string& id) { + if (event == Events::COMPONENT_CHECKING_FOR_UPDATES) { + CrxUpdateItem update_item; + EXPECT_TRUE(update_client_->GetCrxUpdateState(id, &update_item)); + EXPECT_EQ(update_item.on_demand, expected_value_); + } +} + class FakePingManagerImpl : public PingManager { public: explicit FakePingManagerImpl(const Configurator& config); @@ -113,6 +137,7 @@ class UpdateClientTest : public testing::Test { protected: void RunThreads(); + void StopWorkerPool(); // Returns the full path to a test file. static base::FilePath TestFilePath(const char* file); @@ -128,8 +153,7 @@ class UpdateClientTest : public testing::Test { base::RunLoop runloop_; base::Closure quit_closure_; - scoped_ptr<base::SequencedWorkerPoolOwner> worker_pool_; - scoped_ptr<base::Thread> thread_; + scoped_refptr<base::SequencedWorkerPool> worker_pool_; scoped_refptr<update_client::Configurator> config_; @@ -137,29 +161,25 @@ class UpdateClientTest : public testing::Test { }; UpdateClientTest::UpdateClientTest() - : worker_pool_( - new base::SequencedWorkerPoolOwner(kNumWorkerThreads_, "test")), - thread_(new base::Thread("test")) { + : worker_pool_(new base::SequencedWorkerPool(kNumWorkerThreads_, "test")) { quit_closure_ = runloop_.QuitClosure(); - thread_->Start(); - - auto pool = worker_pool_->pool(); config_ = new TestConfigurator( - pool->GetSequencedTaskRunner(pool->GetSequenceToken()), - thread_->task_runner()); + worker_pool_->GetSequencedTaskRunner(worker_pool_->GetSequenceToken()), + message_loop_.task_runner()); } UpdateClientTest::~UpdateClientTest() { - config_ = nullptr; - thread_->Stop(); - worker_pool_->pool()->Shutdown(); } void UpdateClientTest::RunThreads() { runloop_.Run(); } +void UpdateClientTest::StopWorkerPool() { + worker_pool_->Shutdown(); +} + base::FilePath UpdateClientTest::TestFilePath(const char* file) { base::FilePath path; PathService::Get(base::DIR_SOURCE_ROOT, &path); @@ -237,11 +257,17 @@ TEST_F(UpdateClientTest, OneCrxNoUpdate) { }; scoped_ptr<PingManager> ping_manager(new FakePingManager(*config())); - scoped_ptr<UpdateClient> update_client(new UpdateClientImpl( + scoped_refptr<UpdateClient> update_client(new UpdateClientImpl( config(), ping_manager.Pass(), &FakeUpdateChecker::Create, &FakeCrxDownloader::Create)); + // Verify that calling Update does not set ondemand. + OnDemandTester ondemand_tester(update_client, false); + MockObserver observer; + ON_CALL(observer, OnEvent(_, _)) + .WillByDefault(Invoke(&ondemand_tester, &OnDemandTester::CheckOnDemand)); + InSequence seq; EXPECT_CALL(observer, OnEvent(Events::COMPONENT_CHECKING_FOR_UPDATES, "jebgalgnebhfojomionfpkfelancnnkf")).Times(1); @@ -260,6 +286,8 @@ TEST_F(UpdateClientTest, OneCrxNoUpdate) { RunThreads(); update_client->RemoveObserver(&observer); + + StopWorkerPool(); } // Tests the scenario where two CRXs are checked for updates. On CRX has @@ -403,7 +431,7 @@ TEST_F(UpdateClientTest, TwoCrxUpdateNoUpdate) { }; scoped_ptr<PingManager> ping_manager(new FakePingManager(*config())); - scoped_ptr<UpdateClient> update_client(new UpdateClientImpl( + scoped_refptr<UpdateClient> update_client(new UpdateClientImpl( config(), ping_manager.Pass(), &FakeUpdateChecker::Create, &FakeCrxDownloader::Create)); @@ -442,6 +470,8 @@ TEST_F(UpdateClientTest, TwoCrxUpdateNoUpdate) { RunThreads(); update_client->RemoveObserver(&observer); + + StopWorkerPool(); } // Tests the update check for two CRXs scenario. Both CRXs have updates. @@ -632,7 +662,7 @@ TEST_F(UpdateClientTest, TwoCrxUpdate) { }; scoped_ptr<FakePingManager> ping_manager(new FakePingManager(*config())); - scoped_ptr<UpdateClient> update_client(new UpdateClientImpl( + scoped_refptr<UpdateClient> update_client(new UpdateClientImpl( config(), ping_manager.Pass(), &FakeUpdateChecker::Create, &FakeCrxDownloader::Create)); @@ -679,6 +709,8 @@ TEST_F(UpdateClientTest, TwoCrxUpdate) { RunThreads(); update_client->RemoveObserver(&observer); + + StopWorkerPool(); } // Tests the differential update scenario for one CRX. @@ -893,7 +925,7 @@ TEST_F(UpdateClientTest, OneCrxDiffUpdate) { }; scoped_ptr<FakePingManager> ping_manager(new FakePingManager(*config())); - scoped_ptr<UpdateClient> update_client(new UpdateClientImpl( + scoped_refptr<UpdateClient> update_client(new UpdateClientImpl( config(), ping_manager.Pass(), &FakeUpdateChecker::Create, &FakeCrxDownloader::Create)); @@ -944,6 +976,8 @@ TEST_F(UpdateClientTest, OneCrxDiffUpdate) { } update_client->RemoveObserver(&observer); + + StopWorkerPool(); } // Tests the update scenario for one CRX where the CRX installer returns @@ -1106,7 +1140,7 @@ TEST_F(UpdateClientTest, OneCrxInstallError) { }; scoped_ptr<PingManager> ping_manager(new FakePingManager(*config())); - scoped_ptr<UpdateClient> update_client(new UpdateClientImpl( + scoped_refptr<UpdateClient> update_client(new UpdateClientImpl( config(), ping_manager.Pass(), &FakeUpdateChecker::Create, &FakeCrxDownloader::Create)); @@ -1137,6 +1171,8 @@ TEST_F(UpdateClientTest, OneCrxInstallError) { RunThreads(); update_client->RemoveObserver(&observer); + + StopWorkerPool(); } // Tests the fallback from differential to full update scenario for one CRX. @@ -1367,7 +1403,7 @@ TEST_F(UpdateClientTest, OneCrxDiffUpdateFailsFullUpdateSucceeds) { }; scoped_ptr<FakePingManager> ping_manager(new FakePingManager(*config())); - scoped_ptr<UpdateClient> update_client(new UpdateClientImpl( + scoped_refptr<UpdateClient> update_client(new UpdateClientImpl( config(), ping_manager.Pass(), &FakeUpdateChecker::Create, &FakeCrxDownloader::Create)); @@ -1421,6 +1457,289 @@ TEST_F(UpdateClientTest, OneCrxDiffUpdateFailsFullUpdateSucceeds) { } update_client->RemoveObserver(&observer); + + StopWorkerPool(); +} + +// Tests the queuing of update checks. In this scenario, two update checks are +// done for one CRX. The second update check call is queued up and will run +// after the first check has completed. The CRX has no updates. +TEST_F(UpdateClientTest, OneCrxNoUpdateQueuedCall) { + class DataCallbackFake { + public: + static void Callback(const std::vector<std::string>& ids, + std::vector<CrxComponent>* components) { + CrxComponent crx; + crx.name = "test_jebg"; + crx.pk_hash.assign(jebg_hash, jebg_hash + arraysize(jebg_hash)); + crx.version = Version("0.9"); + crx.installer = new TestInstaller; + components->push_back(crx); + } + }; + + class CompletionCallbackFake { + public: + static void Callback(const base::Closure& quit_closure, int error) { + static int num_call = 0; + ++num_call; + + EXPECT_EQ(0, error); + + if (num_call == 2) + quit_closure.Run(); + } + }; + + class FakeUpdateChecker : public UpdateChecker { + public: + static scoped_ptr<UpdateChecker> Create(const Configurator& config) { + return scoped_ptr<UpdateChecker>(new FakeUpdateChecker()); + } + + bool CheckForUpdates( + const std::vector<CrxUpdateItem*>& items_to_check, + const std::string& additional_attributes, + const UpdateCheckCallback& update_check_callback) override { + base::ThreadTaskRunnerHandle::Get()->PostTask( + FROM_HERE, base::Bind(update_check_callback, GURL(), 0, "", + UpdateResponse::Results())); + return true; + } + }; + + class FakeCrxDownloader : public CrxDownloader { + public: + static scoped_ptr<CrxDownloader> Create( + bool is_background_download, + net::URLRequestContextGetter* context_getter, + const scoped_refptr<base::SequencedTaskRunner>& url_fetcher_task_runner, + const scoped_refptr<base::SingleThreadTaskRunner>& + background_task_runner) { + return scoped_ptr<CrxDownloader>(new FakeCrxDownloader()); + } + + private: + FakeCrxDownloader() : CrxDownloader(scoped_ptr<CrxDownloader>().Pass()) {} + ~FakeCrxDownloader() override {} + + void DoStartDownload(const GURL& url) override { EXPECT_TRUE(false); } + }; + + class FakePingManager : public FakePingManagerImpl { + public: + explicit FakePingManager(const Configurator& config) + : FakePingManagerImpl(config) {} + ~FakePingManager() override { EXPECT_TRUE(items().empty()); } + }; + + scoped_ptr<PingManager> ping_manager(new FakePingManager(*config())); + scoped_refptr<UpdateClient> update_client(new UpdateClientImpl( + config(), ping_manager.Pass(), &FakeUpdateChecker::Create, + &FakeCrxDownloader::Create)); + + MockObserver observer; + InSequence seq; + EXPECT_CALL(observer, OnEvent(Events::COMPONENT_CHECKING_FOR_UPDATES, + "jebgalgnebhfojomionfpkfelancnnkf")).Times(1); + EXPECT_CALL(observer, OnEvent(Events::COMPONENT_NOT_UPDATED, + "jebgalgnebhfojomionfpkfelancnnkf")).Times(1); + EXPECT_CALL(observer, OnEvent(Events::COMPONENT_CHECKING_FOR_UPDATES, + "jebgalgnebhfojomionfpkfelancnnkf")).Times(1); + EXPECT_CALL(observer, OnEvent(Events::COMPONENT_NOT_UPDATED, + "jebgalgnebhfojomionfpkfelancnnkf")).Times(1); + + update_client->AddObserver(&observer); + + std::vector<std::string> ids; + ids.push_back(std::string("jebgalgnebhfojomionfpkfelancnnkf")); + + update_client->Update( + ids, base::Bind(&DataCallbackFake::Callback), + base::Bind(&CompletionCallbackFake::Callback, quit_closure())); + update_client->Update( + ids, base::Bind(&DataCallbackFake::Callback), + base::Bind(&CompletionCallbackFake::Callback, quit_closure())); + + RunThreads(); + + update_client->RemoveObserver(&observer); + + StopWorkerPool(); +} + +// Tests the install of one CRX. +TEST_F(UpdateClientTest, OneCrxInstall) { + class DataCallbackFake { + public: + static void Callback(const std::vector<std::string>& ids, + std::vector<CrxComponent>* components) { + CrxComponent crx; + crx.name = "test_jebg"; + crx.pk_hash.assign(jebg_hash, jebg_hash + arraysize(jebg_hash)); + crx.version = Version("0.0"); + crx.installer = new TestInstaller; + + components->push_back(crx); + } + }; + + class CompletionCallbackFake { + public: + static void Callback(const base::Closure& quit_closure, int error) { + EXPECT_EQ(0, error); + quit_closure.Run(); + } + }; + + class FakeUpdateChecker : public UpdateChecker { + public: + static scoped_ptr<UpdateChecker> Create(const Configurator& config) { + return scoped_ptr<UpdateChecker>(new FakeUpdateChecker()); + } + + bool CheckForUpdates( + const std::vector<CrxUpdateItem*>& items_to_check, + const std::string& additional_attributes, + const UpdateCheckCallback& update_check_callback) override { + /* + Fake the following response: + + <?xml version='1.0' encoding='UTF-8'?> + <response protocol='3.0'> + <app appid='jebgalgnebhfojomionfpkfelancnnkf'> + <updatecheck status='ok'> + <urls> + <url codebase='http://localhost/download/'/> + </urls> + <manifest version='1.0' prodversionmin='11.0.1.0'> + <packages> + <package name='jebgalgnebhfojomionfpkfelancnnkf.crx'/> + </packages> + </manifest> + </updatecheck> + </app> + </response> + */ + UpdateResponse::Result::Manifest::Package package; + package.name = "jebgalgnebhfojomionfpkfelancnnkf.crx"; + + UpdateResponse::Result result; + result.extension_id = "jebgalgnebhfojomionfpkfelancnnkf"; + result.crx_urls.push_back(GURL("http://localhost/download/")); + result.manifest.version = "1.0"; + result.manifest.browser_min_version = "11.0.1.0"; + result.manifest.packages.push_back(package); + + UpdateResponse::Results results; + results.list.push_back(result); + + base::ThreadTaskRunnerHandle::Get()->PostTask( + FROM_HERE, base::Bind(update_check_callback, GURL(), 0, "", results)); + return true; + } + }; + + class FakeCrxDownloader : public CrxDownloader { + public: + static scoped_ptr<CrxDownloader> Create( + bool is_background_download, + net::URLRequestContextGetter* context_getter, + const scoped_refptr<base::SequencedTaskRunner>& url_fetcher_task_runner, + const scoped_refptr<base::SingleThreadTaskRunner>& + background_task_runner) { + return scoped_ptr<CrxDownloader>(new FakeCrxDownloader()); + } + + private: + FakeCrxDownloader() : CrxDownloader(scoped_ptr<CrxDownloader>().Pass()) {} + ~FakeCrxDownloader() override {} + + void DoStartDownload(const GURL& url) override { + DownloadMetrics download_metrics; + FilePath path; + Result result; + if (url.path() == "/download/jebgalgnebhfojomionfpkfelancnnkf.crx") { + download_metrics.url = url; + download_metrics.downloader = DownloadMetrics::kNone; + download_metrics.error = 0; + download_metrics.downloaded_bytes = 1843; + download_metrics.total_bytes = 1843; + download_metrics.download_time_ms = 1000; + + EXPECT_TRUE(MakeTestFile( + TestFilePath("jebgalgnebhfojomionfpkfelancnnkf.crx"), &path)); + + result.error = 0; + result.response = path; + result.downloaded_bytes = 1843; + result.total_bytes = 1843; + } else { + NOTREACHED(); + } + + base::ThreadTaskRunnerHandle::Get()->PostTask( + FROM_HERE, base::Bind(&FakeCrxDownloader::OnDownloadProgress, + base::Unretained(this), result)); + + base::ThreadTaskRunnerHandle::Get()->PostTask( + FROM_HERE, + base::Bind(&FakeCrxDownloader::OnDownloadComplete, + base::Unretained(this), true, result, download_metrics)); + } + }; + + class FakePingManager : public FakePingManagerImpl { + public: + explicit FakePingManager(const Configurator& config) + : FakePingManagerImpl(config) {} + ~FakePingManager() override { + const auto& ping_items = items(); + EXPECT_EQ(1U, ping_items.size()); + EXPECT_EQ("jebgalgnebhfojomionfpkfelancnnkf", ping_items[0].id); + EXPECT_TRUE(base::Version("0.0").Equals(ping_items[0].previous_version)); + EXPECT_TRUE(base::Version("1.0").Equals(ping_items[0].next_version)); + EXPECT_EQ(0, ping_items[0].error_category); + EXPECT_EQ(0, ping_items[0].error_code); + } + }; + + scoped_ptr<FakePingManager> ping_manager(new FakePingManager(*config())); + scoped_refptr<UpdateClient> update_client(new UpdateClientImpl( + config(), ping_manager.Pass(), &FakeUpdateChecker::Create, + &FakeCrxDownloader::Create)); + + // Verify that calling Install sets ondemand. + OnDemandTester ondemand_tester(update_client, true); + + MockObserver observer; + ON_CALL(observer, OnEvent(_, _)) + .WillByDefault(Invoke(&ondemand_tester, &OnDemandTester::CheckOnDemand)); + + InSequence seq; + EXPECT_CALL(observer, OnEvent(Events::COMPONENT_CHECKING_FOR_UPDATES, + "jebgalgnebhfojomionfpkfelancnnkf")).Times(1); + EXPECT_CALL(observer, OnEvent(Events::COMPONENT_UPDATE_FOUND, + "jebgalgnebhfojomionfpkfelancnnkf")).Times(1); + EXPECT_CALL(observer, OnEvent(Events::COMPONENT_UPDATE_DOWNLOADING, + "jebgalgnebhfojomionfpkfelancnnkf")).Times(1); + EXPECT_CALL(observer, OnEvent(Events::COMPONENT_UPDATE_READY, + "jebgalgnebhfojomionfpkfelancnnkf")).Times(1); + EXPECT_CALL(observer, OnEvent(Events::COMPONENT_UPDATED, + "jebgalgnebhfojomionfpkfelancnnkf")).Times(1); + + update_client->AddObserver(&observer); + + update_client->Install( + std::string("jebgalgnebhfojomionfpkfelancnnkf"), + base::Bind(&DataCallbackFake::Callback), + base::Bind(&CompletionCallbackFake::Callback, quit_closure())); + + RunThreads(); + + update_client->RemoveObserver(&observer); + + StopWorkerPool(); } } // namespace update_client diff --git a/components/update_client/update_engine.cc b/components/update_client/update_engine.cc index 4c64fde..d4515d6 100644 --- a/components/update_client/update_engine.cc +++ b/components/update_client/update_engine.cc @@ -20,6 +20,7 @@ namespace update_client { UpdateContext::UpdateContext( const scoped_refptr<Configurator>& config, + bool is_foreground, const std::vector<std::string>& ids, const UpdateClient::CrxDataCallback& crx_data_callback, const UpdateEngine::NotifyObserversCallback& notify_observers_callback, @@ -28,6 +29,7 @@ UpdateContext::UpdateContext( CrxDownloader::Factory crx_downloader_factory, PingManager* ping_manager) : config(config), + is_foreground(is_foreground), ids(ids), crx_data_callback(crx_data_callback), notify_observers_callback(notify_observers_callback), @@ -93,14 +95,16 @@ bool UpdateEngine::GetUpdateState(const std::string& id, } void UpdateEngine::Update( + bool is_foreground, const std::vector<std::string>& ids, const UpdateClient::CrxDataCallback& crx_data_callback, const CompletionCallback& callback) { DCHECK(thread_checker_.CalledOnValidThread()); scoped_ptr<UpdateContext> update_context(new UpdateContext( - config_, ids, crx_data_callback, notify_observers_callback_, callback, - update_checker_factory_, crx_downloader_factory_, ping_manager_)); + config_, is_foreground, ids, crx_data_callback, + notify_observers_callback_, callback, update_checker_factory_, + crx_downloader_factory_, ping_manager_)); CrxUpdateItem update_item; scoped_ptr<ActionUpdateCheck> update_check_action(new ActionUpdateCheck( diff --git a/components/update_client/update_engine.h b/components/update_client/update_engine.h index e902a54..c5457f4 100644 --- a/components/update_client/update_engine.h +++ b/components/update_client/update_engine.h @@ -61,7 +61,8 @@ class UpdateEngine { bool GetUpdateState(const std::string& id, CrxUpdateItem* update_state); - void Update(const std::vector<std::string>& ids, + void Update(bool is_foreground, + const std::vector<std::string>& ids, const UpdateClient::CrxDataCallback& crx_data_callback, const CompletionCallback& update_callback); @@ -91,6 +92,7 @@ class UpdateEngine { struct UpdateContext { UpdateContext( const scoped_refptr<Configurator>& config, + bool is_foreground, const std::vector<std::string>& ids, const UpdateClient::CrxDataCallback& crx_data_callback, const UpdateEngine::NotifyObserversCallback& notify_observers_callback, @@ -103,6 +105,9 @@ struct UpdateContext { scoped_refptr<Configurator> config; + // True if this update has been initiated by the user. + bool is_foreground; + // Contains the ids of all CRXs in this context. const std::vector<std::string> ids; |