Receive is_enterprise_rollback from update_engine

The update engine sends a flag for enterprise rollback.
Instead of guessing whether the current update is an enterprise
rollback using channels and versions, we use the rollback flag together
with the powerwash flag to determine which kind of update is happening.

Bug: 864672
Change-Id: Ia9a97b2bfe8abf5d795a2b530689321b9d585a0d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2215058Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Miriam Polzer <mpolzer@google.com>
Cr-Commit-Position: refs/heads/master@{#772579}

chrome/browser/upgrade_detector/installed_version_updater_chromeos.cc

@@ -43,71 +43,33 @@ void InstalledVersionUpdater::UpdateStatusChanged(
     return;
   }
 
-  // Cancel a potential channel request from a previous status update.
-  weak_ptr_factory_.InvalidateWeakPtrs();
-  current_channel_.reset();
-  target_channel_.reset();
-
-  if (status.is_enterprise_rollback()) {
-    // Powerwash will be required. Determine what kind of notification to show
-    // based on the current and target channels.
-    // StatusResult::new_version is the new Chrome OS system version number.
-    new_version_ = status.new_version();
-    auto* client = chromeos::DBusThreadManager::Get()->GetUpdateEngineClient();
-    client->GetChannel(/*get_current_channel=*/false,
-                       base::BindOnce(&InstalledVersionUpdater::OnChannel,
-                                      weak_ptr_factory_.GetWeakPtr(), false));
-    client->GetChannel(/*get_current_channel=*/true,
-                       base::BindOnce(&InstalledVersionUpdater::OnChannel,
-                                      weak_ptr_factory_.GetWeakPtr(), true));
-  } else {
-    // Not going to an earlier version; no powerwash or rollback message is
-    // required.
-    new_version_.reset();
-    build_state_->SetUpdate(BuildState::UpdateType::kNormalUpdate,
-                            base::Version(status.new_version()), base::nullopt);
-  }
-}
+  BuildState::UpdateType update_type = BuildState::UpdateType::kNormalUpdate;
 
-void InstalledVersionUpdater::OnChannel(bool is_current_channel,
-                                        const std::string& channel_name) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  if (status.will_powerwash_after_reboot()) {
+    // Powerwash will be required, this can be triggered by an enterprise
+    // rollback or by the user switching to a more stable channel. Determine
+    // what kind of notification to show based on the enterprise rollback flag.
 
-  // Preserve this channel name.
-  (is_current_channel ? current_channel_ : target_channel_) = channel_name;
+    if (status.is_enterprise_rollback()) {
+      update_type = BuildState::UpdateType::kEnterpriseRollback;
 
-  // Nothing else to do if still waiting on the other value.
-  if (!current_channel_.has_value() || !target_channel_.has_value())
-    return;
+      base::UmaHistogramEnumeration("UpgradeDetecter.RollbackReason",
+                                    RollbackReason::kEnterpriseRollback);
 
-  // Set the UpdateType based on whether or not the device is moving to a more
-  // stable channel.
-  // TODO(crbug.com/864672): Use is_rollback from the update engine so that
-  // admin-driven channel changes appear as enterprise rollbacks rather than
-  // channel switches.
-  BuildState::UpdateType update_type =
-      chromeos::UpdateEngineClient::IsTargetChannelMoreStable(
-          current_channel_.value(), target_channel_.value())
-          ? BuildState::UpdateType::kChannelSwitchRollback
-          : BuildState::UpdateType::kEnterpriseRollback;
-
-  base::UmaHistogramEnumeration(
-      "UpgradeDetector.RollbackReason",
-      update_type == BuildState::UpdateType::kChannelSwitchRollback
-          ? RollbackReason::kToMoreStableChannel
-          : RollbackReason::kEnterpriseRollback);
-
-  LOG(WARNING) << "Device is rolling back, will require powerwash. Reason: "
-               << (update_type ==
-                   BuildState::UpdateType::kChannelSwitchRollback)
-               << ", current_channel: " << current_channel_.value()
-               << ", target_channel: " << target_channel_.value();
-
-  build_state_->SetUpdate(update_type, base::Version(new_version_.value()),
-                          base::nullopt);
+      LOG(WARNING) << "Device is rolling back, will require powerwash. Reason:"
+                   << " Enterprise rollback.";
 
-  // All done; clear state.
-  new_version_.reset();
-  current_channel_.reset();
-  target_channel_.reset();
+    } else {
+      // Powerwash must have been triggered by channel change.
+      update_type = BuildState::UpdateType::kChannelSwitchRollback;
+
+      base::UmaHistogramEnumeration("UpgradeDetecter.RollbackReason",
+                                    RollbackReason::kToMoreStableChannel);
+
+      LOG(WARNING) << "Device is rolling back, will require powerwash. Reason:"
+                   << " Channel switch.";
+    }
+  }
+  build_state_->SetUpdate(update_type, base::Version(status.new_version()),
+                          base::nullopt);
 }

