Updater: Test Wake behavior if a newer version is active.

Bug: 1138014
Change-Id: I7e9e73bc1c0d8ce1361ac5469f8e68ae31a4b90f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2443540
Commit-Queue: Mila Green <milagreen@chromium.org>
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#817760}

chrome/updater/BUILD.gn

@@ -194,6 +194,7 @@ if (is_win || is_mac) {
         "//chrome/updater/mac:installer_sources",
         "//chrome/updater/mac:network_fetcher_sources",
         "//chrome/updater/mac:updater_setup_sources",
+        "//chrome/updater/mac:util",
         "//chrome/updater/mac:xpc_names",
       ]
     }
@@ -327,6 +328,7 @@ if (is_win || is_mac) {
         "//chrome/updater/mac:updater_bundle",
         "//chrome/updater/mac:updater_setup_tests",
         "//chrome/updater/mac:updater_tests",
+        "//chrome/updater/mac:util",
         "//chrome/updater/mac:xpc_names",
         "//third_party/ocmock",
       ]

chrome/updater/app/app_wake.cc

@@ -5,7 +5,6 @@
 #include "chrome/updater/app/app_wake.h"
 
 #include "base/bind.h"
-#include "base/logging.h"
 #include "build/build_config.h"
 #include "chrome/updater/app/app.h"
 #include "chrome/updater/control_service.h"

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

