updater: Skip candidate qualification if there is no active updater.

This requires opening global_prefs a bit earlier. The code will release
global_prefs early if it determines qualification is necessary, and will
also release it before attempting to self-uninstall (this was an
oversight from earlier).

As a side-effect of this CL, a candidate will not be able to start
qualification while the active updater is running, since it cannot open
global_prefs in that case.

I also introduced a specific server exit code for when the server shuts
down post-qualification.

Bug: 1122739
Change-Id: Iad33f88ca438aee3fa2f5f0daf1105e2712baf0f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2385795
Commit-Queue: Joshua Pawlicki <waffles@chromium.org>
Auto-Submit: Joshua Pawlicki <waffles@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803602}

chrome/updater/app/app_server.cc

@@ -25,10 +25,6 @@ void AppServer::Initialize() {
 }
 
 base::OnceClosure AppServer::ModeCheck() {
-  std::unique_ptr<LocalPrefs> local_prefs = CreateLocalPrefs();
-  if (!local_prefs->GetQualified())
-    return base::BindOnce(&AppServer::Qualify, this, std::move(local_prefs));
-
   std::unique_ptr<GlobalPrefs> global_prefs = CreateGlobalPrefs();
   if (!global_prefs) {
     return base::BindOnce(&AppServer::Shutdown, this,
@@ -37,8 +33,18 @@ base::OnceClosure AppServer::ModeCheck() {
 
   base::Version this_version(UPDATER_VERSION_STRING);
   base::Version active_version(global_prefs->GetActiveVersion());
-  if (this_version < active_version)
+  if (this_version < active_version) {
+    global_prefs = nullptr;
     return base::BindOnce(&AppServer::UninstallSelf, this);
+  }
+
+  if (active_version != base::Version("0")) {
+    std::unique_ptr<LocalPrefs> local_prefs = CreateLocalPrefs();
+    if (!local_prefs->GetQualified()) {
+      global_prefs = nullptr;
+      return base::BindOnce(&AppServer::Qualify, this, std::move(local_prefs));
+    }
+  }
 
   if (this_version > active_version || global_prefs->GetSwapping()) {
     if (!SwapVersions(global_prefs.get()))
@@ -62,7 +68,7 @@ void AppServer::Qualify(std::unique_ptr<LocalPrefs> local_prefs) {
   // For now, assume qualification succeeds.
   local_prefs->SetQualified(true);
   PrefsCommitPendingWrites(local_prefs->GetPrefService());
-  Shutdown(kErrorOk);
+  Shutdown(kErrorQualificationExit);
 }
 
 bool AppServer::SwapVersions(GlobalPrefs* global_prefs) {

chrome/updater/app/app_server_unittest.cc

@@ -11,6 +11,7 @@
 #include "base/message_loop/message_pump_type.h"
 #include "base/task/single_thread_task_executor.h"
 #include "base/task/thread_pool/thread_pool_instance.h"
+#include "chrome/updater/constants.h"
 #include "chrome/updater/prefs.h"
 #include "chrome/updater/updater_version.h"
 #include "chrome/updater/util.h"
@@ -81,13 +82,18 @@ class AppServerTestCase : public testing::Test {
 }  // namespace
 
 TEST_F(AppServerTestCase, SimpleQualify) {
+  {
+    std::unique_ptr<GlobalPrefs> global_prefs = CreateGlobalPrefs();
+    global_prefs->SetActiveVersion("0.0.0.1");
+    PrefsCommitPendingWrites(global_prefs->GetPrefService());
+  }
   auto app = base::MakeRefCounted<AppServerTest>();
 
-  // Expect the app to qualify and then Shutdown(0).
+  // Expect the app to qualify and then shutdown.
   EXPECT_CALL(*app, ActiveDuty).Times(0);
   EXPECT_CALL(*app, SwapRPCInterfaces).Times(0);
   EXPECT_CALL(*app, UninstallSelf).Times(0);
-  EXPECT_EQ(app->Run(), 0);
+  EXPECT_EQ(app->Run(), kErrorQualificationExit);
   EXPECT_TRUE(CreateLocalPrefs()->GetQualified());
 }
 
@@ -128,6 +134,21 @@ TEST_F(AppServerTestCase, SelfPromote) {
   EXPECT_EQ(global_prefs->GetActiveVersion(), UPDATER_VERSION_STRING);
 }
 
+TEST_F(AppServerTestCase, InstallAutoPromotes) {
+  auto app = base::MakeRefCounted<AppServerTest>();
+
+  // Expect the app to SwapRpcInterfaces and then ActiveDuty then Shutdown(0).
+  // In this case it bypasses qualification.
+  EXPECT_CALL(*app, ActiveDuty).Times(1);
+  EXPECT_CALL(*app, SwapRPCInterfaces).WillOnce(Return(true));
+  EXPECT_CALL(*app, UninstallSelf).Times(0);
+  EXPECT_EQ(app->Run(), 0);
+  EXPECT_FALSE(CreateLocalPrefs()->GetQualified());
+  std::unique_ptr<GlobalPrefs> global_prefs = CreateGlobalPrefs();
+  EXPECT_FALSE(global_prefs->GetSwapping());
+  EXPECT_EQ(global_prefs->GetActiveVersion(), UPDATER_VERSION_STRING);
+}
+
 TEST_F(AppServerTestCase, SelfPromoteFails) {
   {
     std::unique_ptr<LocalPrefs> local_prefs = CreateLocalPrefs();

chrome/updater/constants.h

@@ -175,9 +175,16 @@ constexpr int kErrorApplicationInstallerFailed = kCustomInstallErrorBase + 3;
 //
 // The server process may exit with any of these exit codes.
 constexpr int kErrorOk = 0;
+
+// The server could not acquire the lock needed to run.
 constexpr int kErrorFailedToLockPrefsMutex = 1;
+
+// The server candidate failed to promote itself to active.
 constexpr int kErrorFailedToSwap = 2;
 
+// The server has finished qualification and will not do further operations.
+constexpr int kErrorQualificationExit = 3;
+
 // Policy Management constants.
 extern const char kProxyModeDirect[];
 extern const char kProxyModeAutoDetect[];

chrome/updater/test/integration_tests.cc

@@ -27,9 +27,11 @@ void ExpectActiveVersion(std::string expected) {
   EXPECT_EQ(CreateGlobalPrefs()->GetActiveVersion(), expected);
 }
 
+#if defined(OS_MAC)
 void ExpectQualified() {
   EXPECT_TRUE(CreateLocalPrefs()->GetQualified());
 }
+#endif
 
 }  // namespace
 
@@ -69,7 +71,6 @@ TEST_F(IntegrationTest, InstallAndPromote) {
   ExpectActiveVersion("0");
   RunWake(0);  // Candidate qualifies and promotes to active.
 #endif
-  ExpectQualified();
   ExpectActiveVersion(UPDATER_VERSION_STRING);
   ExpectActive();
   Uninstall();