chrome/browser/upgrade_detector/installed_version_updater_chromeos.h

@@ -33,21 +33,6 @@ class InstalledVersionUpdater : public chromeos::UpdateEngineClient::Observer {
 
   SEQUENCE_CHECKER(sequence_checker_);
   BuildState* const build_state_;
-
-  // Holds the current channel following an enterprise rollback while waiting
-  // for the target channel to be retrieved.
-  base::Optional<std::string> current_channel_;
-
-  // Holds the target channel following an enterprise rollback while waiting for
-  // the current channel to be retrieved.
-  base::Optional<std::string> target_channel_;
-
-  // Holds a new version received from the update engine client following an
-  // enterprise rollback while the current and target channels are being
-  // retrieved.
-  base::Optional<std::string> new_version_;
-
-  base::WeakPtrFactory<InstalledVersionUpdater> weak_ptr_factory_{this};
 };
 
 #endif  // CHROME_BROWSER_UPGRADE_DETECTOR_INSTALLED_VERSION_UPDATER_CHROMEOS_H_

chrome/browser/upgrade_detector/installed_version_updater_chromeos_unittest.cc

@@ -95,6 +95,7 @@ TEST_F(InstalledVersionUpdaterTest, Rollback) {
 
   status.set_current_operation(update_engine::UPDATED_NEED_REBOOT);
   status.set_new_version(new_version);
+  status.set_will_powerwash_after_reboot(true);
   status.set_is_enterprise_rollback(true);
   EXPECT_CALL(
       mock_observer_,
@@ -107,5 +108,29 @@ TEST_F(InstalledVersionUpdaterTest, Rollback) {
                                   base::Version(new_version)))),
                      Property(&BuildState::critical_version, IsFalse()))));
   NotifyStatusChanged(std::move(status));
+}
+
+// Tests that a channel change notifies the BuildState appropriately.
+TEST_F(InstalledVersionUpdaterTest, ChannelChange) {
+  InstalledVersionUpdater installed_version_updater(&build_state_);
+  update_engine::StatusResult status;
+  const std::string new_version("1.2.3");
+
+  status.set_current_operation(update_engine::UPDATED_NEED_REBOOT);
+  status.set_new_version(new_version);
+  status.set_will_powerwash_after_reboot(true);
+  status.set_is_enterprise_rollback(false);
+  EXPECT_CALL(
+      mock_observer_,
+      OnUpdate(AllOf(
+          Eq(&build_state_),
+          Property(&BuildState::update_type,
+                   Eq(BuildState::UpdateType::kChannelSwitchRollback)),
+          Property(&BuildState::installed_version, IsTrue()),
+          Property(
+              &BuildState::installed_version,
+              Eq(base::Optional<base::Version>(base::Version(new_version)))),
+          Property(&BuildState::critical_version, IsFalse()))));
+  NotifyStatusChanged(std::move(status));
   task_environment_.RunUntilIdle();
 }

chromeos/dbus/update_engine_client.cc

@@ -351,17 +351,6 @@ class UpdateEngineClientImpl : public UpdateEngineClient {
       return;
     }
 
-    // TODO(https://crbug.com/864672): Use GetStatus to determine this based on
-    // the Omaha response, and not version comparison.
-    const std::string current_version =
-        version_loader::GetVersion(version_loader::VERSION_SHORT);
-    status.set_is_enterprise_rollback(
-        version_loader::IsRollback(current_version, status.new_version()));
-    if (status.is_enterprise_rollback()) {
-      LOG(WARNING) << "New image is a rollback from " << current_version
-                   << " to " << status.new_version() << ".";
-    }
-
     last_status_ = status;
     for (auto& observer : observers_)
       observer.UpdateStatusChanged(status);
@@ -471,17 +460,6 @@ class UpdateEngineClientImpl : public UpdateEngineClient {
       return;
     }
 
-    // TODO(https://crbug.com/864672): Use GetStatus to determine this based on
-    // the Omaha response, and not version comparison.
-    const std::string current_version =
-        version_loader::GetVersion(version_loader::VERSION_SHORT);
-    status.set_is_enterprise_rollback(
-        version_loader::IsRollback(current_version, status.new_version()));
-    if (status.is_enterprise_rollback()) {
-      LOG(WARNING) << "New image is a rollback from " << current_version
-                   << " to " << status.new_version() << ".";
-    }
-
     last_status_ = status;
     for (auto& observer : observers_)
       observer.UpdateStatusChanged(status);