Use a better registration suffix that will always be unique while respecting the MSDN rules.

chrome/installer/util/shell_util.cc

Index: chrome/installer/util/shell_util.cc
diff --git a/chrome/installer/util/shell_util.cc b/chrome/installer/util/shell_util.cc
index 2dc53ef030a988f9c8045d235cebf828f154de85..f27da4065bed322f4c590d7c8e1245681e20f7bb 100644
--- a/chrome/installer/util/shell_util.cc
+++ b/chrome/installer/util/shell_util.cc
@@ -28,6 +28,7 @@
 #include "base/values.h"
 #include "base/win/registry.h"
 #include "base/win/scoped_comptr.h"
+#include "base/win/win_util.h"
 #include "base/win/windows_version.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_switches.h"
@@ -85,6 +86,64 @@ bool IsChromeMetroSupported() {
   return VerifyVersionInfo(&min_version_info, type_mask, condition_mask) != 0;
 }
 
+// Sets |suffix| to a base 16 encoding of this user's sid preceded by a dot.
+// This suffix is then meant to be added to all registration that may conflict
+// with another user-level Chrome install.
+// This is guaranteed to be 25 characters (including the dot).
+// Note that prior to Chrome 21, the suffix registered used to be the user's
+// username (see GetOldUserSpecificRegistrySuffix() below). We still honor old
+// installs registered that way, but it was wrong because some of the characters
+// allowed in a username are not allowed in a ProgId.
+// Returns true unless the OS call to retrieve the username fails.
+bool GetNewUserSpecificRegistrySuffix(string16* suffix) {
+  string16 encoded_sid;
+  if (!base::win::GetUserSidBase16Encoded(&encoded_sid)) {
+    PLOG(DFATAL) << "GetUserSidBase16Encoded failed";
+    return false;
+  }
+  suffix->reserve(encoded_sid.length() + 1);
+  suffix->assign(1, L'.');
+  suffix->append(encoded_sid);
+  return true;
+}
+
+// Sets |suffix| to this user's username preceded by a dot. This suffix is then
+// meant to be added to all registration that may conflict with another
+// user-level Chrome install.
+// Returns true unless the OS call to retrieve the username fails.
+bool GetOldUserSpecificRegistrySuffix(string16* suffix) {

[gab, 2012/06/21 05:55:41]

This is not new, simply moved from lower to avoid cyclic dependency (well
actually only had to move GetNewUserSpecificRegistrySuffix() to avoid
cyclic-dep., but I figured they belonged together)

+  wchar_t user_name[256];
+  DWORD size = arraysize(user_name);
+  if (::GetUserName(user_name, &size) == 0 || size < 1) {
+    PLOG(DFATAL) << "GetUserName failed";
+    return false;
+  }
+  suffix->reserve(size);
+  suffix->assign(1, L'.');
+  suffix->append(user_name, size - 1);
+  return true;
+}
+
+// Returns Chrome's ProgId as "ChromeHTML|suffix|".

[grt (UTC plus 2), 2012/06/22 14:08:24]

although there are other funcs in this file with Chrome in the name, something
nags at me that it's not the best practice.  among the reasons: 1) it's Chrome
or Chromium depending on the build, 2) all of this is for Chrome anyway, and 3)
kChromeHTMLProgId should be replaced with a method on BrowserDistribution.

[gab, 2012/06/22 18:24:04]

Done.

+// |suffix| can be the empty string.
+string16 GetChromeHTMLProgId(const string16& suffix) {

[grt (UTC plus 2), 2012/06/22 14:08:24]

i'm not too fond of the fact that this is called all over the place and will
recompute the new suffix over and over again.  can you do something about that?

[gab, 2012/06/22 18:24:04]

Done.

Cached it locally as a static variable which is only initialized once. Let me
know if that works for you.

+  string16 chrome_html(ShellUtil::kChromeHTMLProgId);
+  chrome_html.append(suffix);
+
+  // ProgIds cannot be longer than 39 characters.
+  // Ref: http://msdn.microsoft.com/en-us/library/aa911706.aspx.
+  // We can however only make sure it's compliant with MSDN in the new-style of

[grt (UTC plus 2), 2012/06/22 14:08:24]

suggest: "Make all new registrations comply with this requirement (existing
registrations must be preserved)."

[gab, 2012/06/22 18:24:04]

Done.

