diff options
Diffstat (limited to 'chromium/components/update_client')
12 files changed, 136 insertions, 108 deletions
diff --git a/chromium/components/update_client/component.cc b/chromium/components/update_client/component.cc index c014a3011b0..5973357da2a 100644 --- a/chromium/components/update_client/component.cc +++ b/chromium/components/update_client/component.cc @@ -8,8 +8,8 @@ #include <utility> #include "base/bind.h" -#include "base/bind_helpers.h" #include "base/callback.h" +#include "base/callback_helpers.h" #include "base/check_op.h" #include "base/files/file_util.h" #include "base/files/scoped_temp_dir.h" @@ -386,9 +386,6 @@ void Component::SetUpdateCheckResult( if (result) SetParseResult(result.value()); - - base::SequencedTaskRunnerHandle::Get()->PostTask( - FROM_HERE, std::move(update_check_complete_)); } void Component::NotifyWait() { @@ -593,6 +590,23 @@ void Component::StateNew::DoHandle() { auto& component = State::component(); if (component.crx_component()) { TransitionState(std::make_unique<StateChecking>(&component)); + + // Notify that the component is being checked for updates after the + // transition to `StateChecking` occurs. This event indicates the start + // of the update check. The component receives the update check results when + // the update checks completes, and after that, `UpdateEngine` invokes the + // function `StateChecking::DoHandle` to transition the component out of + // the `StateChecking`. The current design allows for notifying observers + // on state transitions but it does not allow such notifications when a + // new state is entered. Hence, posting the task below is a workaround for + // this design oversight. + base::SequencedTaskRunnerHandle::Get()->PostTask( + FROM_HERE, + base::BindOnce( + [](Component& component) { + component.NotifyObservers(Events::COMPONENT_CHECKING_FOR_UPDATES); + }, + std::ref(component))); } else { component.error_code_ = static_cast<int>(Error::CRX_NOT_FOUND); component.error_category_ = ErrorCategory::kService; @@ -601,47 +615,37 @@ void Component::StateNew::DoHandle() { } Component::StateChecking::StateChecking(Component* component) - : State(component, ComponentState::kChecking) {} + : State(component, ComponentState::kChecking) { + component->last_check_ = base::TimeTicks::Now(); +} Component::StateChecking::~StateChecking() { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); } -// Unlike how other states are handled, this function does not change the -// state right away. The state transition happens when the UpdateChecker -// calls Component::UpdateCheckComplete and |update_check_complete_| is invoked. -// This is an artifact of how multiple components must be checked for updates -// together but the state machine defines the transitions for one component -// at a time. void Component::StateChecking::DoHandle() { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); auto& component = State::component(); DCHECK(component.crx_component()); - component.last_check_ = base::TimeTicks::Now(); - component.update_check_complete_ = base::BindOnce( - &Component::StateChecking::UpdateCheckComplete, base::Unretained(this)); - - component.NotifyObservers(Events::COMPONENT_CHECKING_FOR_UPDATES); -} + if (component.error_code_) { + TransitionState(std::make_unique<StateUpdateError>(&component)); + return; + } -void Component::StateChecking::UpdateCheckComplete() { - DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); - auto& component = State::component(); - if (!component.error_code_) { - if (component.status_ == "ok") { - TransitionState(std::make_unique<StateCanUpdate>(&component)); - return; - } + if (component.status_ == "ok") { + TransitionState(std::make_unique<StateCanUpdate>(&component)); + return; + } - if (component.status_ == "noupdate") { - if (component.action_run_.empty()) - TransitionState(std::make_unique<StateUpToDate>(&component)); - else - TransitionState(std::make_unique<StateRun>(&component)); - return; + if (component.status_ == "noupdate") { + if (component.action_run_.empty()) { + TransitionState(std::make_unique<StateUpToDate>(&component)); + } else { + TransitionState(std::make_unique<StateRun>(&component)); } + return; } TransitionState(std::make_unique<StateUpdateError>(&component)); diff --git a/chromium/components/update_client/component.h b/chromium/components/update_client/component.h index 1b371dad2ac..46b38607d87 100644 --- a/chromium/components/update_client/component.h +++ b/chromium/components/update_client/component.h @@ -206,8 +206,6 @@ class Component { private: // State overrides. void DoHandle() override; - - void UpdateCheckComplete(); }; class StateUpdateError : public State { @@ -489,8 +487,6 @@ class Component { std::unique_ptr<State> state_; const UpdateContext& update_context_; - base::OnceClosure update_check_complete_; - ComponentState previous_state_ = ComponentState::kLastStatus; // True if this component has reached a final state because all its states diff --git a/chromium/components/update_client/crx_downloader_unittest.cc b/chromium/components/update_client/crx_downloader_unittest.cc index b2a98b78027..5ffb3373b07 100644 --- a/chromium/components/update_client/crx_downloader_unittest.cc +++ b/chromium/components/update_client/crx_downloader_unittest.cc @@ -12,7 +12,7 @@ #include "base/memory/ref_counted.h" #include "base/path_service.h" #include "base/run_loop.h" -#include "base/test/bind_test_util.h" +#include "base/test/bind.h" #include "base/test/task_environment.h" #include "base/threading/thread_task_runner_handle.h" #include "build/build_config.h" diff --git a/chromium/components/update_client/net/url_loader_post_interceptor.cc b/chromium/components/update_client/net/url_loader_post_interceptor.cc index f6a48340963..778307197b4 100644 --- a/chromium/components/update_client/net/url_loader_post_interceptor.cc +++ b/chromium/components/update_client/net/url_loader_post_interceptor.cc @@ -9,7 +9,7 @@ #include "base/macros.h" #include "base/run_loop.h" #include "base/strings/stringprintf.h" -#include "base/test/bind_test_util.h" +#include "base/test/bind.h" #include "components/update_client/test_configurator.h" #include "net/test/embedded_test_server/embedded_test_server.h" #include "net/test/embedded_test_server/http_request.h" diff --git a/chromium/components/update_client/ping_manager_unittest.cc b/chromium/components/update_client/ping_manager_unittest.cc index 1d5994c50c3..b38196f9f74 100644 --- a/chromium/components/update_client/ping_manager_unittest.cc +++ b/chromium/components/update_client/ping_manager_unittest.cc @@ -103,7 +103,7 @@ void PingManagerTest::PingSentCallback(int error, const std::string& response) { scoped_refptr<UpdateContext> PingManagerTest::MakeMockUpdateContext() const { return base::MakeRefCounted<UpdateContext>( config_, false, std::vector<std::string>(), - UpdateClient::CrxDataCallback(), UpdateClient::CrxStateChangeCallback(), + UpdateClient::CrxStateChangeCallback(), UpdateEngine::NotifyObserversCallback(), UpdateEngine::Callback(), nullptr); } diff --git a/chromium/components/update_client/request_sender.cc b/chromium/components/update_client/request_sender.cc index 9e8fee89862..3fc7cf7aea2 100644 --- a/chromium/components/update_client/request_sender.cc +++ b/chromium/components/update_client/request_sender.cc @@ -8,7 +8,7 @@ #include "base/base64.h" #include "base/bind.h" -#include "base/bind_helpers.h" +#include "base/callback_helpers.h" #include "base/location.h" #include "base/logging.h" #include "base/numerics/safe_conversions.h" diff --git a/chromium/components/update_client/update_checker_unittest.cc b/chromium/components/update_client/update_checker_unittest.cc index 7717154d9ba..8ed5ffb688f 100644 --- a/chromium/components/update_client/update_checker_unittest.cc +++ b/chromium/components/update_client/update_checker_unittest.cc @@ -21,7 +21,7 @@ #include "base/stl_util.h" #include "base/strings/stringprintf.h" #include "base/task/post_task.h" -#include "base/test/bind_test_util.h" +#include "base/test/bind.h" #include "base/test/task_environment.h" #include "base/threading/thread_task_runner_handle.h" #include "base/version.h" @@ -227,7 +227,7 @@ void UpdateCheckerTest::UpdateCheckComplete( scoped_refptr<UpdateContext> UpdateCheckerTest::MakeMockUpdateContext() const { return base::MakeRefCounted<UpdateContext>( config_, false, std::vector<std::string>(), - UpdateClient::CrxDataCallback(), UpdateClient::CrxStateChangeCallback(), + UpdateClient::CrxStateChangeCallback(), UpdateEngine::NotifyObserversCallback(), UpdateEngine::Callback(), nullptr); } diff --git a/chromium/components/update_client/update_client.h b/chromium/components/update_client/update_client.h index be4f70211ba..f462b297243 100644 --- a/chromium/components/update_client/update_client.h +++ b/chromium/components/update_client/update_client.h @@ -331,6 +331,12 @@ using Callback = base::OnceCallback<void(Error error)>; // the browser process has gone single-threaded. class UpdateClient : public base::RefCountedThreadSafe<UpdateClient> { public: + // Returns `CrxComponent` instances corresponding to the component ids + // passed as an argument to the callback. The order of components in the input + // and output vectors must match. If the instance of the `CrxComponent` is not + // available for some reason, implementors of the callback must not skip + // skip the component, and instead, they must insert a `nullopt` value in + // the output vector. using CrxDataCallback = base::OnceCallback<std::vector<base::Optional<CrxComponent>>( const std::vector<std::string>& ids)>; diff --git a/chromium/components/update_client/update_client_errors.h b/chromium/components/update_client/update_client_errors.h index 1cf13a8c05f..61d70b20c91 100644 --- a/chromium/components/update_client/update_client_errors.h +++ b/chromium/components/update_client/update_client_errors.h @@ -18,6 +18,7 @@ enum class Error { UPDATE_CHECK_ERROR = 5, CRX_NOT_FOUND = 6, INVALID_ARGUMENT = 7, + BAD_CRX_DATA_CALLBACK = 8, MAX_VALUE, }; diff --git a/chromium/components/update_client/update_client_unittest.cc b/chromium/components/update_client/update_client_unittest.cc index 31b6a55c2b2..5907ba967e8 100644 --- a/chromium/components/update_client/update_client_unittest.cc +++ b/chromium/components/update_client/update_client_unittest.cc @@ -4587,4 +4587,60 @@ TEST_F(UpdateClientTest, CustomAttributeNoUpdate) { EXPECT_EQ(1, observer.calls); } + +// Tests the scenario where `CrxDataCallback` returns a vector whose elements +// don't include a value for one of the component ids specified by the `ids` +// parameter of the `UpdateClient::Update` function. Expects the completion +// callback to include a specific error, and no other events and pings be +// generated, since the update engine rejects the UpdateClient::Update call. +TEST_F(UpdateClientTest, BadCrxDataCallback) { + class CompletionCallbackMock { + public: + static void Callback(base::OnceClosure quit_closure, Error error) { + EXPECT_EQ(Error::BAD_CRX_DATA_CALLBACK, error); + std::move(quit_closure).Run(); + } + }; + + class MockPingManager : public MockPingManagerImpl { + public: + explicit MockPingManager(scoped_refptr<Configurator> config) + : MockPingManagerImpl(config) {} + + protected: + ~MockPingManager() override { EXPECT_TRUE(ping_data().empty()); } + }; + + scoped_refptr<UpdateClient> update_client = + base::MakeRefCounted<UpdateClientImpl>( + config(), base::MakeRefCounted<MockPingManager>(config()), + UpdateChecker::Factory{}); + + MockObserver observer; + + std::vector<CrxUpdateItem> items; + auto receiver = base::MakeRefCounted<MockCrxStateChangeReceiver>(); + EXPECT_CALL(*receiver, Receive(_)) + .WillRepeatedly( + [&items](const CrxUpdateItem& item) { items.push_back(item); }); + + update_client->AddObserver(&observer); + const std::vector<std::string> ids = {"jebgalgnebhfojomionfpkfelancnnkf", + "gjpmebpgbhcamgdgjcmnjfhggjpgcimm"}; + // The `CrxDataCallback` argument only returns a value for the first + // component id. This means that its result is ill formed, and the `Update` + // call completes with an error. + update_client->Update( + ids, base::BindOnce([](const std::vector<std::string>& ids) { + EXPECT_EQ(ids.size(), size_t{2}); + return std::vector<base::Optional<CrxComponent>>{base::nullopt}; + }), + base::BindRepeating(&MockCrxStateChangeReceiver::Receive, receiver), true, + base::BindOnce(&CompletionCallbackMock::Callback, quit_closure())); + RunThreads(); + + EXPECT_TRUE(items.empty()); + update_client->RemoveObserver(&observer); +} + } // namespace update_client diff --git a/chromium/components/update_client/update_engine.cc b/chromium/components/update_client/update_engine.cc index 57816ebc2f3..0a4c2d615ea 100644 --- a/chromium/components/update_client/update_engine.cc +++ b/chromium/components/update_client/update_engine.cc @@ -9,6 +9,7 @@ #include <utility> #include "base/bind.h" +#include "base/callback_helpers.h" #include "base/check_op.h" #include "base/guid.h" #include "base/location.h" @@ -32,7 +33,6 @@ UpdateContext::UpdateContext( scoped_refptr<Configurator> config, bool is_foreground, const std::vector<std::string>& ids, - UpdateClient::CrxDataCallback crx_data_callback, UpdateClient::CrxStateChangeCallback crx_state_change_callback, const UpdateEngine::NotifyObserversCallback& notify_observers_callback, UpdateEngine::Callback callback, @@ -41,7 +41,6 @@ UpdateContext::UpdateContext( is_foreground(is_foreground), enabled_component_updates(config->EnabledComponentUpdates()), ids(ids), - crx_data_callback(std::move(crx_data_callback)), crx_state_change_callback(crx_state_change_callback), notify_observers_callback(notify_observers_callback), callback(std::move(callback)), @@ -93,22 +92,25 @@ void UpdateEngine::Update( return; } + // Calls out to get the corresponding CrxComponent data for the components. + const std::vector<base::Optional<CrxComponent>> crx_components = + std::move(crx_data_callback).Run(ids); + if (crx_components.size() < ids.size()) { + base::ThreadTaskRunnerHandle::Get()->PostTask( + FROM_HERE, + base::BindOnce(std::move(callback), Error::BAD_CRX_DATA_CALLBACK)); + return; + } + const auto update_context = base::MakeRefCounted<UpdateContext>( - config_, is_foreground, ids, std::move(crx_data_callback), - crx_state_change_callback, notify_observers_callback_, - std::move(callback), metadata_.get()); + config_, is_foreground, ids, crx_state_change_callback, + notify_observers_callback_, std::move(callback), metadata_.get()); DCHECK(!update_context->session_id.empty()); const auto result = update_contexts_.insert( std::make_pair(update_context->session_id, update_context)); DCHECK(result.second); - // Calls out to get the corresponding CrxComponent data for the CRXs in this - // update context. - const auto crx_components = - std::move(update_context->crx_data_callback).Run(update_context->ids); - DCHECK_EQ(update_context->ids.size(), crx_components.size()); - for (size_t i = 0; i != update_context->ids.size(); ++i) { const auto& id = update_context->ids[i]; @@ -139,33 +141,6 @@ void UpdateEngine::Update( return; } - for (const auto& id : update_context->components_to_check_for_updates) - update_context->components[id]->Handle( - base::BindOnce(&UpdateEngine::ComponentCheckingForUpdatesStart, this, - update_context, id)); -} - -void UpdateEngine::ComponentCheckingForUpdatesStart( - scoped_refptr<UpdateContext> update_context, - const std::string& id) { - DCHECK(thread_checker_.CalledOnValidThread()); - DCHECK(update_context); - - DCHECK_EQ(1u, update_context->components.count(id)); - DCHECK(update_context->components.at(id)); - - // Handle |kChecking| state. - auto& component = *update_context->components.at(id); - component.Handle( - base::BindOnce(&UpdateEngine::ComponentCheckingForUpdatesComplete, this, - update_context)); - - ++update_context->num_components_ready_to_check; - if (update_context->num_components_ready_to_check < - update_context->components_to_check_for_updates.size()) { - return; - } - base::ThreadTaskRunnerHandle::Get()->PostTask( FROM_HERE, base::BindOnce(&UpdateEngine::DoUpdateCheck, this, update_context)); @@ -175,6 +150,10 @@ void UpdateEngine::DoUpdateCheck(scoped_refptr<UpdateContext> update_context) { DCHECK(thread_checker_.CalledOnValidThread()); DCHECK(update_context); + // Make the components transition from |kNew| to |kChecking| state. + for (const auto& id : update_context->components_to_check_for_updates) + update_context->components[id]->Handle(base::DoNothing()); + update_context->update_checker = update_checker_factory_(config_, metadata_.get()); @@ -221,6 +200,9 @@ void UpdateEngine::UpdateCheckResultsAvailable( component->SetUpdateCheckResult(base::nullopt, ErrorCategory::kUpdateCheck, error); } + base::ThreadTaskRunnerHandle::Get()->PostTask( + FROM_HERE, base::BindOnce(&UpdateEngine::UpdateCheckComplete, this, + update_context)); return; } @@ -261,18 +243,6 @@ void UpdateEngine::UpdateCheckResultsAvailable( static_cast<int>(ProtocolError::UPDATE_RESPONSE_NOT_FOUND)); } } -} - -void UpdateEngine::ComponentCheckingForUpdatesComplete( - scoped_refptr<UpdateContext> update_context) { - DCHECK(thread_checker_.CalledOnValidThread()); - DCHECK(update_context); - - ++update_context->num_components_checked; - if (update_context->num_components_checked < - update_context->components_to_check_for_updates.size()) { - return; - } base::ThreadTaskRunnerHandle::Get()->PostTask( FROM_HERE, @@ -284,9 +254,17 @@ void UpdateEngine::UpdateCheckComplete( DCHECK(thread_checker_.CalledOnValidThread()); DCHECK(update_context); - for (const auto& id : update_context->components_to_check_for_updates) + for (const auto& id : update_context->components_to_check_for_updates) { update_context->component_queue.push(id); + // Handle the |kChecking| state and transition the component to the + // next state, depending on the update check results. + DCHECK_EQ(1u, update_context->components.count(id)); + auto& component = update_context->components.at(id); + DCHECK_EQ(component->state(), ComponentState::kChecking); + component->Handle(base::DoNothing()); + } + base::ThreadTaskRunnerHandle::Get()->PostTask( FROM_HERE, base::BindOnce(&UpdateEngine::HandleComponent, this, update_context)); @@ -407,7 +385,7 @@ void UpdateEngine::SendUninstallPing(const std::string& id, const auto update_context = base::MakeRefCounted<UpdateContext>( config_, false, std::vector<std::string>{id}, - UpdateClient::CrxDataCallback(), UpdateClient::CrxStateChangeCallback(), + UpdateClient::CrxStateChangeCallback(), UpdateEngine::NotifyObserversCallback(), std::move(callback), metadata_.get()); DCHECK(!update_context->session_id.empty()); @@ -437,7 +415,7 @@ void UpdateEngine::SendRegistrationPing(const std::string& id, const auto update_context = base::MakeRefCounted<UpdateContext>( config_, false, std::vector<std::string>{id}, - UpdateClient::CrxDataCallback(), UpdateClient::CrxStateChangeCallback(), + UpdateClient::CrxStateChangeCallback(), UpdateEngine::NotifyObserversCallback(), std::move(callback), metadata_.get()); DCHECK(!update_context->session_id.empty()); diff --git a/chromium/components/update_client/update_engine.h b/chromium/components/update_client/update_engine.h index 6adba5d2620..a6f291cf5f0 100644 --- a/chromium/components/update_client/update_engine.h +++ b/chromium/components/update_client/update_engine.h @@ -80,13 +80,6 @@ class UpdateEngine : public base::RefCountedThreadSafe<UpdateEngine> { void UpdateComplete(scoped_refptr<UpdateContext> update_context, Error error); - void ComponentCheckingForUpdatesStart( - scoped_refptr<UpdateContext> update_context, - const std::string& id); - void ComponentCheckingForUpdatesComplete( - scoped_refptr<UpdateContext> update_context); - void UpdateCheckComplete(scoped_refptr<UpdateContext> update_context); - void DoUpdateCheck(scoped_refptr<UpdateContext> update_context); void UpdateCheckResultsAvailable( scoped_refptr<UpdateContext> update_context, @@ -94,6 +87,7 @@ class UpdateEngine : public base::RefCountedThreadSafe<UpdateEngine> { ErrorCategory error_category, int error, int retry_after_sec); + void UpdateCheckComplete(scoped_refptr<UpdateContext> update_context); void HandleComponent(scoped_refptr<UpdateContext> update_context); void HandleComponentComplete(scoped_refptr<UpdateContext> update_context); @@ -127,7 +121,6 @@ struct UpdateContext : public base::RefCountedThreadSafe<UpdateContext> { scoped_refptr<Configurator> config, bool is_foreground, const std::vector<std::string>& ids, - UpdateClient::CrxDataCallback crx_data_callback, UpdateClient::CrxStateChangeCallback crx_state_change_callback, const UpdateEngine::NotifyObserversCallback& notify_observers_callback, UpdateEngine::Callback callback, @@ -150,9 +143,6 @@ struct UpdateContext : public base::RefCountedThreadSafe<UpdateContext> { // Contains the map of ids to components for all the CRX in this context. IdToComponentPtrMap components; - // Called before an update check, when update metadata is needed. - UpdateEngine::CrxDataCallback crx_data_callback; - // Called when the observable state of the CRX component has changed. UpdateClient::CrxStateChangeCallback crx_state_change_callback; @@ -177,9 +167,6 @@ struct UpdateContext : public base::RefCountedThreadSafe<UpdateContext> { // The error reported by the update checker. int update_check_error = 0; - size_t num_components_ready_to_check = 0; - size_t num_components_checked = 0; - // Contains the ids of the components that the state machine must handle. base::queue<std::string> component_queue; |