Fix Handoff from Omaha to O4 fails

The updater scope was not taking the legacy command line format into
account, now fixed by determining the scope using the legacy-compatible
command line.

Added integration tests.

Bug: 1379746
Change-Id: I42386daf3276156ab34e6ac17094b0b6439a8898
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4111693
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Commit-Queue: S Ganesh <ganesh@chromium.org>
Auto-Submit: S Ganesh <ganesh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1084071}

chrome/updater/test/integration_test_commands.h

@@ -46,6 +46,11 @@ class IntegrationTestCommands
                                     const std::string& install_data_index,
                                     const base::Version& from_version,
                                     const base::Version& to_version) const = 0;
+  virtual void ExpectInstallSequence(ScopedServer* test_server,
+                                     const std::string& app_id,
+                                     const std::string& install_data_index,
+                                     const base::Version& from_version,
+                                     const base::Version& to_version) const = 0;
   virtual void ExpectVersionActive(const std::string& version) const = 0;
   virtual void ExpectVersionNotActive(const std::string& version) const = 0;
   virtual void Uninstall() const = 0;
@@ -90,6 +95,7 @@ class IntegrationTestCommands
   virtual void RunUninstallCmdLine() const = 0;
   virtual void SetUpTestService() const = 0;
   virtual void TearDownTestService() const = 0;
+  virtual void RunHandoff(const std::string& app_id) const = 0;
 #endif  // BUILDFLAG(IS_WIN)
   virtual void StressUpdateService() const = 0;
   virtual void CallServiceUpdate(const std::string& app_id,

chrome/updater/test/integration_test_commands_system.cc

@@ -102,6 +102,16 @@ class IntegrationTestCommandsSystem : public IntegrationTestCommands {
                                         to_version);
   }
 
+  void ExpectInstallSequence(ScopedServer* test_server,
+                             const std::string& app_id,
+                             const std::string& install_data_index,
+                             const base::Version& from_version,
+                             const base::Version& to_version) const override {
+    updater::test::ExpectInstallSequence(updater_scope_, test_server, app_id,
+                                         install_data_index, from_version,
+                                         to_version);
+  }
+
   void ExpectVersionActive(const std::string& version) const override {
     RunCommand("expect_version_active", {Param("version", version)});
   }
