Register the updater app id in registry (Windows).

Send both the updater and Chrome app ids in the "update apps" use case.
With this change, --install and --ua commands work as expected, meaning
the app ids and the versions are sent during update checks.

There is a edge case, not handled currently, where the app id for
the updater is not available until the first update check has happened.
This is going to be resolved in a future CL which lands as part of
fixing this CR bug.

R=waffles

Bug: 1020285
Change-Id: I2856a795ed61f590f85d792e4d5e1493bd23b84f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1894833Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712315}

chrome/updater/installer.cc

@@ -57,6 +57,21 @@ update_client::CrxComponent Installer::MakeCrxComponent() {
   return component;
 }
 
+std::vector<std::string> Installer::FindAppIds() {
+  base::FilePath app_install_dir;
+  if (!GetProductDirectory(&app_install_dir))
+    return {};
+  app_install_dir = app_install_dir.AppendASCII(kAppsDir);
+  std::vector<std::string> app_ids;
+  base::FileEnumerator file_enumerator(app_install_dir, false,
+                                       base::FileEnumerator::DIRECTORIES);
+  for (auto path = file_enumerator.Next(); !path.value().empty();
+       path = file_enumerator.Next()) {
+    app_ids.push_back(path.BaseName().MaybeAsASCII());
+  }
+  return app_ids;
+}
+
 void Installer::FindInstallOfApp() {
   VLOG(1) << __func__ << " for " << app_id_;
 

chrome/updater/installer.h

@@ -40,6 +40,9 @@ class Installer final : public update_client::CrxInstaller {
 
   const std::string app_id() const { return app_id_; }
 
+  // Returns the app ids that are managed by the CRX installer.
+  static std::vector<std::string> FindAppIds();
+
   // Finds the highest version install of the app, and updates the install
   // info for this installer instance.
   void FindInstallOfApp();

chrome/updater/update_apps.cc

@@ -54,9 +54,21 @@ class Observer : public update_client::UpdateClient::Observer {
 }  // namespace
 
 int UpdateApps() {
-  auto installer = base::MakeRefCounted<Installer>(kUpdaterAppId);
-  installer->FindInstallOfApp();
-  const auto component = installer->MakeCrxComponent();
+  auto app_ids = Installer::FindAppIds();
+
+  // Include the app id for the updater if it is not found. This could happen
+  // before the first update for the updater has been handled. This is a
+  // temporary workaround until the source of truth for the registered
+  // version is resolved.
+  if (!base::Contains(app_ids, kUpdaterAppId))
+    app_ids.push_back(kUpdaterAppId);
+
+  std::vector<base::Optional<update_client::CrxComponent>> components;
+  for (const auto& app_id : app_ids) {
+    auto installer = base::MakeRefCounted<Installer>(app_id);
+    installer->FindInstallOfApp();
+    components.push_back(installer->MakeCrxComponent());
+  }
 
   base::SingleThreadTaskExecutor main_task_executor(base::MessagePumpType::UI);
   base::RunLoop runloop;
@@ -71,17 +83,16 @@ int UpdateApps() {
     Observer observer(update_client);
     update_client->AddObserver(&observer);
 
-    const std::vector<std::string> ids = {installer->app_id()};
     update_client->Update(
-        ids,
+        app_ids,
         base::BindOnce(
-            [](const update_client::CrxComponent& component,
-               const std::vector<std::string>& ids)
-                -> std::vector<base::Optional<update_client::CrxComponent>> {
-              DCHECK_EQ(1u, ids.size());
-              return {component};
+            [](const std::vector<base::Optional<update_client::CrxComponent>>&
+                   components,
+               const std::vector<std::string>& ids) {
+              DCHECK_EQ(components.size(), ids.size());
+              return components;
             },
-            component),
+            components),
         false,
         base::BindOnce(
             [](base::OnceClosure closure, update_client::Error error) {

chrome/updater/updater.cc

@@ -23,13 +23,16 @@
 #include "chrome/updater/win/setup/uninstall.h"
 #endif
 
-// To install the updater on Windows, run:
-// "updater.exe --install --enable-logging --v=1 --vmodule=*/chrome/updater/*"
-// from the build directory. The program needs a number of dependencies which
-// are available in the |out| directory of the build.
-// To uninstall, run "updater.exe --uninstall" from its install directory or
-// from the build out directory. Doing this will make the program delete its
-// install directory using a shim cmd script.
+// To install the updater on Windows, run "updatersetup.exe" from the
+// build directory.
+//
+// To uninstall, run "updater.exe --uninstall" from its install directory,
+// which is under %LOCALAPPDATA%\Google\GoogleUpdater, or from the |out|
+// directory of the build.
+//
+// To debug, use the command line arguments:
+//    --enable-logging --vmodule=*/chrome/updater/*=2.
+
 namespace updater {
 
 namespace {
@@ -86,8 +89,7 @@ int UpdaterUpdateApps() {
 int UpdaterInstallApp() {
 #if defined(OS_WIN)
   // TODO(sorin): pick up the app id from the tag. https://crbug.com/1014298
-  // For now, use Omaha4 app id, just to get a CRX for testing of the UI.
-  return InstallApp({kUpdaterAppId});
+  return InstallApp({kChromeAppId});
 #else
   NOTREACHED();
   return -1;

chrome/updater/updater_constants.cc

@@ -20,7 +20,6 @@ const char kTestSwitch[] = "test";
 const char kInitDoneNotifierSwitch[] = "init-done-notifier";
 const char kNoRateLimitSwitch[] = "no-rate-limit";
 const char kEnableLoggingSwitch[] = "enable-logging";
-const char kLoggingLevelSwitch[] = "v";
 const char kLoggingModuleSwitch[] = "vmodule";
 
 // URLs.

chrome/updater/updater_constants.h

@@ -44,9 +44,6 @@ extern const char kInitDoneNotifierSwitch[];
 // Enables logging.
 extern const char kEnableLoggingSwitch[];
 
-// Specifies the logging level.
-extern const char kLoggingLevelSwitch[];
-
 // Specifies the logging module filter.
 extern const char kLoggingModuleSwitch[];
 
@@ -59,7 +56,7 @@ extern const char kUpdaterJSONDefaultUrl[];
 extern const char kCrashUploadURL[];
 extern const char kCrashStagingUploadURL[];
 
-// Paths.
+// File system paths.
 //
 // The directory name where CRX apps get installed. This is provided for demo
 // purposes, since products installed by this updater will be installed in

chrome/updater/win/constants.cc

@@ -6,7 +6,9 @@
 
 namespace updater {
 
-// The prefix to use for global names in WIN32 API's.
 const base::char16 kGlobalPrefix[] = L"Global\\G";
 
+extern const base::char16 kRegistryValuePV[] = L"pv";
+extern const base::char16 kRegistryValueName[] = L"name";
+
 }  // namespace updater

chrome/updater/win/constants.h

@@ -6,6 +6,7 @@
 #define CHROME_UPDATER_WIN_CONSTANTS_H_
 
 #include "base/strings/string16.h"
+#include "chrome/updater/updater_version.h"
 
 namespace updater {
 
@@ -13,6 +14,17 @@ namespace updater {
 // to avoid collision on kernel object names.
 extern const base::char16 kGlobalPrefix[];
 
+// Registry keys and value names.
+#define COMPANY_KEY "Software\\" COMPANY_SHORTNAME_STRING "\\"
+// Use |Update| instead of PRODUCT_FULLNAME_STRING for the registry key name
+// to be backward compatible with Google Update / Omaha.
+#define UPDATER_KEY COMPANY_KEY "Update\\"
+#define CLIENTS_KEY UPDATER_KEY "Clients\\"
+#define CLIENT_STATE_KEY UPDATER_KEY "ClientState\\"
+
+extern const base::char16 kRegistryValuePV[];
+extern const base::char16 kRegistryValueName[];
+
 }  // namespace updater
 
 #endif  // CHROME_UPDATER_WIN_CONSTANTS_H_

chrome/updater/win/installer/create_installer_archive.py

@@ -256,6 +256,10 @@ def DoComponentBuildTasks(staging_dir, build_dir, setup_runtime_deps):
     g_archive_inputs.append(setup_component_dll)
     shutil.copy(setup_component_dll, installer_dir)
 
+  # Handle the ICU data file dependency.
+  g_archive_inputs.append("icudtl.dat")
+  shutil.copy("icudtl.dat", installer_dir)
+
 def main(options):
   """Main method that reads input file, creates archive file and writes
   resource input file.

chrome/updater/win/installer/installer.cc

@@ -208,7 +208,8 @@ ProcessExitResult RunSetup(const Configuration& configuration,
     return ProcessExitResult(COMMAND_STRING_OVERFLOW);
   }
 
-  if (!cmd_line.append(L" --install --enable-logging --v=1"))
+  if (!cmd_line.append(
+          L" --install --enable-logging --vmodule=*/chrome/updater/*=2"))
     return ProcessExitResult(COMMAND_STRING_OVERFLOW);
 
   return RunProcessAndWait(setup_exe.get(), cmd_line.get());

chrome/updater/win/net/network_winhttp.cc

@@ -212,6 +212,9 @@ scoped_hinternet NetworkFetcherWinHTTP::OpenRequest() {
 
 HRESULT NetworkFetcherWinHTTP::SendRequest(const std::string& data) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
+  VLOG(2) << data;
+
   const uint32_t bytes_to_send = base::saturated_cast<uint32_t>(data.size());
   void* request_body =
       bytes_to_send ? const_cast<char*>(data.c_str()) : WINHTTP_NO_REQUEST_DATA;
@@ -232,7 +235,7 @@ void NetworkFetcherWinHTTP::SendRequestComplete() {
       request_handle_.get(),
       WINHTTP_QUERY_RAW_HEADERS_CRLF | WINHTTP_QUERY_FLAG_REQUEST_HEADERS,
       WINHTTP_HEADER_NAME_BY_INDEX, &all);
-  VLOG(2) << "request headers: " << all;
+  VLOG(3) << "request headers: " << all;
 
   net_error_ = ReceiveResponse();
   if (FAILED(net_error_))
@@ -252,7 +255,7 @@ void NetworkFetcherWinHTTP::ReceiveResponseComplete() {
   base::string16 all;
   QueryHeadersString(request_handle_.get(), WINHTTP_QUERY_RAW_HEADERS_CRLF,
                      WINHTTP_HEADER_NAME_BY_INDEX, &all);
-  VLOG(2) << "response headers: " << all;
+  VLOG(3) << "response headers: " << all;
 
   int response_code = 0;
   net_error_ = QueryHeadersInt(request_handle_.get(), WINHTTP_QUERY_STATUS_CODE,
@@ -518,7 +521,7 @@ void NetworkFetcherWinHTTP::StatusCallback(HINTERNET handle,
     base::StringAppendF(&msg, ", info=%s",
                         base::SysWideToUTF8(info_string).c_str());
 
-  VLOG(2) << "WinHttp status callback:"
+  VLOG(3) << "WinHttp status callback:"
           << " handle=" << handle << ", " << msg;
 
   switch (status) {

chrome/updater/win/setup/setup.cc

@@ -15,28 +15,33 @@
 #include "base/logging.h"
 #include "base/path_service.h"
 #include "base/strings/string16.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/win/scoped_com_initializer.h"
-#include "chrome/installer/util/copy_tree_work_item.h"
 #include "chrome/installer/util/self_cleaning_temp_dir.h"
 #include "chrome/installer/util/work_item_list.h"
 #include "chrome/updater/updater_constants.h"
 #include "chrome/updater/util.h"
+#include "chrome/updater/win/constants.h"
 #include "chrome/updater/win/setup/setup_util.h"
 #include "chrome/updater/win/task_scheduler.h"
+#include "chrome/updater/win/util.h"
 
 namespace updater {
 
 namespace {
 
 const base::char16* kUpdaterFiles[] = {
+    L"icudtl.dat",
     L"updater.exe",
     L"uninstall.cmd",
 #if defined(COMPONENT_BUILD)
     // TODO(sorin): get the list of component dependencies from a build-time
     // file instead of hardcoding the names of the components here.
     L"base.dll",
+    L"base_i18n.dll",
     L"boringssl.dll",
     L"crcrypto.dll",
+    L"icui18n.dll",
     L"icuuc.dll",
     L"libc++.dll",
     L"prefs.dll",
@@ -90,18 +95,28 @@ int Setup() {
   for (const auto* file : kUpdaterFiles) {
     const base::FilePath target_path = product_dir.Append(file);
     const base::FilePath source_path = source_dir.Append(file);
-    install_list->AddWorkItem(
-        WorkItem::CreateCopyTreeWorkItem(source_path, target_path, temp_dir,
-                                         WorkItem::ALWAYS, base::FilePath()));
+    install_list->AddCopyTreeWorkItem(source_path.value(), target_path.value(),
+                                      temp_dir.value(), WorkItem::ALWAYS);
+  }
+
+  for (const auto& key_path :
+       {GetRegistryKeyClientsUpdater(), GetRegistryKeyClientStateUpdater()}) {
+    install_list->AddCreateRegKeyWorkItem(HKEY_CURRENT_USER, key_path,
+                                          WorkItem::kWow64Default);
+    install_list->AddSetRegValueWorkItem(
+        HKEY_CURRENT_USER, key_path, WorkItem::kWow64Default, kRegistryValuePV,
+        base::ASCIIToUTF16(UPDATER_VERSION_STRING), true);
+    install_list->AddSetRegValueWorkItem(
+        HKEY_CURRENT_USER, key_path, WorkItem::kWow64Default,
+        kRegistryValueName, base::ASCIIToUTF16(PRODUCT_FULLNAME_STRING), true);
   }
 
   base::CommandLine run_updater_ua_command(product_dir.Append(L"updater.exe"));
   run_updater_ua_command.AppendSwitch(kUpdateAppsSwitch);
 #if !defined(NDEBUG)
   run_updater_ua_command.AppendSwitch(kEnableLoggingSwitch);
-  run_updater_ua_command.AppendSwitchASCII(kLoggingLevelSwitch, "1");
   run_updater_ua_command.AppendSwitchASCII(kLoggingModuleSwitch,
-                                           "*/chrome/updater/*");
+                                           "*/chrome/updater/*=2");
 #endif
   if (!install_list->Do() || !RegisterUpdateAppsTask(run_updater_ua_command)) {
     LOG(ERROR) << "Install failed, rolling back...";

chrome/updater/win/setup/uninstall.cc

@@ -16,17 +16,21 @@
 #include "base/stl_util.h"
 #include "base/strings/string16.h"
 #include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/win/scoped_com_initializer.h"
+#include "chrome/installer/util/work_item_list.h"
 #include "chrome/updater/updater_constants.h"
 #include "chrome/updater/util.h"
+#include "chrome/updater/win/constants.h"
 #include "chrome/updater/win/setup/setup_util.h"
 #include "chrome/updater/win/task_scheduler.h"
 
 namespace updater {
 
 // Reverses the changes made by setup. This is a best effort uninstall:
-// 1. deletes the scheduled task.
-// 2. runs the uninstall script in the install directory of the updater.
+// 1. Deletes the scheduled task.
+// 2. Deletes the Clients and ClientState keys.
+// 3. Runs the uninstall script in the install directory of the updater.
 // The execution of this function and the script race each other but the script
 // loops and waits in between iterations trying to delete the install directory.
 int Uninstall() {
@@ -45,6 +49,16 @@ int Uninstall() {
 
   updater::UnregisterUpdateAppsTask();
 
+  std::unique_ptr<WorkItemList> uninstall_list(WorkItem::CreateWorkItemList());
+  uninstall_list->AddDeleteRegKeyWorkItem(HKEY_CURRENT_USER,
+                                          base::ASCIIToUTF16(UPDATER_KEY),
+                                          WorkItem::kWow64Default);
+  if (!uninstall_list->Do()) {
+    LOG(ERROR) << "Failed to delete the registry keys.";
+    uninstall_list->Rollback();
+    return -1;
+  }
+
   base::FilePath product_dir;
   if (!GetProductDirectory(&product_dir)) {
     LOG(ERROR) << "GetProductDirectory failed.";
@@ -60,8 +74,7 @@ int Uninstall() {
   base::FilePath script_path = product_dir.AppendASCII(kUninstallScript);
 
   base::string16 cmdline = cmd_path;
-  base::StringAppendF(&cmdline, L" /Q /C \"%ls\"",
-                      script_path.AsUTF16Unsafe().c_str());
+  base::StringAppendF(&cmdline, L" /Q /C \"%ls\"", script_path.value().c_str());
   base::LaunchOptions options;
   options.start_hidden = true;
 

chrome/updater/win/util.cc

@@ -13,8 +13,10 @@
 #include "base/guid.h"
 #include "base/logging.h"
 #include "base/process/process_iterator.h"
+#include "base/strings/strcat.h"
 #include "base/strings/string16.h"
-#include "base/strings/sys_string_conversions.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/updater/updater_constants.h"
 #include "chrome/updater/win/constants.h"
 #include "chrome/updater/win/user_info.h"
 
@@ -168,7 +170,7 @@ HRESULT CreateUniqueEventInEnvironment(const base::string16& var_name,
                                        HANDLE* unique_event) {
   DCHECK(unique_event);
 
-  const base::string16 event_name = base::SysUTF8ToWide(base::GenerateGUID());
+  const base::string16 event_name = base::ASCIIToUTF16(base::GenerateGUID());
   NamedObjectAttributes attr;
   GetNamedObjectAttributes(event_name.c_str(), is_machine, &attr);
 
@@ -301,4 +303,13 @@ void GetAdminDaclSecurityAttributes(CSecurityAttributes* sec_attr,
   GetAdminDaclSecurityDescriptor(&sd, accessmask);
   sec_attr->Set(sd);
 }
+
+base::string16 GetRegistryKeyClientsUpdater() {
+  return base::ASCIIToUTF16(base::StrCat({CLIENTS_KEY, kUpdaterAppId}));
+}
+
+base::string16 GetRegistryKeyClientStateUpdater() {
+  return base::ASCIIToUTF16(base::StrCat({CLIENT_STATE_KEY, kUpdaterAppId}));
+}
+
 }  // namespace updater

chrome/updater/win/util.h

@@ -90,6 +90,14 @@ void GetAdminDaclSecurityAttributes(CSecurityAttributes* sec_attr,
 // to admins and system.
 void GetAdminDaclSecurityDescriptor(CSecurityDesc* sd, ACCESS_MASK accessmask);
 
+// Returns the registry path for the Updater app id under the |Clients| subkey.
+// The path does not include the registry root hive prefix.
+base::string16 GetRegistryKeyClientsUpdater();
+
+// Returns the registry path for the Updater app id under the |ClientState|
+// subkey. The path does not include the registry root hive prefix.
+base::string16 GetRegistryKeyClientStateUpdater();
+
 }  // namespace updater
 
 #endif  // CHROME_UPDATER_WIN_UTIL_H_