+  // registrations as we didn't respect this before (and can't go back on what's
+  // currently installed).
+  string16 new_style_suffix;
+  if (GetNewUserSpecificRegistrySuffix(&new_style_suffix) &&
+      suffix.compare(new_style_suffix) == 0 && chrome_html.length() > 39) {
+    LOG(DFATAL) << "The suffixed ProgId is longer than 39 characters.";

[grt (UTC plus 2), 2012/06/22 14:08:24]

suggest NOTREACHED(); since the log will be meaningless in release builds and
the person who sees the NOTREACHED in practice can refer to the source to find
out what it means.

[gab, 2012/06/22 18:24:04]

Done.

+    chrome_html.erase(39, string16::npos);

[grt (UTC plus 2), 2012/06/22 14:08:24]

chrome_html.erase(39);

[gab, 2012/06/22 18:24:04]

Done.

+  }
+  return chrome_html;
+}
+
 // This class represents a single registry entry. The objective is to
 // encapsulate all the registry entries required for registering Chrome at one
 // place. This class can not be instantiated outside the class and the objects
@@ -181,8 +240,7 @@ class RegistryEntry {
     // File association ProgId
     string16 chrome_html_prog_id(ShellUtil::kRegClasses);
     chrome_html_prog_id.push_back(FilePath::kSeparators[0]);
-    chrome_html_prog_id.append(ShellUtil::kChromeHTMLProgId);
-    chrome_html_prog_id.append(suffix);
+    chrome_html_prog_id.append(GetChromeHTMLProgId(suffix));
     entries->push_front(new RegistryEntry(
         chrome_html_prog_id, ShellUtil::kChromeHTMLProgIdDesc));
     entries->push_front(new RegistryEntry(
@@ -234,7 +292,7 @@ class RegistryEntry {
                                            std::list<RegistryEntry*>* entries) {
     entries->push_front(new RegistryEntry(
         GetCapabilitiesKey(dist, suffix).append(L"\\URLAssociations"),
-        protocol, string16(ShellUtil::kChromeHTMLProgId).append(suffix)));
+        protocol, GetChromeHTMLProgId(suffix)));
     return true;
   }
 
@@ -295,8 +353,7 @@ class RegistryEntry {
     entries->push_front(new RegistryEntry(capabilities + L"\\Startmenu",
         L"StartMenuInternet", reg_app_name));
 
-    string16 html_prog_id(ShellUtil::kChromeHTMLProgId);
-    html_prog_id.append(suffix);
+    string16 html_prog_id(GetChromeHTMLProgId(suffix));
     for (int i = 0; ShellUtil::kFileAssociations[i] != NULL; i++) {
       entries->push_front(new RegistryEntry(
           capabilities + L"\\FileAssociations",
@@ -365,8 +422,7 @@ class RegistryEntry {
                              const string16& suffix,
                              std::list<RegistryEntry*>* entries) {
     // File extension associations.
-    string16 html_prog_id(ShellUtil::kChromeHTMLProgId);
-    html_prog_id.append(suffix);
+    string16 html_prog_id(GetChromeHTMLProgId(suffix));
     for (int i = 0; ShellUtil::kFileAssociations[i] != NULL; i++) {
       string16 ext_key(ShellUtil::kRegClasses);
       ext_key.push_back(FilePath::kSeparators[0]);
@@ -677,8 +733,7 @@ void RemoveBadWindows8RegistrationIfNeeded(
     // <root hkey>\Software\Classes\ChromiumHTML[.user]\shell\open\command
     key = ShellUtil::kRegClasses;
     key.push_back(FilePath::kSeparators[0]);
-    key.append(ShellUtil::kChromeHTMLProgId);
-    key.append(installation_suffix);
+    key.append(GetChromeHTMLProgId(installation_suffix));
     key.append(ShellUtil::kRegShellOpen);
     InstallUtil::DeleteRegistryValue(root_key, key,
                                      ShellUtil::kRegDelegateExecute);
@@ -747,28 +802,13 @@ bool QuickIsChromeRegistered(BrowserDistribution* dist,
   return false;
 }
 
-// Sets |suffix| to this user's username preceded by a dot. This suffix is then
-// meant to be added to all registration that may conflict with another
-// user-level Chrome install.
-// Returns true unless the OS call to retrieve the username fails.
-bool GetUserSpecificRegistrySuffix(string16* suffix) {
-  wchar_t user_name[256];
-  DWORD size = arraysize(user_name);
-  if (::GetUserName(user_name, &size) == 0 || size < 1) {
-    PLOG(DFATAL) << "GetUserName failed";
-    return false;
-  }
-  suffix->reserve(size);
-  suffix->assign(1, L'.');
-  suffix->append(user_name, size - 1);
-  return true;
-}
-
-// Sets |suffix| to the current user's username, preceded by a dot, on
-// user-level installs.
+// Sets |suffix| to a base 16 encoding of this user's sid preceded by a dot, on
+// user-level installs, preferably.
 // To support old-style user-level installs however, |suffix| is cleared if
 // the user currently owns the non-suffixed HKLM registrations.
-// |suffix| is also cleared on system-level installs.
+// |suffix| can also be set to the user's username if the current install
+// is suffixed as per the old-style registrations.
+// |suffix| is cleared on system-level installs.
 // |suffix| should then be appended to all Chrome properties that may conflict
 // with other Chrome user-level installs.
 // Returns true unless one of the underlying calls fails.
@@ -782,9 +822,19 @@ bool GetInstallationSpecificSuffix(BrowserDistribution* dist,
     // registered with no suffix.
     suffix->clear();
     return true;
-  } else {
-    return GetUserSpecificRegistrySuffix(suffix);
   }
+
+  string16 old_suffix;
+  if (!GetOldUserSpecificRegistrySuffix(&old_suffix)) {
+    return false;
+  } else if (QuickIsChromeRegistered(dist, chrome_exe, old_suffix,
+                                     CONFIRM_SHELL_REGISTRATION)) {
+    // Username suffix for installs that are suffixed as per the old-style.
+    suffix->assign(old_suffix);
+    return true;
+  }
+
+  return GetNewUserSpecificRegistrySuffix(suffix);
 }
 
 // Returns the root registry key (HKLM or HKCU) into which shell integration
@@ -1060,11 +1110,26 @@ void ShellUtil::GetRegisteredBrowsers(
 
 string16 ShellUtil::GetCurrentInstallationSuffix(BrowserDistribution* dist,
                                                  const string16& chrome_exe) {
+  // This method is somewhat the opposite of GetInstallationSpecificSuffix().
+  // In this case we are not trying to determine the current suffix for the
+  // upcoming installation (i.e. not trying to stick to a currently bad
+  // registration style if one is present).
+  // Here we want to determine which suffix we should use at run-time.
+  // In order of preference, we prefer (for user-level installs):
+  //   1) Base 16 encoded sid (new-style).
+  //   2) Username (old-style).
+  //   3) Unsuffixed (even worst).
   string16 tested_suffix;
   if (!InstallUtil::IsPerUserInstall(chrome_exe.c_str()) ||
-      !GetUserSpecificRegistrySuffix(&tested_suffix) ||
+      !GetNewUserSpecificRegistrySuffix(&tested_suffix) ||

[grt (UTC plus 2), 2012/06/22 14:08:24]

a failure in GetNewUserSpecificRegistrySuffix means that the following
QuickIsChromeRegistered can't be done, but it doesn't mean that
GetOldUserSpecificRegistrySuffix and its QuickIsChromeRegistered must also be
skipped.

[gab, 2012/06/22 18:24:04]

Ah yes, good catch, changed it to check old suffix if the new suffix's check
failed. If both failed then assume unsuffixed registration as originally
intended.

It's brackets and &&/|| logic madness, let me know if you feel I should split
this logic up some other way.

+      !QuickIsChromeRegistered(dist, chrome_exe, tested_suffix,
+                               CONFIRM_PROGID_REGISTRATION) ||
+      !GetOldUserSpecificRegistrySuffix(&tested_suffix) ||
       !QuickIsChromeRegistered(dist, chrome_exe, tested_suffix,
                                CONFIRM_PROGID_REGISTRATION)) {
+    DCHECK(InstallUtil::IsPerUserInstall(chrome_exe.c_str()) ||

[grt (UTC plus 2), 2012/06/22 14:08:24]

it's not clear to me what this is checking.  should it be !IsPerUserInstall ||
QuickIsChromeRegistered...?

[gab, 2012/06/22 18:24:04]

Ah yes, good catch :)! Added a comment to clarify.

+           QuickIsChromeRegistered(dist, chrome_exe, string16(),
+                                   CONFIRM_PROGID_REGISTRATION));
     return string16();
   }
   return tested_suffix;
@@ -1082,7 +1147,7 @@ string16 ShellUtil::GetBrowserModelId(BrowserDistribution* dist,
   string16 app_id(dist->GetBaseAppId());
   string16 suffix;
   if (InstallUtil::IsPerUserInstall(chrome_exe.c_str()) &&
-      !GetUserSpecificRegistrySuffix(&suffix)) {
+      !GetNewUserSpecificRegistrySuffix(&suffix)) {
     NOTREACHED();
   }
   // There is only one component (i.e. the suffixed appid) in this case, but it