update_engine: Pass Action ownership to ActionProcessor

Currently, an object that uses an ActionProcessor for processing one or more
actions has to own the Actions. This is problematic, because if we want to
create an action on the fly and use an ActionProcessor to perform it, we have to
own the Action until it is finished. Furthermore, if someone forget to own the
action, there will be memory leaks because ActionProcessor does not delete the
Action.

This patch passes the ownership of the Actions to the ActionProcessor through
unique pointers. If an object wants to have access to the Action, it can get it
when ActionComplete() is called.

BUG=chromium:807976
TEST=unittests
TEST=cros flash
TEST=precq

Change-Id: I28f7e9fd3425f17cc51b4db4a4abc130a7d6ef8f
Reviewed-on: https://chromium-review.googlesource.com/1065113
Commit-Ready: Amin Hassani <ahassani@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Xiaochu Liu <xiaochu@chromium.org>

1 files changed, 49 insertions, 42 deletions

diff --git a/payload_consumer/filesystem_verifier_action_unittest.cc b/payload_consumer/filesystem_verifier_action_unittest.cc
index 2e1d95de..db8b664d 100644
--- a/payload_consumer/filesystem_verifier_action_unittest.cc
+++ b/payload_consumer/filesystem_verifier_action_unittest.cc
@@ -18,8 +18,10 @@
 
 #include <fcntl.h>
 
+#include <memory>
 #include <set>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include <base/bind.h>
@@ -90,27 +92,24 @@ class FilesystemVerifierActionTestDelegate : public ActionProcessorDelegate {
     if (action->Type() == FilesystemVerifierAction::StaticType()) {
       ran_ = true;
       code_ = code;
+    } else if (action->Type() ==
+               ObjectCollectorAction<InstallPlan>::StaticType()) {
+      auto collector_action =
+          static_cast<ObjectCollectorAction<InstallPlan>*>(action);
+      install_plan_.reset(new InstallPlan(collector_action->object()));
     }
   }
   bool ran() const { return ran_; }
   ErrorCode code() const { return code_; }
 
+  std::unique_ptr<InstallPlan> install_plan_;
+
  private:
   FilesystemVerifierAction* action_;
   bool ran_;
   ErrorCode code_;
 };
 
