Add fixing of MSI DisplayVersion to Chrome installs/updates.

chrome/installer/setup/install_worker.cc

Index: chrome/installer/setup/install_worker.cc
diff --git a/chrome/installer/setup/install_worker.cc b/chrome/installer/setup/install_worker.cc
index cca3132c6606beb850f0a1e8213422f4a586343c..e6f74116aafc704ae47411ab93cf3e73fde65785 100644
--- a/chrome/installer/setup/install_worker.cc
+++ b/chrome/installer/setup/install_worker.cc
@@ -11,6 +11,7 @@
 #include <shlobj.h>
 #include <time.h>
 
+#include <algorithm>
 #include <vector>
 
 #include "base/bind.h"
@@ -20,7 +21,9 @@
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/path_service.h"
+#include "base/strings/string_tokenizer.h"
 #include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/version.h"
 #include "base/win/registry.h"
@@ -78,6 +81,24 @@ const wchar_t kElevationPolicyKeyPath[] =
 const wchar_t kLegacyCmdInstallApp[] = L"install-application";
 const wchar_t kLegacyCmdInstallExtension[] = L"install-extension";
 
+// The prefix of the ClientState value that contains the MSI product id in the
+// format "<kMSIProductIdPrefix><PRODUCTID>".
+const wchar_t kMSIProductIdPrefix[] = L"EnterpriseProduct";
+
+// The common path for uninstall registry entries that show up in the
+// "Add/Remove Programs" dialog and its later incarnations.
+const wchar_t kUninstallRegPathRoot[] =
+    L"Software\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\";
+
+// This is the path to the MSI user data cache, which should probably never be
+// tampered with but which we'll try tweaking anyway. Other items of note here
+// are that this hard-codes the Local System SID meaning that this is strictly
+// only usable for system-level installs. This would need to change for
+// http://crbug.com/111058 to be fixed.

[grt (UTC plus 2), 2014/11/27 19:42:04]

please add to the doc comment that this is a format string and what the
substituted value must be.

[robertshield, 2014/11/27 22:25:57]

Done.

+const wchar_t kMSIUserDataCacheRoot[] =
+    L"SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Installer\\UserData\\"
+    L"S-1-5-18\\Products\\%ls\\InstallProperties";
+
 void GetOldIELowRightsElevationPolicyKeyPath(base::string16* key_path) {
   key_path->assign(kElevationPolicyKeyPath,
                    arraysize(kElevationPolicyKeyPath) - 1);
@@ -672,8 +693,81 @@ void CleanupBadCanaryDelegateExecuteRegistration(
   }
 }
 
+// Returns the MSI product ID from the ClientState key that is populated for MSI
+// installs. This property is encoded in a value name whose format is
+// "EnterpriseId<GUID>" where <GUID> is the MSI product id. <GUID> is in the
+// format XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX. The id will be placed in
+// |product_id| if the latter is non-null.
+//
+// This format is strange and its provenance is shrouded in mystery but it has
+// the data we need, so use it.
+bool ExtractMSIProductId(const InstallerState& installer_state,
+                         const Product& product,
+                         base::string16* product_id) {

[grt (UTC plus 2), 2014/11/27 19:42:04]

why the out param for this? why not return the product id or an empty string on
failure?

[robertshield, 2014/11/27 22:25:57]

The result of pruning an earlier API and not pruning hard enough. Now fixed.

+  HKEY reg_root = installer_state.root_key();
+  BrowserDistribution* dist = product.distribution();
+  DCHECK(dist);
+
+  base::win::RegistryValueIterator value_iter(reg_root,
+                                              dist->GetStateKey().c_str());
+  while (value_iter.Valid()) {

[grt (UTC plus 2), 2014/11/27 19:42:04]

nit:
  for (; value_iter.Valid(); ++value_iter) {
and remove line 721

[robertshield, 2014/11/27 22:25:57]

Done.

+    base::string16 value_name(value_iter.Name());
+    if (StartsWith(value_name, kMSIProductIdPrefix, false)) {
+      if (product_id) {
+        *product_id += value_name.substr(arraysize(kMSIProductIdPrefix) - 1);

[grt (UTC plus 2), 2014/11/27 19:42:04]

why append?

[robertshield, 2014/11/27 22:25:57]

Done.

+      }
+      return true;
+    }
+    ++value_iter;
+  }
+  return false;
+}
+
 }  // namespace
 
