Add options to write version number to registry.

chrome/installer/setup/setup_main.cc

Index: chrome/installer/setup/setup_main.cc
diff --git a/chrome/installer/setup/setup_main.cc b/chrome/installer/setup/setup_main.cc
index 15998d1bb97eec20b37bd7bfa9c4fd41d62b74db..c94c952a8d2d34dfe559b45ce1246ca7f999e55a 100644
--- a/chrome/installer/setup/setup_main.cc
+++ b/chrome/installer/setup/setup_main.cc
@@ -80,6 +80,12 @@ using installer::Products;
 
 const wchar_t kGoogleUpdatePipeName[] = L"\\\\.\\pipe\\GoogleCrashServices\\";
 const wchar_t kSystemPrincipalSid[] = L"S-1-5-18";
+const wchar_t kDisplayVersion[] = L"DisplayVersion";
+const wchar_t kMsiInstallRegistryPrefix[] =
+    L"SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Installer\\UserData\\"
+    L"S-1-5-18\\Products\\";
+const wchar_t kMsiInstallRegistryPostfix[] = L"\\InstallProperties";
+const wchar_t kMsiDisplayVersionOverwriteDelay[] = L"30";  // seconds as string
 
 const MINIDUMP_TYPE kLargerDumpType = static_cast<MINIDUMP_TYPE>(
     MiniDumpWithProcessThreadData |  // Get PEB and TEB.
@@ -88,6 +94,77 @@ const MINIDUMP_TYPE kLargerDumpType = static_cast<MINIDUMP_TYPE>(
 
 namespace {
 
+// Overwrite an existing DisplayVersion as written by the MSI installer
+// with the real version number of Chrome.
+LONG OverwriteDisplayVersion(const base::string16& path,
+                             const base::string16& value) {
+  base::win::RegKey key;
+  LONG result = 0;
+  base::string16 existing;
+  if ((result = key.Open(HKEY_LOCAL_MACHINE, path.c_str(),
+                         KEY_QUERY_VALUE | KEY_SET_VALUE | KEY_WOW64_64KEY))
+      != ERROR_SUCCESS) {
+    LOG(ERROR) << "Failed to set DisplayVersion: " << path << " not found";
+    return result;
+  }
+  if ((result = key.ReadValue(kDisplayVersion, &existing)) != ERROR_SUCCESS) {
+    LOG(ERROR) << "Failed to set DisplayVersion: " << kDisplayVersion
+               << " not found under " << path;
+    return result;
+  }
+  if ((result = key.WriteValue(kDisplayVersion, value.c_str()))
+      != ERROR_SUCCESS) {
+    LOG(ERROR) << "Failed to set DisplayVersion: " << kDisplayVersion
+               << " could not be written under " << path;
+    return result;
+  }
+  VLOG(1) << "Set DisplayVersion at " << path << " to " << value
+          << " from " << existing;
+  return ERROR_SUCCESS;
+}
+
+void DelayedOverwriteDisplayVersion(const base::FilePath& setup_exe,
+                                    const std::string& id,
+                                    const Version& version) {
+  base::string16 reg_path(kMsiInstallRegistryPrefix);
+  reg_path += base::UTF8ToUTF16(id);
+  reg_path += kMsiInstallRegistryPostfix;
+
+  // This process has to be able to exit so we launch ourselves with
+  // instructions on what to do and then return.
+  base::string16 command_line;

[robertshield, 2015/08/18 17:27:26]

base::CommandLine provides convenience methods to do this much more simply.
AppendSwitchNative() will help.

[bcwhite, 2015/09/02 15:21:45]

Done.

+  command_line += L"\"";
+  command_line += setup_exe.AsUTF16Unsafe();
+  command_line += L"\" --";
+  command_line += base::UTF8ToUTF16(
+      installer::switches::kSetDisplayVersionPath);
+  command_line += L"=";
+  command_line += reg_path;
+  command_line += L" --";
+  command_line += base::UTF8ToUTF16(
+      installer::switches::kSetDisplayVersionValue);
+  command_line += L"=";
+  command_line += base::UTF8ToUTF16(version.GetString());
+  command_line += L" --";
+  command_line += base::UTF8ToUTF16(installer::switches::kDelay);
+  command_line += L"=";
+  command_line += kMsiDisplayVersionOverwriteDelay;
+
+  STARTUPINFOW si = {sizeof(si)};
+  PROCESS_INFORMATION pi = {0};
+  if (!::CreateProcess(setup_exe.AsUTF16Unsafe().c_str(),

[robertshield, 2015/08/18 17:27:26]

use base::LaunchProcess here
(https://code.google.com/p/chromium/codesearch#chromium/src/base/process/launc...)

[grt (UTC plus 2), 2015/08/19 17:43:30]

i wonder if this should also use force_breakaway_from_job_? i don't know whether
or not msiexec runs procs in a job, but i think it should be safe to try to
breakaway in any case. what do you think, robert? the reason to do so is that it
would be bad if msiexec was using a job object and did something like wait for
all procs in the job to complete.

[robertshield, 2015/08/19 17:57:07]

I don't think it would hurt. MSI could also conceivably be using the job object
to perform install cancellation, which we'd then escape from. Afaict we no-op
above in OverwriteDisplayVersion if the expected values aren't present so this
should be okay. 

[bcwhite, 2015/09/02 15:21:45]

Why here but not elsewhere in this file?

[grt (UTC plus 2), 2015/09/02 20:43:55]

base::LaunchProcess should always be used unless something special it doesn't
support is needed. If there are other direct uses of CreateProcess here, they
should likely be switched. It's very easy to mis-use CreateProcess in
interesting ways.

[bcwhite, 2015/09/09 18:20:22]

Done.

+                       (LPWSTR)command_line.c_str(),  // discards const

[robertshield, 2015/08/18 17:27:26]

can you const_cast instead?

[bcwhite, 2015/09/02 15:21:45]

Already gone with switch to base::CommandLine.

+                       NULL, NULL, FALSE, CREATE_NO_WINDOW, NULL, NULL,
+                       &si, &pi)) {
+    LOG(ERROR) << "Failed to set DisplayVersion: "
+               << "could not launch subprocess to make desired changes."
+               << " (error=" << ::GetLastError() << ")\n"
+               << command_line;
+    return;
+  }
+  ::CloseHandle(pi.hThread);
+}
+
 // Returns NULL if no compressed archive is available for processing, otherwise
 // returns a patch helper configured to uncompress and patch.
 scoped_ptr<installer::ArchivePatchHelper> CreateChromeArchiveHelper(
@@ -918,6 +995,18 @@ bool HandleNonInstallCmdLineOptions(const InstallationState& original_state,
                                     const base::CommandLine& cmd_line,
                                     InstallerState* installer_state,
                                     int* exit_code) {
+  // This option is independent of all others so doesn't belong in the if/else
+  // block below.
+  if (cmd_line.HasSwitch(installer::switches::kDelay)) {
+    const std::string delay_seconds_string(
+        cmd_line.GetSwitchValueASCII(installer::switches::kDelay));
+    int delay_seconds;
+    if (base::StringToInt(delay_seconds_string, &delay_seconds) &&
+        delay_seconds > 0) {
+      ::Sleep(delay_seconds * 1000);
+    }
+  }
+
   // TODO(gab): Add a local |status| variable which each block below sets;
   // only determine the |exit_code| from |status| at the end (this will allow
   // this method to validate that
@@ -1152,6 +1241,14 @@ bool HandleNonInstallCmdLineOptions(const InstallationState& original_state,
     bool updates_enabled = GoogleUpdateSettings::ReenableAutoupdates();
     *exit_code = updates_enabled ? installer::REENABLE_UPDATES_SUCCEEDED :
                                    installer::REENABLE_UPDATES_FAILED;
+  } else if (cmd_line.HasSwitch(installer::switches::kSetDisplayVersionPath)) {
+    const base::string16 registry_path(
+        cmd_line.GetSwitchValueNative(
+            installer::switches::kSetDisplayVersionPath));
+    const base::string16 registry_value(

[grt (UTC plus 2), 2015/08/19 17:43:30]

i think it's better to extract the version from the current binary rather than
pass it on the command line (see chrome/app/client_util.cc's
GetCurrentModuleVersion for code to copy/paste).

[bcwhite, 2015/09/02 15:21:45]

If the original process has already gone through the trouble to get it, is it
worth doing this?  Also, this leaves the flexibility of the caller being able to
choose something different if so desired.  Lastly, it is (IMO) more symmetric to
pass both key & value rather than just one.

+        cmd_line.GetSwitchValueNative(
+            installer::switches::kSetDisplayVersionValue));
+    *exit_code = OverwriteDisplayVersion(registry_path, registry_value);
   } else {
     handled = false;
   }
@@ -1536,13 +1633,19 @@ InstallStatus InstallProductsHelper(const InstallationState& original_state,
     }
   }
 
-  // If installation completed successfully, return the path to the directory
-  // containing the newly installed setup.exe and uncompressed archive if the
-  // caller requested it.
-  if (installer_directory &&
-      InstallUtil::GetInstallReturnCode(install_status) == 0) {
-    *installer_directory =
-        installer_state.GetInstallerDirectory(*installer_version);
+  // If the installation completed successfully...
+  if (InstallUtil::GetInstallReturnCode(install_status) == 0) {
+    // Update the DisplayVersion created by an MSI-based install.
+    std::string install_id;
+    if (prefs.GetString(installer::master_preferences::kMsiInstallId,
+                        &install_id)) {
+      DelayedOverwriteDisplayVersion(setup_exe, install_id, *installer_version);

[grt (UTC plus 2), 2015/08/19 17:43:30]

if i'm reading the code correctly, |setup_exe| is the path to the currently
running setup.exe, which will be deleted once this execution of setup.exe exits.
this code should instead use the path to the setup.exe that was just installed.
the call to GetInstallerDirectory on line 1648 gives you the directory
containing the installed setup.exe.

[bcwhite, 2015/09/02 15:21:45]

Done.

+    }
+    // Return the path to the directory containing the newly installed
+    // setup.exe and uncompressed archive if the caller requested it.
+    if (installer_directory)
+      *installer_directory =
+          installer_state.GetInstallerDirectory(*installer_version);
   }
 
   // temp_path's dtor will take care of deleting or scheduling itself for