updater: Promote to active using RPC in the --install case.

Also fixes a missing init in Mac server and a threading assert in Mac
DMG installation.

Also simplifies some integration testing.

Bug: 1122739, 1063435
Change-Id: I73b30e36aa5d18cbd4618717a8b617538843f08b
Fixed: 1122739
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2410818
Commit-Queue: Joshua Pawlicki <waffles@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#807022}

chrome/updater/app/app_install.cc

@@ -11,6 +11,7 @@
 #include "base/check.h"
 #include "base/command_line.h"
 #include "base/i18n/icu_util.h"
+#include "base/logging.h"
 #include "base/task/task_traits.h"
 #include "base/task/thread_pool.h"
 #include "base/threading/sequenced_task_runner_handle.h"
@@ -21,7 +22,9 @@
 #include "chrome/updater/control_service.h"
 #include "chrome/updater/persisted_data.h"
 #include "chrome/updater/prefs.h"
+#include "chrome/updater/registration_data.h"
 #include "chrome/updater/setup.h"
+#include "chrome/updater/update_service.h"
 #include "chrome/updater/updater_version.h"
 #include "components/prefs/pref_service.h"
 
@@ -78,13 +81,6 @@ AppInstall::~AppInstall() = default;
 
 void AppInstall::Initialize() {
   base::i18n::InitializeICU();
-
-  // Creating |global_prefs_| requires acquiring a global lock, and this lock is
-  // typically owned by the RPC server. That means that if the server is
-  // running, the following code will block, and the install will not proceed
-  // until the server releases the lock.
-  global_prefs_ = CreateGlobalPrefs();
-  local_prefs_ = CreateLocalPrefs();
 }
 
 void AppInstall::FirstTaskRun() {
@@ -94,22 +90,21 @@ void AppInstall::FirstTaskRun() {
   splash_screen_ = splash_screen_maker_.Run();
   splash_screen_->Show();
 
-  base::ThreadPool::PostTaskAndReplyWithResult(
+  base::ThreadPool::PostTask(
       FROM_HERE,
       {base::MayBlock(), base::TaskPriority::USER_BLOCKING,
        base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
-      base::BindOnce([]() { return InstallCandidate(false); }),
       base::BindOnce(
-          [](SplashScreen* splash_screen, base::OnceCallback<void(int)> done,
-             int result) {
-            splash_screen->Dismiss(base::BindOnce(std::move(done), result));
-          },
-          splash_screen_.get(),
-          base::BindOnce(&AppInstall::InstallCandidateDone, this)));
+          &InstallCandidate, false, base::SequencedTaskRunnerHandle::Get(),
+          base::BindOnce(
+              [](SplashScreen* splash_screen,
+                 base::OnceCallback<void(int)> done, int result) {
+                splash_screen->Dismiss(base::BindOnce(std::move(done), result));
+              },
+              splash_screen_.get(),
+              base::BindOnce(&AppInstall::InstallCandidateDone, this))));
 }
 
-// Updates the prefs if installing the updater is successful, then continue
-// installing the application if --app-id is specified on the command line.
 void AppInstall::InstallCandidateDone(int result) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
@@ -118,50 +113,42 @@ void AppInstall::InstallCandidateDone(int result) {
     return;
   }
 
-  // Invoke |HandleAppId| to continue the execution flow.
-  MakeActive(base::BindOnce(&AppInstall::HandleAppId, this));
+  // Invoke ControlService::Run to wake this version of the updater, do an
+  // update check, and possibly promote this version as a result.
+  // The instance of |CreateControlService| has sequence affinity. Bind it
+  // in the closure to ensure it is released in this sequence.
+  scoped_refptr<ControlService> control_service = CreateControlService();
+  control_service->Run(base::BindOnce(
+      [](scoped_refptr<ControlService> /*control_service*/,
+         scoped_refptr<AppInstall> app_install) {
+        app_install->RegisterUpdater();
+      },
+      control_service, base::WrapRefCounted(this)));
 }
 
-void AppInstall::HandleAppId() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+void AppInstall::RegisterUpdater() {
+  // TODO(crbug.com/1128060): We should update the updater's registration with
+  // the new version, brand code, etc. For now, fake it.
+  RegistrationResponse result;
+  result.status_code = 0;
+  RegisterUpdaterDone(result);
+}
 