+// Converts a correctly formatted guid thusly:
+// 12345678-90ab-cdef-ghij-klmnopqrstuv -> 87654321ba09fedchgjilknmporqtsvu
+// This reverses the string of the first three terms and inverts the bytes of
+// the last two. It also removes the dashes. It expects the input to NOT be
+// wrapped in braces.
+// Returns an empty string on failure.
+base::string16 ConvertGUIDToMSIUserDataFormat(const base::string16& guid) {

[grt (UTC plus 2), 2014/11/27 19:42:04]

please keep the definition order here in the .cc file in sync with the
declaration order in the .h file. in other words, either put this at the bottom
of the file here or at the top in the header.

[robertshield, 2014/11/27 22:25:57]

Done.

+  // Perform some very mild validation of the input.
+  if (guid.size() != 36)
+    return base::string16();
+
+  base::string16 msi_formatted_guid;
+  int token_count = 0;
+  base::WStringTokenizer t(guid, L"-");

[grt (UTC plus 2), 2014/11/27 19:42:04]

t -> tokenizer ("Function names, variable names, and filenames should be
descriptive; eschew abbreviation."
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin...).

[robertshield, 2014/11/27 22:25:57]

Done.

+  while (t.GetNext()) {
+    // There should be exactly five tokens, bail if not.
+    if (token_count >= 5)
+      return base::string16();
+
+    base::string16 token(t.token());
+    if (token_count < 3) {
+      // For the first three tokens, just reverse the characters.
+      std::reverse(token.begin(), token.end());
+      msi_formatted_guid += token;
+    } else {
+      // The last two tokens need to be of even length.
+      if (token.size() % 2)
+        return base::string16();
+      // Swap the halves of each byte (i.e. pair of chars). I don't even.
+      for (size_t i = 0; i < token.size(); i += 2) {
+        msi_formatted_guid += token[i+1];
+        msi_formatted_guid += token[i];
+      }
+    }
+    token_count++;
+  }
+
+  if (token_count == 5)

[grt (UTC plus 2), 2014/11/27 19:42:04]

nit: more shorter?
  return (token_count == 5) ? msi_formatted_guid : base::string();

[robertshield, 2014/11/27 22:25:57]

Done.

+    return msi_formatted_guid;
+  else
+    return base::string16();
+}
+
 // This method adds work items to create (or update) Chrome uninstall entry in
 // either the Control Panel->Add/Remove Programs list or in the Omaha client
 // state key if running under an MSI installer.
@@ -721,102 +815,127 @@ void AddUninstallShortcutWorkItems(const InstallerState& installer_state,
       true);
 
   // MSI installations will manage their own uninstall shortcuts.
