Replace the iterator with a reference in the update_client::UpdateEngine.

Since the UpdateContext is now a ref counted type, using the iterator
adds an unnecessary level of obfuscation.

Also, to implement task cancellation, a mechanism is needed to relate
the update task and its context, and the iterator is not appropriate
for that.

TBR: waffles
Bug: 822990
Change-Id: I8db51b24e4ef0f9b30f434820fa6d3bd1b066e98
Reviewed-on: https://chromium-review.googlesource.com/966966
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544069}

components/update_client/update_engine.cc

@@ -94,9 +94,9 @@ void UpdateEngine::Update(bool is_foreground,
 
   DCHECK(result.second);
 
-  const auto it = result.first;
-  const auto& update_context = *it;
+  const auto update_context = *result.first;
   DCHECK(update_context);
+  DCHECK(!update_context->session_id.empty());
 
   // Calls out to get the corresponding CrxComponent data for the CRXs in this
   // update context.
@@ -123,16 +123,14 @@ void UpdateEngine::Update(bool is_foreground,
     // Handle |kNew| state. This will transition the components to |kChecking|.
     component.Handle(
         base::BindOnce(&UpdateEngine::ComponentCheckingForUpdatesStart,
-                       base::Unretained(this), it, id));
+                       base::Unretained(this), update_context, id));
   }
 }
 
 void UpdateEngine::ComponentCheckingForUpdatesStart(
-    const UpdateContextIterator it,
+    scoped_refptr<UpdateContext> update_context,
     const std::string& id) {
   DCHECK(thread_checker_.CalledOnValidThread());
-
-  const auto& update_context = *it;
   DCHECK(update_context);
 
   DCHECK_EQ(1u, update_context->components.count(id));
@@ -142,7 +140,7 @@ void UpdateEngine::ComponentCheckingForUpdatesStart(
   auto& component = *update_context->components.at(id);
   component.Handle(
       base::BindOnce(&UpdateEngine::ComponentCheckingForUpdatesComplete,
-                     base::Unretained(this), it));
+                     base::Unretained(this), update_context));
 
   ++update_context->num_components_ready_to_check;
   if (update_context->num_components_ready_to_check <
@@ -151,14 +149,12 @@ void UpdateEngine::ComponentCheckingForUpdatesStart(
   }
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::BindOnce(&UpdateEngine::DoUpdateCheck, base::Unretained(this), it));
+      FROM_HERE, base::BindOnce(&UpdateEngine::DoUpdateCheck,
+                                base::Unretained(this), update_context));
 }
 
-void UpdateEngine::DoUpdateCheck(const UpdateContextIterator it) {
+void UpdateEngine::DoUpdateCheck(scoped_refptr<UpdateContext> update_context) {
   DCHECK(thread_checker_.CalledOnValidThread());
-
-  const auto& update_context = *it;
   DCHECK(update_context);
 
   update_context->update_checker =
@@ -169,15 +165,13 @@ void UpdateEngine::DoUpdateCheck(const UpdateContextIterator it) {
       update_context->components, config_->ExtraRequestParams(),
       update_context->enabled_component_updates,
       base::BindOnce(&UpdateEngine::UpdateCheckDone, base::Unretained(this),
-                     it));
+                     update_context));
 }
 
-void UpdateEngine::UpdateCheckDone(const UpdateContextIterator it,
+void UpdateEngine::UpdateCheckDone(scoped_refptr<UpdateContext> update_context,
                                    int error,
                                    int retry_after_sec) {
   DCHECK(thread_checker_.CalledOnValidThread());
-
-  const auto& update_context = *it;
   DCHECK(update_context);
 
   update_context->retry_after_sec = retry_after_sec;
@@ -207,10 +201,8 @@ void UpdateEngine::UpdateCheckDone(const UpdateContextIterator it,
 }
 
 void UpdateEngine::ComponentCheckingForUpdatesComplete(
-    const UpdateContextIterator it) {
+    scoped_refptr<UpdateContext> update_context) {
   DCHECK(thread_checker_.CalledOnValidThread());
-
-  const auto& update_context = *it;
   DCHECK(update_context);
 
   ++update_context->num_components_checked;
@@ -220,13 +212,12 @@ void UpdateEngine::ComponentCheckingForUpdatesComplete(
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(&UpdateEngine::UpdateCheckComplete,
-                                base::Unretained(this), it));
+                                base::Unretained(this), update_context));
 }
 
-void UpdateEngine::UpdateCheckComplete(const UpdateContextIterator it) {
+void UpdateEngine::UpdateCheckComplete(
+    scoped_refptr<UpdateContext> update_context) {
   DCHECK(thread_checker_.CalledOnValidThread());
-
-  const auto& update_context = *it;
   DCHECK(update_context);
 
   for (const auto& id : update_context->ids)
@@ -234,13 +225,12 @@ void UpdateEngine::UpdateCheckComplete(const UpdateContextIterator it) {
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(&UpdateEngine::HandleComponent,
-                                base::Unretained(this), it));
+                                base::Unretained(this), update_context));
 }
 