-  // This releases the prefs lock, and the RPC server can be started.
-  global_prefs_ = nullptr;
-  local_prefs_ = nullptr;
+void AppInstall::RegisterUpdaterDone(const RegistrationResponse& response) {
+  VLOG(1) << "Updater registration complete, code = " << response.status_code;
+  HandleAppId();
+}
 
-  // If no app id is provided, then invoke ControlService::Run to wake
-  // this version of the updater, do an update check, and possibly promote
-  // this version as a result.
+void AppInstall::HandleAppId() {
   const std::string app_id =
       base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(kAppIdSwitch);
   if (app_id.empty()) {
-    // The instance of |CreateControlService| has sequence affinity. Bind it
-    // in the closure to ensure it is released in this sequence.
-    scoped_refptr<ControlService> control_service = CreateControlService();
-    control_service->Run(base::BindOnce(
-        [](scoped_refptr<ControlService> /*control_service*/,
-           scoped_refptr<AppInstall> app_install) { app_install->Shutdown(0); },
-        control_service, base::WrapRefCounted(this)));
+    Shutdown(0);
     return;
   }
-
   app_install_controller_ = app_install_controller_maker_.Run();
   app_install_controller_->InstallApp(
       app_id, base::BindOnce(&AppInstall::Shutdown, this));
 }
 
-// TODO(crbug.com/1109231) - this is a temporary workaround.
-void AppInstall::MakeActive(base::OnceClosure done) {
-  local_prefs_->SetQualified(true);
-  local_prefs_->GetPrefService()->CommitPendingWrite(base::BindOnce(
-      [](base::OnceClosure done, PrefService* pref_service) {
-        DCHECK(pref_service);
-        auto persisted_data = base::MakeRefCounted<PersistedData>(pref_service);
-        persisted_data->SetProductVersion(
-            kUpdaterAppId, base::Version(UPDATER_VERSION_STRING));
-        pref_service->CommitPendingWrite(std::move(done));
-      },
-      std::move(done), global_prefs_->GetPrefService()));
-}
-
 }  // namespace updater

chrome/updater/app/app_install.h