@@ -251,6 +261,10 @@ class IntegrationTestCommandsSystem : public IntegrationTestCommands {
   void TearDownTestService() const override {
     updater::test::RunTestServiceCommand("teardown");
   }
+
+  void RunHandoff(const std::string& app_id) const override {
+    RunCommand("run_handoff", {Param("app_id", app_id)});
+  }
 #endif  // BUILDFLAG(IS_WIN)
 
   base::FilePath GetDifferentUserPath() const override {

chrome/updater/test/integration_test_commands_user.cc

@@ -83,6 +83,16 @@ class IntegrationTestCommandsUser : public IntegrationTestCommands {
                                         to_version);
   }
 
+  void ExpectInstallSequence(ScopedServer* test_server,
+                             const std::string& app_id,
+                             const std::string& install_data_index,
+                             const base::Version& from_version,
+                             const base::Version& to_version) const override {
+    updater::test::ExpectInstallSequence(updater_scope_, test_server, app_id,
+                                         install_data_index, from_version,
+                                         to_version);
+  }
+
   void ExpectVersionActive(const std::string& version) const override {
     updater::test::ExpectVersionActive(updater_scope_, version);
   }
@@ -218,6 +228,10 @@ class IntegrationTestCommandsUser : public IntegrationTestCommands {
   void SetUpTestService() const override {}
 
   void TearDownTestService() const override {}
+
+  void RunHandoff(const std::string& app_id) const override {
+    updater::test::RunHandoff(updater_scope_, app_id);
+  }
 #endif  // BUILDFLAG(IS_WIN)
 
   base::FilePath GetDifferentUserPath() const override {

chrome/updater/test/integration_tests.cc

@@ -194,6 +194,10 @@ class IntegrationTest : public ::testing::Test {
   }
 
   void RunUninstallCmdLine() { test_commands_->RunUninstallCmdLine(); }
+
+  void RunHandoff(const std::string& app_id) {
+    test_commands_->RunHandoff(app_id);
+  }
 #endif  // BUILDFLAG(IS_WIN)
 
   void SetupFakeUpdaterHigherVersion() {
@@ -308,14 +312,13 @@ class IntegrationTest : public ::testing::Test {
     test_commands_->ExpectSelfUpdateSequence(test_server);
   }
 
-  void ExpectInstallEvent(ScopedServer* test_server,
-                          const std::string& app_id) {
-    test_server->ExpectOnce(
-        {base::BindRepeating(
-            RequestMatcherRegex,
-            base::StrCat({R"(.*"appid":")", app_id, R"(","enabled":true,")",
-                          R"(event":\[{"eventresult":1,"eventtype":2,.*)"}))},
-        "");
+  void ExpectInstallSequence(ScopedServer* test_server,
+                             const std::string& app_id,
+                             const std::string& install_data_index,
+                             const base::Version& from_version,
+                             const base::Version& to_version) {
+    test_commands_->ExpectInstallSequence(
+        test_server, app_id, install_data_index, from_version, to_version);
   }
 
   void StressUpdateService() { test_commands_->StressUpdateService(); }
@@ -561,6 +564,21 @@ TEST_F(IntegrationTest, UpdateApp) {
 }
 
 #if BUILDFLAG(IS_WIN)
+TEST_F(IntegrationTest, Handoff) {
+  ScopedServer test_server(test_commands_);
+  ASSERT_NO_FATAL_FAILURE(Install());
+
+  const std::string kAppId("test");
+  const base::Version v1("1");
+  ExpectInstallSequence(&test_server, kAppId, "", base::Version({0, 0, 0, 0}),
+                        v1);
+  RunHandoff(kAppId);
+  EXPECT_TRUE(WaitForUpdaterExit());
+  ExpectAppVersion(kAppId, v1);
+
+  Uninstall();
+}
+
 TEST_F(IntegrationTest, ForceInstallApp) {
   ScopedServer test_server(test_commands_);
   ASSERT_NO_FATAL_FAILURE(Install());

chrome/updater/test/integration_tests_helper.cc

@@ -273,6 +273,7 @@ void AppTestHelper::FirstTaskRun() {
     {"expect_legacy_policy_status_succeeds",
      WithSystemScope(Wrap(&ExpectLegacyPolicyStatusSucceeds))},
     {"run_uninstall_cmd_line", WithSystemScope(Wrap(&RunUninstallCmdLine))},
+    {"run_handoff", WithSwitch("app_id", WithSystemScope(Wrap(&RunHandoff)))},
 #endif  // BUILDFLAG(IS_WIN)
     {"expect_version_active",
      WithSwitch("version", WithSystemScope(Wrap(&ExpectVersionActive)))},

chrome/updater/test/integration_tests_impl.cc

@@ -194,6 +194,57 @@ void RunUpdaterWithSwitch(const base::Version& version,
   EXPECT_EQ(exit_code, expected_exit_code);
 }
 
+void ExpectSequence(UpdaterScope scope,
+                    ScopedServer* test_server,
+                    const std::string& app_id,
+                    const std::string& install_data_index,
+                    int event_type,
+                    const base::Version& from_version,
+                    const base::Version& to_version) {
+  base::FilePath test_data_path;
+  ASSERT_TRUE(base::PathService::Get(chrome::DIR_TEST_DATA, &test_data_path));
+  base::FilePath crx_path = test_data_path.Append(FILE_PATH_LITERAL("updater"))
+                                .AppendASCII(kDoNothingCRXName);
+  ASSERT_TRUE(base::PathExists(crx_path));
+
+  // First request: update check.
+  test_server->ExpectOnce(
+      {base::BindRepeating(
+           RequestMatcherRegex,
+           base::StringPrintf(R"(.*"appid":"%s".*)", app_id.c_str())),
+       base::BindRepeating(
+           RequestMatcherRegex,
+           base::StringPrintf(
+               R"(.*%s)",
+               !install_data_index.empty()
+                   ? base::StringPrintf(
+                         R"("data":\[{"index":"%s","name":"install"}],.*)",
+                         install_data_index.c_str())
+                         .c_str()
+                   : "")),
+       GetScopePredicate(scope)},
+      GetUpdateResponse(app_id, install_data_index,
+                        test_server->base_url().spec(), to_version, crx_path,
+                        kDoNothingCRXRun, {}));
+
+  // Second request: update download.
+  std::string crx_bytes;
+  base::ReadFileToString(crx_path, &crx_bytes);
+  test_server->ExpectOnce({base::BindRepeating(RequestMatcherRegex, "")},
+                          crx_bytes);
+
+  // Third request: event ping.
+  test_server->ExpectOnce(
+      {base::BindRepeating(
+           RequestMatcherRegex,
+           base::StringPrintf(R"(.*"eventresult":1,"eventtype":%d,)"
+                              R"("nextversion":"%s","previousversion":"%s".*)",
+                              event_type, to_version.GetString().c_str(),
+                              from_version.GetString().c_str())),
+       GetScopePredicate(scope)},
+      ")]}'\n");
+}
+
 }  // namespace
 
 void ExitTestMode(UpdaterScope scope) {
@@ -544,48 +595,18 @@ void ExpectUpdateSequence(UpdaterScope scope,
                           const std::string& install_data_index,
                           const base::Version& from_version,
                           const base::Version& to_version) {
-  base::FilePath test_data_path;
-  ASSERT_TRUE(base::PathService::Get(chrome::DIR_TEST_DATA, &test_data_path));
-  base::FilePath crx_path = test_data_path.Append(FILE_PATH_LITERAL("updater"))
-                                .AppendASCII(kDoNothingCRXName);
-  ASSERT_TRUE(base::PathExists(crx_path));
+  ExpectSequence(scope, test_server, app_id, install_data_index, 3,
+                 from_version, to_version);
+}
 
-  // First request: update check.
-  test_server->ExpectOnce(
-      {base::BindRepeating(
-           RequestMatcherRegex,
-           base::StringPrintf(R"(.*"appid":"%s".*)", app_id.c_str())),
-       base::BindRepeating(
-           RequestMatcherRegex,
-           base::StringPrintf(
-               R"(.*%s)",
-               !install_data_index.empty()
-                   ? base::StringPrintf(
-                         R"("data":\[{"index":"%s","name":"install"}],.*)",
-                         install_data_index.c_str())
-                         .c_str()
-                   : "")),
-       GetScopePredicate(scope)},
-      GetUpdateResponse(app_id, install_data_index,
-                        test_server->base_url().spec(), to_version, crx_path,
-                        kDoNothingCRXRun, {}));
-
-  // Second request: update download.
-  std::string crx_bytes;
-  base::ReadFileToString(crx_path, &crx_bytes);
-  test_server->ExpectOnce({base::BindRepeating(RequestMatcherRegex, "")},
-                          crx_bytes);
-
-  // Third request: event ping.
-  test_server->ExpectOnce(
-      {base::BindRepeating(
-           RequestMatcherRegex,
-           base::StringPrintf(R"(.*"eventresult":1,"eventtype":3,)"
-                              R"("nextversion":"%s","previousversion":"%s".*)",
-                              to_version.GetString().c_str(),
-                              from_version.GetString().c_str())),
-       GetScopePredicate(scope)},
-      ")]}'\n");
+void ExpectInstallSequence(UpdaterScope scope,
+                           ScopedServer* test_server,
+                           const std::string& app_id,
+                           const std::string& install_data_index,
+                           const base::Version& from_version,
+                           const base::Version& to_version) {
+  ExpectSequence(scope, test_server, app_id, install_data_index, 2,
+                 from_version, to_version);
 }
 
 // Runs multiple cycles of instantiating the update service, calling

chrome/updater/test/integration_tests_impl.h

@@ -215,6 +215,7 @@ void InvokeTestServiceFunction(const std::string& function_name,
                                const base::Value::Dict& arguments);
 
 void RunUninstallCmdLine(UpdaterScope scope);
+void RunHandoff(UpdaterScope scope, const std::string& app_id);
 #endif  // BUILDFLAG(IS_WIN)
 
 // Returns the number of files in the directory, not including directories,
@@ -234,6 +235,13 @@ void ExpectUpdateSequence(UpdaterScope scope,
                           const base::Version& from_version,
                           const base::Version& to_version);
 
+void ExpectInstallSequence(UpdaterScope scope,
+                           ScopedServer* test_server,
+                           const std::string& app_id,
+                           const std::string& install_data_index,
+                           const base::Version& from_version,
+                           const base::Version& to_version);
+
 void StressUpdateService(UpdaterScope scope);
 
 void CallServiceUpdate(UpdaterScope updater_scope,
@@ -259,7 +267,6 @@ void UninstallApp(UpdaterScope scope, const std::string& app_id);
 void RunOfflineInstall(UpdaterScope scope,
                        bool is_legacy_install,
                        bool is_silent_install);
-
 }  // namespace updater::test
 
 #endif  // CHROME_UPDATER_TEST_INTEGRATION_TESTS_IMPL_H_

chrome/updater/test/integration_tests_win.cc

@@ -1410,6 +1410,27 @@ void RunUninstallCmdLine(UpdaterScope scope) {
   EXPECT_EQ(0, exit_code);
 }
 
+void RunHandoff(UpdaterScope scope, const std::string& app_id) {
+  const absl::optional<base::FilePath> installed_executable_path =
+      GetInstalledExecutablePath(scope);
+  ASSERT_TRUE(installed_executable_path);
+  ASSERT_TRUE(base::PathExists(*installed_executable_path));
+
+  base::ScopedAllowBaseSyncPrimitivesForTesting allow_wait_process;
+  const std::wstring command_line(base::StrCat(
+      {installed_executable_path->value(), L" /handoff \"appguid=",
+       base::ASCIIToWide(app_id), L"&needsadmin=",
+       IsSystemInstall(scope) ? L"Prefers" : L"False", L"\" /silent"}));
+  VLOG(0) << " RunHandoff: " << command_line;
+  const base::Process process = base::LaunchProcess(command_line, {});
+  ASSERT_TRUE(process.IsValid());
+
+  int exit_code = 0;
+  ASSERT_TRUE(process.WaitForExitWithTimeout(TestTimeouts::action_max_timeout(),
+                                             &exit_code));
+  ASSERT_EQ(exit_code, 0);
+}
+
 void SetupFakeLegacyUpdaterData(UpdaterScope scope) {
   const HKEY root = UpdaterScopeToHKeyRoot(scope);
 

chrome/updater/updater_scope.cc

@@ -8,11 +8,11 @@
 #include "base/command_line.h"
 #include "build/build_config.h"
 #include "chrome/updater/constants.h"
+#include "chrome/updater/util/util.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
 
 #if BUILDFLAG(IS_WIN)
 #include "chrome/updater/tag.h"
-#include "chrome/updater/util/util.h"
 #include "chrome/updater/util/win_util.h"
 #endif
 
@@ -73,7 +73,7 @@ UpdaterScope GetUpdaterScopeForCommandLine(
 }
 
 UpdaterScope GetUpdaterScope() {
-  return GetUpdaterScopeForCommandLine(*base::CommandLine::ForCurrentProcess());
+  return GetUpdaterScopeForCommandLine(GetCommandLineLegacyCompatible());
 }
 
 bool IsSystemInstall() {

docs/updater/test_plan.md

@@ -70,6 +70,10 @@ IntegrationTest.Install tests that the updater can be installed on a clean OS,
 that it is immediately active after installation, and then can be cleanly
 uninstalled.
 
+IntegrationTest.Handoff tests that the updater can be installed on a clean OS,
+that it can install an app via a "/handoff" command line, and then can be
+cleanly uninstalled.
+
 Overinstall cases are tested by IntegrationTest.OverinstallWorking and
 IntegrationTest.OverinstallBroken, to ensure that the updater can be installed
 on an unclean OS, and as a post-condition of installation, the system has a