-void UpdateEngine::HandleComponent(const UpdateContextIterator it) {
+void UpdateEngine::HandleComponent(
+    scoped_refptr<UpdateContext> update_context) {
   DCHECK(thread_checker_.CalledOnValidThread());
-
-  const auto& update_context = *it;
   DCHECK(update_context);
 
   auto& queue = update_context->component_queue;
@@ -251,8 +241,9 @@ void UpdateEngine::HandleComponent(const UpdateContextIterator it) {
                             : Error::NONE;
 
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(&UpdateEngine::UpdateComplete,
-                                  base::Unretained(this), it, error));
+        FROM_HERE,
+        base::BindOnce(&UpdateEngine::UpdateComplete, base::Unretained(this),
+                       update_context, error));
     return;
   }
 
@@ -261,12 +252,12 @@ void UpdateEngine::HandleComponent(const UpdateContextIterator it) {
   const auto& component = update_context->components.at(id);
   DCHECK(component);
 
-  auto& next_update_delay = (*it)->next_update_delay;
+  auto& next_update_delay = update_context->next_update_delay;
   if (!next_update_delay.is_zero() && component->IsUpdateAvailable()) {
     base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
         FROM_HERE,
         base::BindOnce(&UpdateEngine::HandleComponent, base::Unretained(this),
-                       it),
+                       update_context),
         next_update_delay);
     next_update_delay = base::TimeDelta();
 
@@ -276,13 +267,12 @@ void UpdateEngine::HandleComponent(const UpdateContextIterator it) {
   }
 
   component->Handle(base::BindOnce(&UpdateEngine::HandleComponentComplete,
-                                   base::Unretained(this), it));
+                                   base::Unretained(this), update_context));
 }
 
-void UpdateEngine::HandleComponentComplete(const UpdateContextIterator it) {
+void UpdateEngine::HandleComponentComplete(
+    scoped_refptr<UpdateContext> update_context) {
   DCHECK(thread_checker_.CalledOnValidThread());
-
-  const auto& update_context = *it;
   DCHECK(update_context);
 
   auto& queue = update_context->component_queue;
@@ -306,18 +296,23 @@ void UpdateEngine::HandleComponentComplete(const UpdateContextIterator it) {
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(&UpdateEngine::HandleComponent,
-                                base::Unretained(this), it));
+                                base::Unretained(this), update_context));
 }
 
-void UpdateEngine::UpdateComplete(const UpdateContextIterator it, Error error) {
+void UpdateEngine::UpdateComplete(scoped_refptr<UpdateContext> update_context,
+                                  Error error) {
   DCHECK(thread_checker_.CalledOnValidThread());
-
-  const auto& update_context = *it;
   DCHECK(update_context);
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(std::move(update_context->callback), error));
 
+  const auto it =
+      std::find_if(update_contexts_.begin(), update_contexts_.end(),
+                   [update_context](scoped_refptr<UpdateContext> other) {
+                     return update_context->session_id == other->session_id;
+                   });
+  DCHECK(it != update_contexts_.end());
   update_contexts_.erase(it);
 }
 
@@ -362,9 +357,7 @@ void UpdateEngine::SendUninstallPing(const std::string& id,
           nullptr));
 
   DCHECK(result.second);
-
-  const auto it = result.first;
-  const auto& update_context = *it;
+  const auto update_context = *result.first;
   DCHECK(update_context);
   DCHECK_EQ(1u, update_context->ids.size());
   DCHECK_EQ(1u, update_context->components.count(id));
@@ -376,7 +369,7 @@ void UpdateEngine::SendUninstallPing(const std::string& id,
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(&UpdateEngine::HandleComponent,
-                                base::Unretained(this), it));
+                                base::Unretained(this), update_context));
 }
 
 }  // namespace update_client

components/update_client/update_engine.h

@@ -5,7 +5,6 @@
 #ifndef COMPONENTS_UPDATE_CLIENT_UPDATE_ENGINE_H_
 #define COMPONENTS_UPDATE_CLIENT_UPDATE_ENGINE_H_
 
-#include <iterator>
 #include <list>
 #include <map>
 #include <memory>
@@ -69,22 +68,23 @@ class UpdateEngine : public base::RefCounted<UpdateEngine> {
   ~UpdateEngine();
 
   using UpdateContexts = std::set<scoped_refptr<UpdateContext>>;
-  using UpdateContextIterator = UpdateContexts::iterator;
 
-  void UpdateComplete(UpdateContextIterator it, Error error);
+  void UpdateComplete(scoped_refptr<UpdateContext> update_context, Error error);
 
-  void ComponentCheckingForUpdatesStart(UpdateContextIterator it,
-                                        const std::string& id);
-  void ComponentCheckingForUpdatesComplete(UpdateContextIterator it);
-  void UpdateCheckComplete(UpdateContextIterator it);
+  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(UpdateContextIterator it);
-  void UpdateCheckDone(UpdateContextIterator it,
+  void DoUpdateCheck(scoped_refptr<UpdateContext> update_context);
+  void UpdateCheckDone(scoped_refptr<UpdateContext> update_context,
                        int error,
                        int retry_after_sec);
 
-  void HandleComponent(UpdateContextIterator it);
-  void HandleComponentComplete(UpdateContextIterator it);
+  void HandleComponent(scoped_refptr<UpdateContext> update_context);
+  void HandleComponentComplete(scoped_refptr<UpdateContext> update_context);
 
   // Returns true if the update engine rejects this update call because it
   // occurs too soon.