-  if (!installer_state.is_msi() && product.ShouldCreateUninstallEntry()) {
-    // We need to quote the command line for the Add/Remove Programs dialog.
-    CommandLine quoted_uninstall_cmd(installer_path);
-    DCHECK_EQ(quoted_uninstall_cmd.GetCommandLineString()[0], '"');
-    quoted_uninstall_cmd.AppendArguments(uninstall_arguments, false);
-
-    base::string16 uninstall_reg = browser_dist->GetUninstallRegPath();
-    install_list->AddCreateRegKeyWorkItem(
-        reg_root, uninstall_reg, KEY_WOW64_32KEY);
-    install_list->AddSetRegValueWorkItem(reg_root,
-                                         uninstall_reg,
-                                         KEY_WOW64_32KEY,
-                                         installer::kUninstallDisplayNameField,
-                                         browser_dist->GetDisplayName(),
-                                         true);
-    install_list->AddSetRegValueWorkItem(
-        reg_root,
-        uninstall_reg,
-        KEY_WOW64_32KEY,
-        installer::kUninstallStringField,
-        quoted_uninstall_cmd.GetCommandLineString(),
-        true);
-    install_list->AddSetRegValueWorkItem(reg_root,
-                                         uninstall_reg,
-                                         KEY_WOW64_32KEY,
-                                         L"InstallLocation",
-                                         install_path.value(),
-                                         true);
-
-    BrowserDistribution* dist = product.distribution();
-    base::string16 chrome_icon = ShellUtil::FormatIconLocation(
-        install_path.Append(dist->GetIconFilename()).value(),
-        dist->GetIconIndex(BrowserDistribution::SHORTCUT_CHROME));
-    install_list->AddSetRegValueWorkItem(reg_root,
-                                         uninstall_reg,
-                                         KEY_WOW64_32KEY,
-                                         L"DisplayIcon",
-                                         chrome_icon,
-                                         true);
-    install_list->AddSetRegValueWorkItem(reg_root,
-                                         uninstall_reg,
-                                         KEY_WOW64_32KEY,
-                                         L"NoModify",
-                                         static_cast<DWORD>(1),
-                                         true);
-    install_list->AddSetRegValueWorkItem(reg_root,
-                                         uninstall_reg,
-                                         KEY_WOW64_32KEY,
-                                         L"NoRepair",
-                                         static_cast<DWORD>(1),
-                                         true);
-
-    install_list->AddSetRegValueWorkItem(reg_root,
-                                         uninstall_reg,
-                                         KEY_WOW64_32KEY,
-                                         L"Publisher",
-                                         browser_dist->GetPublisherName(),
-                                         true);
-    install_list->AddSetRegValueWorkItem(reg_root,
-                                         uninstall_reg,
-                                         KEY_WOW64_32KEY,
-                                         L"Version",
-                                         ASCIIToUTF16(new_version.GetString()),
-                                         true);
-    install_list->AddSetRegValueWorkItem(reg_root,
-                                         uninstall_reg,
-                                         KEY_WOW64_32KEY,
-                                         L"DisplayVersion",
-                                         ASCIIToUTF16(new_version.GetString()),
-                                         true);
-    // TODO(wfh): Ensure that this value is preserved in the 64-bit hive when
-    // 64-bit installs place the uninstall information into the 64-bit registry.
-    install_list->AddSetRegValueWorkItem(reg_root,
-                                         uninstall_reg,
-                                         KEY_WOW64_32KEY,
-                                         L"InstallDate",
-                                         InstallUtil::GetCurrentDate(),
-                                         false);
-
-    const std::vector<uint16>& version_components = new_version.components();
-    if (version_components.size() == 4) {
-      // Our version should be in major.minor.build.rev.
+  if (product.ShouldCreateUninstallEntry()) {
+    if (!installer_state.is_msi()) {
+      // We need to quote the command line for the Add/Remove Programs dialog.
+      CommandLine quoted_uninstall_cmd(installer_path);
+      DCHECK_EQ(quoted_uninstall_cmd.GetCommandLineString()[0], '"');
+      quoted_uninstall_cmd.AppendArguments(uninstall_arguments, false);
+
+      base::string16 uninstall_reg = browser_dist->GetUninstallRegPath();
+      install_list->AddCreateRegKeyWorkItem(reg_root, uninstall_reg,
+                                            KEY_WOW64_32KEY);
       install_list->AddSetRegValueWorkItem(
-          reg_root,
-          uninstall_reg,
-          KEY_WOW64_32KEY,
-          L"VersionMajor",
-          static_cast<DWORD>(version_components[2]),
+          reg_root, uninstall_reg, KEY_WOW64_32KEY,
+          installer::kUninstallDisplayNameField, browser_dist->GetDisplayName(),
           true);
       install_list->AddSetRegValueWorkItem(
-          reg_root,
-          uninstall_reg,
-          KEY_WOW64_32KEY,
-          L"VersionMinor",
-          static_cast<DWORD>(version_components[3]),
-          true);
+          reg_root, uninstall_reg, KEY_WOW64_32KEY,
+          installer::kUninstallStringField,
+          quoted_uninstall_cmd.GetCommandLineString(), true);
+      install_list->AddSetRegValueWorkItem(reg_root, uninstall_reg,
+                                           KEY_WOW64_32KEY, L"InstallLocation",
+                                           install_path.value(), true);
+
+      BrowserDistribution* dist = product.distribution();
+      base::string16 chrome_icon = ShellUtil::FormatIconLocation(
+          install_path.Append(dist->GetIconFilename()).value(),
+          dist->GetIconIndex(BrowserDistribution::SHORTCUT_CHROME));
+      install_list->AddSetRegValueWorkItem(reg_root, uninstall_reg,
+                                           KEY_WOW64_32KEY, L"DisplayIcon",
+                                           chrome_icon, true);
+      install_list->AddSetRegValueWorkItem(reg_root, uninstall_reg,
+                                           KEY_WOW64_32KEY, L"NoModify",
+                                           static_cast<DWORD>(1), true);
+      install_list->AddSetRegValueWorkItem(reg_root, uninstall_reg,
+                                           KEY_WOW64_32KEY, L"NoRepair",
+                                           static_cast<DWORD>(1), true);
+
+      install_list->AddSetRegValueWorkItem(
+          reg_root, uninstall_reg, KEY_WOW64_32KEY, L"Publisher",
+          browser_dist->GetPublisherName(), true);
+      install_list->AddSetRegValueWorkItem(
+          reg_root, uninstall_reg, KEY_WOW64_32KEY, L"Version",
+          ASCIIToUTF16(new_version.GetString()), true);
+      install_list->AddSetRegValueWorkItem(
+          reg_root, uninstall_reg, KEY_WOW64_32KEY, L"DisplayVersion",
+          ASCIIToUTF16(new_version.GetString()), true);
+      // TODO(wfh): Ensure that this value is preserved in the 64-bit hive when
+      // 64-bit installs place the uninstall information into the 64-bit
+      // registry.
+      install_list->AddSetRegValueWorkItem(
+          reg_root, uninstall_reg, KEY_WOW64_32KEY, L"InstallDate",
+          InstallUtil::GetCurrentDate(), false);
+
+      const std::vector<uint16>& version_components = new_version.components();
+      if (version_components.size() == 4) {
+        // Our version should be in major.minor.build.rev.
+        install_list->AddSetRegValueWorkItem(
+            reg_root, uninstall_reg, KEY_WOW64_32KEY, L"VersionMajor",
+            static_cast<DWORD>(version_components[2]), true);
+        install_list->AddSetRegValueWorkItem(
+            reg_root, uninstall_reg, KEY_WOW64_32KEY, L"VersionMinor",
+            static_cast<DWORD>(version_components[3]), true);
+      }
+    } else {
+      // This is an MSI install. Largely, leave all uninstall shortcuts and the
+      // like up to the MSI machinery EXCEPT for the DisplayVersion property.
+      //
+      // Due to reasons, the Chrome version number is wider than can fit inside
+      // the <8bits>.<16bits>.<8bits> allotted to MSI version numbers. To make
+      // things work the A.B.C.D Chrome version is lossily encoded into an X.Y.Z
+      // version, dropping the unneeded A and B terms. For full details, see
+      // https://code.google.com/p/chromium/issues/detail?id=67348#c62
+      //
+      // The upshot of this is that Chrome MSIs have a different visible version
+      // than actual Chrome releases which causes trouble with some workflows.
+      // To help mitigate this, attempt to keep the DisplayVersion property in
+      // sync for MSI installs in both the Add/Remove Programs dialog and the
+      // corresponding MSI user data cache.
+
+      // Figure out MSI ProductID by looking in ClientState.
+      // The ProductID is encoded as a key named "EnterpriseProduct<ProductID>".
+      base::string16 msi_product_id;
+      if (ExtractMSIProductId(installer_state, product, &msi_product_id)) {
+        base::string16 uninstall_reg_path(kUninstallRegPathRoot);
+        uninstall_reg_path.append(L"{");
+        uninstall_reg_path.append(msi_product_id);
+        uninstall_reg_path.append(L"}");
+
+        install_list->AddCreateRegKeyWorkItem(reg_root, uninstall_reg_path,

[grt (UTC plus 2), 2014/11/27 19:42:04]

this key is owned by Windows, no? if so, it isn't Chrome's responsibility to
create it. if it's not there (imagine it gets moved in the future), then
Chrome's installer probably shouldn't create it, don't you think? or am i
misunderstanding?

[robertshield, 2014/11/27 22:25:57]

Hrmm, that's not unreasonable: the key is owned by the MSI machinery, I was
creating it here because I wasn't sure if it would be present before the custom
action that runs the .exe installer was run. Conversely it's at least vaguely
possible that creating it here is harmful. Let me postpone my answer here until
I complete the end-to-end test w/ building a full MSI.

+                                              WorkItem::kWow64Default);
+        install_list->AddSetRegValueWorkItem(
+            reg_root, uninstall_reg_path, WorkItem::kWow64Default,
+            installer::kUninstallDisplayVersionField,
+            base::ASCIIToWide(new_version.GetString()), true);

[grt (UTC plus 2), 2014/11/27 19:42:04]

please use base::ASCIIToUTF16 here and on line 931 (see http://crbug.com/23581
for reference).

[grt (UTC plus 2), 2014/11/27 19:42:04]

should failures here not also be ignored?

[robertshield, 2014/11/27 22:25:57]

Done.

[robertshield, 2014/11/27 22:25:57]

Ok. 
<grump>
If ASCIIToWide is deprecated, it should be marked as such.
</grump>

[grt (UTC plus 2), 2014/12/01 16:19:30]

Agreed.

+
+        // Also fix up the MSI user data cache. This is a little hacky, there
+        // may be a better way and I am not certain this won't cause undesired
+        // side-effects. Note that the user data cache registry path includes
+        // the product id without dashes. Also note that the
+        // kMSIUserDataCacheRoot currently hardcodes the local system SID, which
+        // will need to be changed for user-level MSI installs to happen as
+        // tracked by http://crbug.com/111058. Leave a DCHECK here to warn those
+        // who next venture here to use a correct SID in kMSIUserDataCacheRoot.
+        DCHECK(installer_state.system_install());
+
+        base::string16 msi_user_data_product_id(
+            ConvertGUIDToMSIUserDataFormat(msi_product_id));
+        if (!msi_user_data_product_id.empty()) {
+          base::string16 msi_cache_reg_path(base::StringPrintf(
+              kMSIUserDataCacheRoot, msi_user_data_product_id.c_str()));
+
+          install_list->AddSetRegValueWorkItem(
+              reg_root, msi_cache_reg_path, KEY_WOW64_64KEY,
+              installer::kUninstallDisplayVersionField,
+              base::ASCIIToWide(new_version.GetString()), true)->
+                  set_ignore_failure(true);
+        } else {
+          VLOG(1) << "Failed to convert MSI user data id.";

[grt (UTC plus 2), 2014/11/27 19:42:04]

suggestion: LOG(DFATAL) and include msi_product_id in the log message so we can
see what the input was.

[robertshield, 2014/11/27 22:25:57]

I had picked VLOG since we always run the installer with verbose-logging in the
MSI and I'm not sure about wanting to DCHECK from a child-process of an MSI
custom action which doesn't run on the interactive desktop iirc.

Let me know if you'd rather I LOG(ERROR). 

Adding the extra logging details in any case.

[grt (UTC plus 2), 2014/12/01 16:19:30]

This new code runs on all updates following an MSI install, so it isn't
restricted to only running in the context of a Windows Installer-driven MSI
install.
desktop iirc.

We'll never ship an MSI with DCHECK on, so this will never CHECK when run from
an MSI. DFATAL will mean it'll be a DCHECK in debug builds (which you'll be
doing by hand) and an ERROR in release builds.
I still thing LOG(DFATAL) is a good blend: CHECK in debug, ERROR otherwise.
Thanks.

+        }
+      } else {
+        VLOG(1) << "Failed to extract MSI product id.";
+      }
     }
   }
 }