commit 2b73ac21602d96ebb25801f06c33be0f3ef33dc9
author Jae Hoon Kim <kimjae@chromium.org> Tue Jul 02 11:17:24 2019 -0700
committer Jae Hoon Kim <kimjae@chromium.org> Tue Jul 16 21:46:58 2019 +0000
tree d474c265d4c208efd2cde33b51fc1ee26d00e1fd
parent 75daa3822680b04882ce13d4803ee78f9c3f365f

update_engine: Force Update + Install uniform logic pattern

The change here is to make the exit points within |CheckForUpdate()| and
|CheckForInstall()| uniform with simpler logic.

When a force update or install is called, it is heavily dependent on the
|forced_update_pending_callback_| and the existence of a scheduled
update in the message loop. The |forced_update_pending_callback_| is the
one that notifies a change hence calling upon |OnUpdateScheduled()| that
was waiting on the message loop.

The call to |ScheduleUpdates()| before forcing the callback is to
guarantee that the |forced_update_pending_callback_| will take effect.

Even if the |forced_update_pending_callback_| is not set, it is not a
failure. Simply, the periodic check will use the set forced variables
when it is scheduled.

BUG=none
TEST=FEATURES="test" emerge-${BOARD} update_engine

Change-Id: Ic97671a70880c8b05d513a8dea93fce3b982bbf6

update_attempter.cc

diff --git a/update_attempter.cc b/update_attempter.cc
index 62a999c..cf588e5 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -872,16 +872,17 @@
     // |OnUpdateScheduled()|.
   }
 
+  // |forced_update_pending_callback_| should always be set, but even in the
+  // case that it is not, we still return true indicating success because the
+  // scheduled periodic check will pick up these changes.
   if (forced_update_pending_callback_.get()) {
-    if (!system_state_->dlcservice()->GetInstalled(&dlc_module_ids_)) {
-      dlc_module_ids_.clear();
-    }
-    // Make sure that a scheduling request is made prior to calling the forced
-    // update pending callback.
+    // Always call |ScheduleUpdates()| before forcing an update. This is because
+    // we need an update to be scheduled for the
+    // |forced_update_pending_callback_| to have an effect. Here we don't need
+    // to care about the return value from |ScheduleUpdate()|.
     ScheduleUpdates();
     forced_update_pending_callback_->Run(true, interactive);
   }
-
   return true;
 }
 
@@ -903,15 +904,16 @@
     forced_omaha_url_ = constants::kOmahaDefaultAUTestURL;
   }
 
-  if (!ScheduleUpdates()) {
-    if (forced_update_pending_callback_.get()) {
-      // Make sure that a scheduling request is made prior to calling the forced
-      // update pending callback.
-      ScheduleUpdates();
-      forced_update_pending_callback_->Run(true, true);
-      return true;
-    }
-    return false;
+  // |forced_update_pending_callback_| should always be set, but even in the
+  // case that it is not, we still return true indicating success because the
+  // scheduled periodic check will pick up these changes.
+  if (forced_update_pending_callback_.get()) {
+    // Always call |ScheduleUpdates()| before forcing an update. This is because
+    // we need an update to be scheduled for the
+    // |forced_update_pending_callback_| to have an effect. Here we don't need
+    // to care about the return value from |ScheduleUpdate()|.
+    ScheduleUpdates();
+    forced_update_pending_callback_->Run(true, true);
   }
   return true;
 }

update_attempter_unittest.cc

diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index 203d704..b7d0997 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -100,6 +100,7 @@
   // Expects:
   string expected_forced_app_version = "";
   string expected_forced_omaha_url = "";
+  bool should_schedule_updates_be_called = true;
   bool expected_result = true;
 };
 
@@ -332,6 +333,8 @@
             attempter_.forced_app_version());
   EXPECT_EQ(cfu_params_.expected_forced_omaha_url,
             attempter_.forced_omaha_url());
+  EXPECT_EQ(cfu_params_.should_schedule_updates_be_called,
+            attempter_.WasScheduleUpdatesCalled());
 }
 
 void UpdateAttempterTest::ScheduleQuitMainLoop() {
@@ -1391,6 +1394,8 @@
   cfu_params_.status = UpdateStatus::CHECKING_FOR_UPDATE;
   // GIVEN a interactive update.
 
+  // THEN |ScheduleUpdates()| should not be called.
+  cfu_params_.should_schedule_updates_be_called = false;
   // THEN result should indicate failure.
   cfu_params_.expected_result = false;
 
@@ -1564,6 +1569,34 @@
   TestCheckForUpdate();
 }
 
+TEST_F(UpdateAttempterTest, CheckForUpdateMissingForcedCallback1) {
+  // GIVEN a official build.
+  // GIVEN forced callback is not set.
+  attempter_.set_forced_update_pending_callback(nullptr);
+
+  // THEN we except forced app version + forced omaha url to be cleared.
+  // THEN |ScheduleUpdates()| should not be called.
+  cfu_params_.should_schedule_updates_be_called = false;
+
+  TestCheckForUpdate();
+}
+
+TEST_F(UpdateAttempterTest, CheckForUpdateMissingForcedCallback2) {
+  // GIVEN a non offical build with dev features enabled.
+  cfu_params_.is_official_build = false;
+  cfu_params_.are_dev_features_enabled = true;
+  // GIVEN forced callback is not set.
+  attempter_.set_forced_update_pending_callback(nullptr);
+
+  // THEN the forced app version + forced omaha url changes based on input.
+  cfu_params_.expected_forced_app_version = cfu_params_.app_version;
+  cfu_params_.expected_forced_omaha_url = cfu_params_.omaha_url;
+  // THEN |ScheduleUpdates()| should not be called.
+  cfu_params_.should_schedule_updates_be_called = false;
+
+  TestCheckForUpdate();
+}
+
 TEST_F(UpdateAttempterTest, CheckForInstallTest) {
   fake_system_state_.fake_hardware()->SetIsOfficialBuild(true);
   fake_system_state_.fake_hardware()->SetAreDevFeaturesEnabled(false);