Simplify handling of Component::StateChecking.

This seems to work and no change in the unit tests was needed
after removing the clutter of how StateChecking::DoHandle works.

High level, the state transitions kNew->kChecking and kChecking->whatever
don't need to count callbacks. Instead, the code relies on the sequence
provided by the task runner.

This streamlines the execution path of UpdateEngine::DoUpdateCheck since
it makes clear that it can't be invoked multiple times during
a single call to UpdateEngine::Update.

Bug: 1146864
Change-Id: I116e0f68b6668856aa3c3564ed4f51bf7efd4026
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2523788Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#825456}

components/update_client/component.cc

@@ -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_ == "noupdate") {
-      if (component.action_run_.empty())
+    if (component.action_run_.empty()) {
       TransitionState(std::make_unique<StateUpToDate>(&component));
-      else
+    } else {
       TransitionState(std::make_unique<StateRun>(&component));
-      return;
     }
+    return;
   }
 
   TransitionState(std::make_unique<StateUpdateError>(&component));

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

components/update_client/update_engine.cc

@@ -9,6 +9,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/check_op.h"
 #include "base/guid.h"
 #include "base/location.h"
@@ -140,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));
@@ -176,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());
 
@@ -222,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;
   }
 
@@ -262,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,
@@ -285,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));

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);
@@ -173,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;