-void StartProcessorInRunLoop(ActionProcessor* processor,
-                             FilesystemVerifierAction* filesystem_copier_action,
-                             bool terminate_early) {
-  processor->StartProcessing();
-  if (terminate_early) {
-    EXPECT_NE(nullptr, filesystem_copier_action);
-    processor->StopProcessing();
-  }
-}
-
 bool FilesystemVerifierActionTest::DoTest(bool terminate_early,
                                           bool hash_fail) {
   string a_loop_file;
@@ -165,27 +164,32 @@ bool FilesystemVerifierActionTest::DoTest(bool terminate_early,
   }
   install_plan.partitions = {part};
 
-  ActionProcessor processor;
-
-  ObjectFeederAction<InstallPlan> feeder_action;
-  FilesystemVerifierAction copier_action;
-  ObjectCollectorAction<InstallPlan> collector_action;
+  auto feeder_action = std::make_unique<ObjectFeederAction<InstallPlan>>();
+  feeder_action->set_obj(install_plan);
+  auto copier_action = std::make_unique<FilesystemVerifierAction>();
+  auto collector_action =
+      std::make_unique<ObjectCollectorAction<InstallPlan>>();
 
-  BondActions(&feeder_action, &copier_action);
-  BondActions(&copier_action, &collector_action);
+  BondActions(feeder_action.get(), copier_action.get());
+  BondActions(copier_action.get(), collector_action.get());
 
-  FilesystemVerifierActionTestDelegate delegate(&copier_action);
+  ActionProcessor processor;
+  FilesystemVerifierActionTestDelegate delegate(copier_action.get());
   processor.set_delegate(&delegate);
-  processor.EnqueueAction(&feeder_action);
-  processor.EnqueueAction(&copier_action);
-  processor.EnqueueAction(&collector_action);
-
-  feeder_action.set_obj(install_plan);
-
-  loop_.PostTask(FROM_HERE, base::Bind(&StartProcessorInRunLoop,
-                                       &processor,
-                                       &copier_action,
-                                       terminate_early));
+  processor.EnqueueAction(std::move(feeder_action));
+  processor.EnqueueAction(std::move(copier_action));
+  processor.EnqueueAction(std::move(collector_action));
+
+  loop_.PostTask(FROM_HERE,
+                 base::Bind(
+                     [](ActionProcessor* processor, bool terminate_early) {
+                       processor->StartProcessing();
+                       if (terminate_early) {
+                         processor->StopProcessing();
+                       }
+                     },
+                     base::Unretained(&processor),
+                     terminate_early));
   loop_.Run();
 
   if (!terminate_early) {
@@ -214,7 +218,7 @@ bool FilesystemVerifierActionTest::DoTest(bool terminate_early,
   EXPECT_TRUE(is_a_file_reading_eq);
   success = success && is_a_file_reading_eq;
 
-  bool is_install_plan_eq = (collector_action.object() == install_plan);
+  bool is_install_plan_eq = (*delegate.install_plan_ == install_plan);
   EXPECT_TRUE(is_install_plan_eq);
   success = success && is_install_plan_eq;
   return success;
@@ -240,13 +244,14 @@ TEST_F(FilesystemVerifierActionTest, MissingInputObjectTest) {
 
   processor.set_delegate(&delegate);
 
-  FilesystemVerifierAction copier_action;
-  ObjectCollectorAction<InstallPlan> collector_action;
+  auto copier_action = std::make_unique<FilesystemVerifierAction>();
+  auto collector_action =
+      std::make_unique<ObjectCollectorAction<InstallPlan>>();
 
-  BondActions(&copier_action, &collector_action);
+  BondActions(copier_action.get(), collector_action.get());
 
-  processor.EnqueueAction(&copier_action);
-  processor.EnqueueAction(&collector_action);
+  processor.EnqueueAction(std::move(copier_action));
+  processor.EnqueueAction(std::move(collector_action));
   processor.StartProcessing();
   EXPECT_FALSE(processor.IsRunning());
   EXPECT_TRUE(delegate.ran_);
@@ -259,7 +264,6 @@ TEST_F(FilesystemVerifierActionTest, NonExistentDriveTest) {
 
   processor.set_delegate(&delegate);
 
-  ObjectFeederAction<InstallPlan> feeder_action;
   InstallPlan install_plan;
   InstallPlan::Partition part;
   part.name = "nope";
@@ -267,15 +271,18 @@ TEST_F(FilesystemVerifierActionTest, NonExistentDriveTest) {
   part.target_path = "/no/such/file";
   install_plan.partitions = {part};
 
-  feeder_action.set_obj(install_plan);
-  FilesystemVerifierAction verifier_action;
-  ObjectCollectorAction<InstallPlan> collector_action;
+  auto feeder_action = std::make_unique<ObjectFeederAction<InstallPlan>>();
+  auto verifier_action = std::make_unique<FilesystemVerifierAction>();
+  auto collector_action =
+      std::make_unique<ObjectCollectorAction<InstallPlan>>();
+
+  feeder_action->set_obj(install_plan);
 
-  BondActions(&verifier_action, &collector_action);
+  BondActions(verifier_action.get(), collector_action.get());
 
-  processor.EnqueueAction(&feeder_action);
-  processor.EnqueueAction(&verifier_action);
-  processor.EnqueueAction(&collector_action);
+  processor.EnqueueAction(std::move(feeder_action));
+  processor.EnqueueAction(std::move(verifier_action));
+  processor.EnqueueAction(std::move(collector_action));
   processor.StartProcessing();
   EXPECT_FALSE(processor.IsRunning());
   EXPECT_TRUE(delegate.ran_);