@@ -92,7 +92,9 @@ void AppServerMac::ActiveDuty() {
 }
 
 void AppServerMac::UninstallSelf() {
-  UninstallCandidate();
+  base::ThreadPool::PostTaskAndReplyWithResult(
+      FROM_HERE, {base::MayBlock()}, base::BindOnce(&UninstallCandidate),
+      base::BindOnce(&AppServerMac::Shutdown, this));
 }
 
 bool AppServerMac::SwapRPCInterfaces() {

chrome/updater/mac/BUILD.gn

@@ -78,6 +78,20 @@ source_set("xpc_names") {
   frameworks = [ "Foundation.framework" ]
 }
 
+source_set("util") {
+  sources = [
+    "util.h",
+    "util.mm",
+  ]
+
+  deps = [
+    "//base",
+    "//chrome/updater:version_header",
+  ]
+
+  frameworks = [ "Foundation.framework" ]
+}
+
 source_set("updater_setup_sources") {
   sources = [
     "setup/setup.h",
@@ -86,6 +100,7 @@ source_set("updater_setup_sources") {
 
   deps = [
     ":network_fetcher_sources",
+    ":util",
     ":xpc_names",
     "//base",
     "//chrome/common/mac:launchd",

chrome/updater/mac/setup/setup.mm

@@ -27,6 +27,7 @@
 #include "chrome/updater/constants.h"
 #include "chrome/updater/crash_client.h"
 #include "chrome/updater/crash_reporter.h"
+#import "chrome/updater/mac/util.h"
 #import "chrome/updater/mac/xpc_service_names.h"
 #include "chrome/updater/updater_version.h"
 #include "chrome/updater/util.h"
@@ -39,11 +40,6 @@ namespace {
 constexpr char kLoggingModuleSwitchValue[] = "*/updater/*=2";
 
 #pragma mark Helpers
-const base::FilePath GetUpdateFolderName() {
-  return base::FilePath(COMPANY_SHORTNAME_STRING)
-      .AppendASCII(PRODUCT_FULLNAME_STRING);
-}
-
 const base::FilePath GetUpdaterAppName() {
   return base::FilePath(PRODUCT_FULLNAME_STRING ".app");
 }
@@ -52,38 +48,6 @@ const base::FilePath GetUpdaterAppExecutablePath() {
   return base::FilePath("Contents/MacOS").AppendASCII(PRODUCT_FULLNAME_STRING);
 }
 
-bool IsSystemInstall() {
-  return geteuid() == 0;
-}
-
-const base::FilePath GetLibraryFolderPath() {
-  // For user installations: the "~/Library" for the logged in user.
-  // For system installations: "/Library".
-  if (IsSystemInstall()) {
-    base::FilePath local_library_path;
-    if (!base::mac::GetLocalDirectory(NSLibraryDirectory,
-                                      &local_library_path)) {
-      VLOG(1) << "Could not get local library path";
-    }
-    return local_library_path;
-  }
-  return base::mac::GetUserLibraryPath();
-}
-
-const base::FilePath GetUpdaterFolderPath() {
-  // For user installations:
-  // ~/Library/COMPANY_SHORTNAME_STRING/PRODUCT_FULLNAME_STRING.
-  // e.g. ~/Library/Google/GoogleUpdater
-  // For system installations:
-  // /Library/COMPANY_SHORTNAME_STRING/PRODUCT_FULLNAME_STRING.
-  // e.g. /Library/Google/GoogleUpdater
-  return GetLibraryFolderPath().Append(GetUpdateFolderName());
-}
-
-const base::FilePath GetVersionedUpdaterFolderPath() {
-  return GetUpdaterFolderPath().AppendASCII(UPDATER_VERSION_STRING);
-}
-
 const base::FilePath GetUpdaterExecutablePath(
     const base::FilePath& updater_folder_path) {
   return updater_folder_path.Append(GetUpdaterAppName())
@@ -139,6 +103,7 @@ base::ScopedCFTypeRef<CFDictionaryRef> CreateServiceLaunchdPlist(
       MakeProgramArgument(kServerSwitch),
       MakeProgramArgumentWithValue(kServerServiceSwitch,
                                    kServerUpdateServiceSwitchValue),
+      MakeProgramArgument(kEnableLoggingSwitch),
       MakeProgramArgumentWithValue(kLoggingModuleSwitch,
                                    kLoggingModuleSwitchValue),
     ],
@@ -159,7 +124,7 @@ base::ScopedCFTypeRef<CFDictionaryRef> CreateWakeLaunchdPlist(
       [NSMutableArray<NSString*> array];
   [program_arguments addObjectsFromArray:@[
     base::SysUTF8ToNSString(updater_path.value()),
-    MakeProgramArgument(kWakeSwitch)
+    MakeProgramArgument(kWakeSwitch), MakeProgramArgument(kEnableLoggingSwitch)
   ]];
   if (IsSystemInstall())
     [program_arguments addObject:MakeProgramArgument(kSystemSwitch)];
@@ -187,6 +152,7 @@ base::ScopedCFTypeRef<CFDictionaryRef> CreateControlLaunchdPlist(
       MakeProgramArgument(kServerSwitch),
       MakeProgramArgumentWithValue(kServerServiceSwitch,
                                    kServerControlServiceSwitchValue),
+      MakeProgramArgument(kEnableLoggingSwitch),
       MakeProgramArgumentWithValue(kLoggingModuleSwitch,
                                    kLoggingModuleSwitchValue),
     ],
@@ -362,15 +328,19 @@ int PromoteCandidate() {
 
 #pragma mark Uninstall
 int UninstallCandidate() {
+  if (!DeleteCandidateInstallFolder())
+    return setup_exit_codes::kFailedToDeleteFolder;
+
   if (!RemoveUpdateWakeJobFromLaunchd())
     return setup_exit_codes::kFailedToRemoveWakeJobFromLaunchd;
 
+  // Removing the Control job has to be the last step because launchd is likely
+  // to terminate the current process. Clients should expect the connection to
+  // invalidate (possibly with an interruption beforehand) as a result of
+  // service uninstallation.
   if (!RemoveUpdateControlJobFromLaunchd())
     return setup_exit_codes::kFailedToRemoveControlJobFromLaunchd;
 
-  if (!DeleteCandidateInstallFolder())
-    return setup_exit_codes::kFailedToDeleteFolder;
-
   return setup_exit_codes::kSuccess;
 }
 

chrome/updater/mac/util.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_UPDATER_MAC_UTIL_H_
+#define CHROME_UPDATER_MAC_UTIL_H_
+
+namespace base {
+class FilePath;
+class Version;
+}  // namespace base
+
+namespace updater {
+
+bool IsSystemInstall();
+
+// For user installations returns: the "~/Library" for the logged in user.
+// For system installations returns: "/Library".
+base::FilePath GetLibraryFolderPath();
+
+// For user installations:
+// ~/Library/Google/GoogleUpdater
+// For system installations:
+// /Library/Google/GoogleUpdater
+base::FilePath GetUpdaterFolderPath();
+
+// For user installations:
+// ~/Library/Google/GoogleUpdater/88.0.4293.0
+// For system installations:
+// /Library/Google/GoogleUpdater/88.0.4293.0
+base::FilePath GetVersionedUpdaterFolderPathForVersion(
+    const base::Version& version);
+
+// The same as GetVersionedUpdaterFolderPathForVersion, where the version is
+// UPDATER_VERSION_STRING.
+base::FilePath GetVersionedUpdaterFolderPath();
+
+// For user installations:
+// ~/Library/Google/GoogleUpdater/88.0.4293.0/GoogleUpdater.app/Contents/MacOS/GoogleUpdater
+// For system installations:
+// /Library/Google/GoogleUpdater/88.0.4293.0/GoogleUpdater.app/Contents/MacOS/GoogleUpdater
+base::FilePath GetUpdaterExecutablePath();
+
+// For user installations:
+// ~/Library/Google/GoogleUpdater/88.0.4293.0/GoogleUpdater.app/Contents/MacOS
+// For system installations:
+// /Library/Google/GoogleUpdater/88.0.4293.0/GoogleUpdater.app/Contents/MacOS
+base::FilePath GetExecutableFolderPathForVersion(const base::Version& version);
+
+}  // namespace updater
+
+#endif  // CHROME_UPDATER_MAC_UTIL_H_

chrome/updater/mac/util.mm

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#import "chrome/updater/mac/util.h"
+
+#include <unistd.h>
+
+#include "base/files/file_path.h"
+#include "base/logging.h"
+#include "base/mac/foundation_util.h"
+#include "base/version.h"
+#include "chrome/updater/updater_version.h"
+
+namespace updater {
+namespace {
+
+base::FilePath GetUpdateFolderName() {
+  return base::FilePath(COMPANY_SHORTNAME_STRING)
+      .AppendASCII(PRODUCT_FULLNAME_STRING);
+}
+
+base::FilePath ExecutableFolderPath() {
+  return base::FilePath(FILE_PATH_LITERAL(PRODUCT_FULLNAME_STRING ".app"))
+      .Append(FILE_PATH_LITERAL("Contents"))
+      .Append(FILE_PATH_LITERAL("MacOS"));
+}
+
+}  // namespace
+
+bool IsSystemInstall() {
+  return geteuid() == 0;
+}
+
+base::FilePath GetLibraryFolderPath() {
+  if (!IsSystemInstall())
+    return base::mac::GetUserLibraryPath();
+
+  base::FilePath local_library_path;
+  if (base::mac::GetLocalDirectory(NSLibraryDirectory, &local_library_path)) {
+    return local_library_path;
+  }
+  VLOG(1) << "Could not get local library path";
+  return base::FilePath();
+}
+
+base::FilePath GetUpdaterFolderPath() {
+  return GetLibraryFolderPath().Append(GetUpdateFolderName());
+}
+
+base::FilePath GetVersionedUpdaterFolderPathForVersion(
+    const base::Version& version) {
+  return GetUpdaterFolderPath().AppendASCII(version.GetString());
+}
+
+base::FilePath GetVersionedUpdaterFolderPath() {
+  return GetUpdaterFolderPath().AppendASCII(UPDATER_VERSION_STRING);
+}
+
+base::FilePath GetExecutableFolderPathForVersion(const base::Version& version) {
+  return GetVersionedUpdaterFolderPathForVersion(version).Append(
+      ExecutableFolderPath());
+}
+
+base::FilePath GetUpdaterExecutablePath() {
+  return GetVersionedUpdaterFolderPath()
+      .Append(ExecutableFolderPath())
+      .Append(FILE_PATH_LITERAL(PRODUCT_FULLNAME_STRING));
+}
+
+}  // namespace updater

chrome/updater/test/integration_tests.cc

@@ -4,9 +4,15 @@
 
 #include "chrome/updater/test/integration_tests.h"
 
+#include <cstdlib>
+
+#include "base/command_line.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
 #include "base/test/task_environment.h"
 #include "base/version.h"
 #include "build/build_config.h"
+#include "chrome/updater/constants.h"
 #include "chrome/updater/persisted_data.h"
 #include "chrome/updater/prefs.h"
 #include "chrome/updater/test/test_app/constants.h"
@@ -33,6 +39,57 @@ void ExpectActiveVersion(std::string expected) {
 
 }  // namespace
 
+#if defined(OS_MAC)
+// TODO(crbug.com/1138014): These functions should be enabled on Win too.
+void RunWake(int expected_exit_code) {
+  const base::FilePath installed_executable_path = GetInstalledExecutablePath();
+  EXPECT_TRUE(base::PathExists(installed_executable_path));
+  base::CommandLine command_line(installed_executable_path);
+  command_line.AppendSwitch(kWakeSwitch);
+  command_line.AppendSwitch(kEnableLoggingSwitch);
+  command_line.AppendSwitchASCII(kLoggingModuleSwitch, "*/updater/*=2");
+  int exit_code = -1;
+  ASSERT_TRUE(Run(command_line, &exit_code));
+  EXPECT_EQ(exit_code, expected_exit_code);
+}
+
+void SetupFakeUpdaterPrefs(const base::Version& version) {
+  std::unique_ptr<GlobalPrefs> global_prefs = CreateGlobalPrefs();
+  global_prefs->SetActiveVersion(version.GetString());
+  global_prefs->SetSwapping(false);
+  PrefsCommitPendingWrites(global_prefs->GetPrefService());
+
+  ASSERT_EQ(version.GetString(), global_prefs->GetActiveVersion());
+}
+
+void SetupFakeUpdaterInstallFolder(const base::Version& version) {
+  const base::FilePath folder_path = GetFakeUpdaterInstallFolderPath(version);
+  ASSERT_TRUE(base::CreateDirectory(folder_path));
+}
+
+void SetupFakeUpdater(const base::Version& version) {
+  SetupFakeUpdaterPrefs(version);
+  SetupFakeUpdaterInstallFolder(version);
+}
+
+void SetupFakeUpdaterVersion(int offset) {
+  ASSERT_TRUE(offset != 0);
+  base::Version self_version = base::Version(UPDATER_VERSION_STRING);
+  std::vector<uint32_t> components = self_version.components();
+  ASSERT_FALSE(offset < 0 && components[0] <= uint32_t{abs(offset)});
+  components[0] += offset;
+  SetupFakeUpdater(base::Version(components));
+}
+
+void SetupFakeUpdaterLowerVersion() {
+  SetupFakeUpdaterVersion(-1);
+}
+
+void SetupFakeUpdaterHigherVersion() {
+  SetupFakeUpdaterVersion(1);
+}
+#endif  // OS_MAC
+
 class IntegrationTest : public ::testing::Test {
  protected:
   void SetUp() override {
@@ -59,6 +116,17 @@ TEST_F(IntegrationTest, InstallUninstall) {
 }
 
 #if defined(OS_MAC)
+TEST_F(IntegrationTest, SelfUninstallOutdatedUpdater) {
+  Install();
+  ExpectInstalled();
+  SetupFakeUpdaterHigherVersion();
+  EXPECT_NE(CreateGlobalPrefs()->GetActiveVersion(), UPDATER_VERSION_STRING);
+
+  RunWake(0);
+  ExpectCandidateUninstalled();
+  Uninstall();
+}
+
 TEST_F(IntegrationTest, RegisterTestApp) {
   RegisterTestApp();
   ExpectInstalled();
@@ -66,7 +134,7 @@ TEST_F(IntegrationTest, RegisterTestApp) {
   ExpectActive();
   Uninstall();
 }
-#endif
+#endif  // OS_MAC
 
 #endif  // defined(OS_WIN) || !defined(COMPONENT_BUILD)
 

chrome/updater/test/integration_tests.h

@@ -5,6 +5,12 @@
 #ifndef CHROME_UPDATER_TEST_INTEGRATION_TESTS_H_
 #define CHROME_UPDATER_TEST_INTEGRATION_TESTS_H_
 
+namespace base {
+class CommandLine;
+class FilePath;
+class Version;
+}  // namespace base
+
 namespace updater {
 
 namespace test {
@@ -45,6 +51,31 @@ void RunWake(int exit_code);
 // promoted and registered.
 void RegisterTestApp();
 
+// Runs the command and waits for it to exit or time out.
+bool Run(base::CommandLine command_line, int* exit_code);
+
+// Returns the path of the Updater executable.
+base::FilePath GetInstalledExecutablePath();
+
+// Returns the folder path under which the executable for the fake updater
+// should reside.
+base::FilePath GetFakeUpdaterInstallFolderPath(const base::Version& version);
+
+// Creates Prefs with the fake updater version set as active.
+void SetupFakeUpdaterPrefs(const base::Version& version);
+
+// Creates an install folder on the system with the fake updater version.
+void SetupFakeUpdaterInstallFolder(const base::Version& version);
+
+// Sets up a fake updater on the system at a version lower than the test.
+void SetupFakeUpdaterLowerVersion();
+
+// Sets up a fake updater on the system at a version higher than the test.
+void SetupFakeUpdaterHigherVersion();
+
+// Expects that this version of updater is uninstalled from the system.
+void ExpectCandidateUninstalled();
+
 }  // namespace test
 
 }  // namespace updater

chrome/updater/test/integration_tests_mac.mm

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <stdint.h>
+
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
@@ -9,9 +11,12 @@
 #include "base/path_service.h"
 #include "base/process/launch.h"
 #include "base/process/process.h"
+#include "base/version.h"
 #include "chrome/common/mac/launchd.h"
 #include "chrome/updater/constants.h"
+#import "chrome/updater/mac/util.h"
 #include "chrome/updater/mac/xpc_service_names.h"
+#include "chrome/updater/prefs.h"
 #include "chrome/updater/test/test_app/constants.h"
 #include "chrome/updater/test/test_app/test_app_version.h"
 #include "chrome/updater/updater_version.h"
@@ -29,7 +34,7 @@ base::FilePath GetExecutablePath() {
   if (!base::PathService::Get(base::FILE_EXE, &test_executable))
     return base::FilePath();
   return test_executable.DirName()
-      .Append(FILE_PATH_LITERAL(PRODUCT_FULLNAME_STRING ".App"))
+      .Append(FILE_PATH_LITERAL(PRODUCT_FULLNAME_STRING ".app"))
       .Append(FILE_PATH_LITERAL("Contents"))
       .Append(FILE_PATH_LITERAL("MacOS"))
       .Append(FILE_PATH_LITERAL(PRODUCT_FULLNAME_STRING));
@@ -40,7 +45,7 @@ base::FilePath GetTestAppExecutablePath() {
   if (!base::PathService::Get(base::FILE_EXE, &test_executable))
     return base::FilePath();
   return test_executable.DirName()
-      .Append(FILE_PATH_LITERAL(TEST_APP_FULLNAME_STRING ".App"))
+      .Append(FILE_PATH_LITERAL(TEST_APP_FULLNAME_STRING ".app"))
       .Append(FILE_PATH_LITERAL("Contents"))
       .Append(FILE_PATH_LITERAL("MacOS"))
       .Append(FILE_PATH_LITERAL(TEST_APP_FULLNAME_STRING));
@@ -59,6 +64,8 @@ base::FilePath GetDataDirPath() {
       .AppendASCII(PRODUCT_FULLNAME_STRING);
 }
 
+}  // namespace
+
 bool Run(base::CommandLine command_line, int* exit_code) {
   auto process = base::LaunchProcess(command_line, {});
   if (!process.IsValid())
@@ -69,8 +76,6 @@ bool Run(base::CommandLine command_line, int* exit_code) {
   return true;
 }
 
-}  // namespace
-
 void Clean() {
   EXPECT_TRUE(base::DeletePathRecursively(GetProductPath()));
   EXPECT_TRUE(Launchd::GetInstance()->DeletePlist(
@@ -151,14 +156,17 @@ void RegisterTestApp() {
   EXPECT_EQ(0, exit_code);
 }
 
-void RunWake(int expected_exit_code) {
-  const base::FilePath path = GetExecutablePath();
-  ASSERT_FALSE(path.empty());
-  base::CommandLine command_line(path);
-  command_line.AppendSwitch(kWakeSwitch);
-  int exit_code = -1;
-  ASSERT_TRUE(Run(command_line, &exit_code));
-  EXPECT_EQ(exit_code, expected_exit_code);
+base::FilePath GetInstalledExecutablePath() {
+  return GetUpdaterExecutablePath();
+}
+
+void ExpectCandidateUninstalled() {
+  base::FilePath versioned_folder_path = GetVersionedUpdaterFolderPath();
+  EXPECT_FALSE(base::PathExists(versioned_folder_path));
+  EXPECT_FALSE(Launchd::GetInstance()->PlistExists(
+      Launchd::User, Launchd::Agent, CopyWakeLaunchdName()));
+  EXPECT_FALSE(Launchd::GetInstance()->PlistExists(
+      Launchd::User, Launchd::Agent, CopyControlLaunchdName()));
 }
 
 void Uninstall() {
@@ -171,6 +179,10 @@ void Uninstall() {
   EXPECT_EQ(0, exit_code);
 }
 
+base::FilePath GetFakeUpdaterInstallFolderPath(const base::Version& version) {
+  return GetExecutableFolderPathForVersion(version);
+}
+
 }  // namespace test
 
 }  // namespace updater