@@ -20,13 +20,12 @@ class TaskRunner;
 
 namespace updater {
 
-class LocalPrefs;
-class GlobalPrefs;
+struct RegistrationResponse;
 
 // This class defines an interface for installing an application. The interface
 // is intended to be implemented for scenerios where UI and RPC calls to
 // |UpdateService| are involved, hence the word `controller` in the name of
-// the ]interface.
+// the interface.
 class AppInstallController
     : public base::RefCountedThreadSafe<AppInstallController> {
  public:
@@ -57,14 +56,14 @@ class AppInstall : public App {
 
   void InstallCandidateDone(int result);
 
+  void RegisterUpdater();
+
+  void RegisterUpdaterDone(const RegistrationResponse& response);
+
   // Handles the --app-id command line argument, and triggers installing of the
   // corresponding app-id if the argument is present.
   void HandleAppId();
 
-  // Makes this version of the updater active, self-registers for updates, then
-  // runs the |done| closure.
-  void MakeActive(base::OnceClosure done);
-
   // Bound to the main sequence.
   SEQUENCE_CHECKER(sequence_checker_);
 
@@ -80,14 +79,6 @@ class AppInstall : public App {
 
   scoped_refptr<AppInstallController> app_install_controller_;
 
-  // These prefs objects are used to make the updater active and register this
-  // version of the updater for self-updates.
-  //
-  // TODO(crbug.com/1109231) - this is a temporary workaround until a better
-  // fix is found.
-  std::unique_ptr<LocalPrefs> local_prefs_;
-  std::unique_ptr<GlobalPrefs> global_prefs_;
-
   scoped_refptr<base::TaskRunner> make_active_task_runner_;
 };
 

chrome/updater/app/app_register.cc

@@ -134,9 +134,11 @@ void AppRegister::RegisterAppDone(const RegistrationResponse& response) {
     return;
   }
 
-  base::ThreadPool::PostTaskAndReplyWithResult(
-      FROM_HERE, {base::MayBlock()}, base::BindOnce(&InstallCandidate, false),
-      base::BindOnce(&AppRegister::InstallCandidateDone, this));
+  base::ThreadPool::PostTask(
+      FROM_HERE, {base::MayBlock()},
+      base::BindOnce(&InstallCandidate, false,
+                     base::SequencedTaskRunnerHandle::Get(),
+                     base::BindOnce(&AppRegister::InstallCandidateDone, this)));
 }
 
 void AppRegister::InstallCandidateDone(int result) {

chrome/updater/app/app_server.cc

@@ -10,6 +10,7 @@
 #include "base/version.h"
 #include "chrome/updater/configurator.h"
 #include "chrome/updater/constants.h"
+#include "chrome/updater/persisted_data.h"
 #include "chrome/updater/prefs.h"
 #include "chrome/updater/updater_version.h"
 #include "components/prefs/pref_service.h"
@@ -78,6 +79,12 @@ bool AppServer::SwapVersions(GlobalPrefs* global_prefs) {
   if (!result)
     return false;
   global_prefs->SetActiveVersion(UPDATER_VERSION_STRING);
+  scoped_refptr<PersistedData> persisted_data =
+      base::MakeRefCounted<PersistedData>(global_prefs->GetPrefService());
+  if (!persisted_data->GetProductVersion(kUpdaterAppId).IsValid()) {
+    persisted_data->SetProductVersion(kUpdaterAppId,
+                                      base::Version(UPDATER_VERSION_STRING));
+  }
   global_prefs->SetSwapping(false);
   PrefsCommitPendingWrites(global_prefs->GetPrefService());
   return true;

chrome/updater/app/app_update.cc

@@ -8,6 +8,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/task/task_traits.h"
 #include "base/task/thread_pool.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/version.h"
 #include "chrome/updater/app/app.h"
 #include "chrome/updater/configurator.h"
@@ -41,9 +42,11 @@ void AppUpdate::Uninitialize() {
 }
 
 void AppUpdate::FirstTaskRun() {
-  base::ThreadPool::PostTaskAndReplyWithResult(
-      FROM_HERE, {base::MayBlock()}, base::BindOnce(&InstallCandidate, false),
-      base::BindOnce(&AppUpdate::SetupDone, this));
+  base::ThreadPool::PostTask(
+      FROM_HERE, {base::MayBlock()},
+      base::BindOnce(&InstallCandidate, false,
+                     base::SequencedTaskRunnerHandle::Get(),
+                     base::BindOnce(&AppUpdate::SetupDone, this)));
 }
 
 void AppUpdate::SetupDone(int result) {

chrome/updater/app/server/mac/service_delegate.mm

@@ -285,6 +285,7 @@
                  appServer:(scoped_refptr<updater::AppServerMac>)appServer {
   if (self = [super init]) {
     _service = service;
+    _appServer = appServer;
     _callbackRunner = base::SequencedTaskRunnerHandle::Get();
   }
   return self;

chrome/updater/installer.cc

@@ -64,6 +64,7 @@ update_client::CrxComponent Installer::MakeCrxComponent() {
   const auto pv = persisted_data_->GetProductVersion(app_id_);
   if (pv.IsValid()) {
     pv_ = pv;
+    checker_path_ = persisted_data_->GetExistenceCheckerPath(app_id_);
     fingerprint_ = persisted_data_->GetFingerprint(app_id_);
   } else {
     pv_ = base::Version(kNullVersion);

chrome/updater/installer.h

@@ -102,6 +102,7 @@ class Installer final : public update_client::CrxInstaller {
 
   // These members are not updated when the installer succeeds.
   base::Version pv_;
+  base::FilePath checker_path_;
   std::string fingerprint_;
 };
 

chrome/updater/installer_mac.cc

@@ -17,10 +17,8 @@ int Installer::RunApplicationInstaller(const base::FilePath& app_installer,
   DVLOG(1) << "Running application install from DMG";
   // InstallFromDMG() returns the exit code of the script. 0 is success and
   // anything else should be an error.
-  return InstallFromDMG(
-      app_installer, persisted_data_->GetExistenceCheckerPath(app_id_),
-      base::StrCat({persisted_data_->GetProductVersion(app_id_).GetString(),
-                    " ", arguments}));
+  return InstallFromDMG(app_installer, checker_path_,
+                        base::StrCat({pv_.GetString(), " ", arguments}));
 }
 
 }  // namespace updater

chrome/updater/setup.h

@@ -5,9 +5,19 @@
 #ifndef CHROME_UPDATER_SETUP_H_
 #define CHROME_UPDATER_SETUP_H_
 
+#include "base/callback_forward.h"
+#include "base/memory/ref_counted.h"
+
+namespace base {
+class TaskRunner;
+}  // namespace base
+
 namespace updater {
 
-int InstallCandidate(bool is_machine);
+// Installs the candidate, then posts |callback| to |runner|.
+void InstallCandidate(bool is_machine,
+                      scoped_refptr<base::TaskRunner> runner,
+                      base::OnceCallback<void(int)> callback);
 
 }  // namespace updater
 

chrome/updater/setup_mac.cc

@@ -5,10 +5,18 @@
 #include "chrome/updater/mac/setup/setup.h"
 #include "chrome/updater/setup.h"
 
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/task_runner.h"
+
 namespace updater {
 
-int InstallCandidate(bool is_machine) {
-  return InstallCandidate();
+void InstallCandidate(bool is_machine,
+                      scoped_refptr<base::TaskRunner> runner,
+                      base::OnceCallback<void(int)> callback) {
+  runner->PostDelayedTask(
+      FROM_HERE, base::BindOnce(std::move(callback), InstallCandidate()),
+      base::TimeDelta::FromSeconds(3));
 }
 
 }  // namespace updater

chrome/updater/setup_win.cc

@@ -5,10 +5,17 @@
 #include "chrome/updater/setup.h"
 #include "chrome/updater/win/setup/setup.h"
 
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/task_runner.h"
+
 namespace updater {
 
-int InstallCandidate(bool is_machine) {
-  return Setup(is_machine);
+void InstallCandidate(bool is_machine,
+                      scoped_refptr<base::TaskRunner> runner,
+                      base::OnceCallback<void(int)> callback) {
+  runner->PostTask(FROM_HERE,
+                   base::BindOnce(std::move(callback), Setup(is_machine)));
 }
 
 }  // namespace updater

chrome/updater/test/integration_tests.cc

@@ -55,22 +55,6 @@ class IntegrationTest : public ::testing::Test {
 TEST_F(IntegrationTest, InstallUninstall) {
   Install();
   ExpectInstalled();
-  Uninstall();
-}
-
-TEST_F(IntegrationTest, InstallAndPromote) {
-  Install();
-  ExpectInstalled();
-// TODO(crbug.com/1109231): resolve implementation inconsistencies for
-// different platforms. In this case, Windows promotes during Install, and
-// as a post-condition, the version of the updater is active before --wake
-// runs.
-#if defined(OS_WIN)
-  ExpectActiveVersion(UPDATER_VERSION_STRING);
-#else
-  ExpectActiveVersion("0");
-#endif
-  RunWake(0);  // Candidate qualifies and promotes to active.
   ExpectActiveVersion(UPDATER_VERSION_STRING);
   ExpectActive();
   Uninstall();

chrome/updater/test/integration_tests_mac.mm

@@ -128,7 +128,7 @@ void Install() {
   const base::FilePath path = GetExecutablePath();
   ASSERT_FALSE(path.empty());
   base::CommandLine command_line(path);
-  command_line.AppendSwitch(kUpdateSwitch);
+  command_line.AppendSwitch(kInstallSwitch);
   int exit_code = -1;
   ASSERT_TRUE(Run(command_line, &exit_code));
   EXPECT_EQ(0, exit_code);

chrome/updater/updater.cc

@@ -111,10 +111,10 @@ int HandleUpdaterCommands(const base::CommandLine* command_line) {
 #if defined(OS_WIN)
   if (command_line->HasSwitch(kComServiceSwitch))
     return ServiceMain::RunComService(command_line);
+#endif  // OS_WIN
 
   if (command_line->HasSwitch(kInstallSwitch))
     return MakeAppInstall()->Run();
-#endif  // OS_WIN
 
   if (command_line->HasSwitch(kUninstallSwitch))
     return MakeAppUninstall()->Run();