Always suffix ChromeHTML entries on Windows for user-level installs.

chrome/installer/setup/uninstall.cc

Index: chrome/installer/setup/uninstall.cc
diff --git a/chrome/installer/setup/uninstall.cc b/chrome/installer/setup/uninstall.cc
index 86b374fa710dcfb48b072e5f3ab8fa08161bb737..e1c8d8643e48c3b0db1fae83d07a3d78f8f5ade3 100644
--- a/chrome/installer/setup/uninstall.cc
+++ b/chrome/installer/setup/uninstall.cc
@@ -220,33 +220,6 @@ void CloseChromeFrameHelperProcess() {
   }
 }
 
-// This method tries to figure out if current user has registered Chrome.
-// It returns true iff there is a registered browser that will launch the
-// same chrome.exe as the current installation.
-bool CurrentUserHasDefaultBrowser(const InstallerState& installer_state) {
-  using base::win::RegistryKeyIterator;
-  const HKEY root = HKEY_LOCAL_MACHINE;
-  ProgramCompare open_command_pred(
-      installer_state.target_path().Append(kChromeExe));
-  string16 client_open_path;
-  RegKey client_open_key;
-  string16 reg_exe;
-  for (RegistryKeyIterator iter(root, ShellUtil::kRegStartMenuInternet);
-       iter.Valid(); ++iter) {
-    client_open_path.assign(ShellUtil::kRegStartMenuInternet)
-        .append(1, L'\\')
-        .append(iter.Name())
-        .append(ShellUtil::kRegShellOpen);
-    if (client_open_key.Open(root, client_open_path.c_str(),
-                             KEY_QUERY_VALUE) == ERROR_SUCCESS &&
-        client_open_key.ReadValue(L"", &reg_exe) == ERROR_SUCCESS &&
-        open_command_pred.Evaluate(reg_exe)) {
-      return true;
-    }
-  }
-  return false;
-}
-
 // This method deletes Chrome shortcut folder from Windows Start menu. It
 // checks system_uninstall to see if the shortcut is in all users start menu
 // or current user start menu.
@@ -678,9 +651,8 @@ const wchar_t kChromeExtProgId[] = L"ChromiumExt";
   HKEY roots[] = { HKEY_LOCAL_MACHINE, HKEY_CURRENT_USER };
   for (size_t i = 0; i < arraysize(roots); ++i) {
     string16 suffix;
-    if (roots[i] == HKEY_LOCAL_MACHINE &&
-        !ShellUtil::GetUserSpecificDefaultBrowserSuffix(dist, &suffix))
-      suffix = L"";
+    if (roots[i] == HKEY_LOCAL_MACHINE)
+      suffix = ShellUtil::GetCurrentInstallationSuffix();
 
     // Delete Software\Classes\ChromeExt,
     string16 ext_prog_id(ShellUtil::kRegClasses);
@@ -730,12 +702,12 @@ InstallStatus UninstallProduct(const InstallationState& original_state,
                                bool force_uninstall,
                                const CommandLine& cmd_line) {
   InstallStatus status = installer::UNINSTALL_CONFIRMED;
-  string16 suffix;
-  if (!ShellUtil::GetUserSpecificDefaultBrowserSuffix(product.distribution(),
-                                                      &suffix))
-    suffix = L"";
-
   BrowserDistribution* browser_dist = product.distribution();
+  const string16 chrome_exe(
+      installer_state.target_path().Append(installer::kChromeExe).value());
+
+  const string16 suffix(ShellUtil::GetCurrentInstallationSuffix());
+
   bool is_chrome = product.is_chrome();
 
   VLOG(1) << "UninstallProduct: " << browser_dist->GetApplicationName();
@@ -756,8 +728,9 @@ InstallStatus UninstallProduct(const InstallationState& original_state,
     // Check if we need admin rights to cleanup HKLM. If we do, try to launch
     // another uninstaller (silent) in elevated mode to do HKLM cleanup.
     // And continue uninstalling in the current process also to do HKCU cleanup.
-    if (remove_all &&
-        (!suffix.empty() || CurrentUserHasDefaultBrowser(installer_state)) &&
+    if (ShellUtil::IsInstallationPresentInHKLM(browser_dist, chrome_exe,

[grt (UTC plus 2), 2012/05/30 20:37:40]

The behavior implemented in IsInstallationPresentInHKLM is not desired here. 
Uninstall is a best-effort deal, so we don't want to leave everything behind if
one expected value isn't in the registry.  Instead, we just want to know if any
system-level registrations need to be removed.  The old code took some pains to
search for registration pointing toward the current chrome.exe.

[gab, 2012/05/31 06:36:01]

Arg, this was the intent of IsInstallationPresentInHKLM all along, but I messed
up when I refactored my CL into fewer calls (i.e. went from return true if ANY
are present to ALL...)

Fixed.

+                                               suffix) &&
+        (!suffix.empty() || remove_all) &&
         !::IsUserAnAdmin() &&
         base::win::GetVersion() >= base::win::VERSION_VISTA &&
         !cmd_line.HasSwitch(installer::switches::kRunAsAdmin)) {
@@ -811,21 +784,29 @@ InstallStatus UninstallProduct(const InstallationState& original_state,
   // Registration data is put in HKCU for both system level and user level
   // installs.
   InstallStatus ret = installer::UNKNOWN_STATUS;
-  DeleteChromeRegistrationKeys(product.distribution(), HKEY_CURRENT_USER,
-                               suffix, installer_state.target_path(), &ret);
+  DeleteChromeRegistrationKeys(browser_dist, HKEY_CURRENT_USER, suffix,
+                               installer_state.target_path(), &ret);
+
+  // If the user's Chrome is registered with a suffix: it is possible that old
+  // unsuffixed registrations were left in HKCU (e.g. if this install was
+  // previously installed with no suffix in HKCU (old suffix rules) and later

[grt (UTC plus 2), 2012/05/30 20:37:40]

How would this happen in practice?  Would this happen if the user cancelled the
UAC prompt the first time, and then accepted it a second time, or something?

[gab, 2012/05/31 06:36:01]

Yea, if the user cancels the first UAC (now they'd get suffixed anyways, but
before they didn't) and later makes chrome default through the UI and accepts
UAC then (they will get suffixed, even in the current behavior its possible that
they get suffixed then if we realize the unsuffixed install is taken already).

[grt (UTC plus 2), 2012/06/04 02:18:26]

Cool, thanks for the explanation.  I think this is sufficiently non-obvious that
it's worth capturing in code comments somewhere.  What do you think?

[gab, 2012/06/05 19:50:02]

Added clarification as to the example of when this can happen. Let me know if
you think more details are needed.

+  // had to be suffixed when fully registered in HKLM).
+  // Remove remaining HKCU entries with no suffix if any.
+  if (!suffix.empty()) {
+    DeleteChromeRegistrationKeys(browser_dist, HKEY_CURRENT_USER, L"",
+                                 installer_state.target_path(), &ret);
+  }
 
   // Chrome is registered in HKLM for all system-level installs and for
   // user-level installs for which Chrome has been made the default browser.
   // Always remove the HKLM registration for system-level installs.  For
-  // user-level installs, only remove it if both: 1) this uninstall isn't a
-  // self-destruct following the installation of system-level Chrome (because
-  // the system-level Chrome owns the HKLM registration now), and 2) this user
-  // had made Chrome their default browser.
-  if (installer_state.system_install() ||
-      (remove_all &&
-       (!suffix.empty() || CurrentUserHasDefaultBrowser(installer_state)))) {
-    DeleteChromeRegistrationKeys(product.distribution(), HKEY_LOCAL_MACHINE,
-                                 suffix, installer_state.target_path(), &ret);
+  // user-level installs, only remove it if either: 1) The registration has a
+  // suffix, or 2) |remove_all| is true which means this is not a self-destruct
+  // following a system-level Chrome install (which would otherwise need the
+  // same HKLM registrations).
+  if (installer_state.system_install() || !suffix.empty() || remove_all) {

[grt (UTC plus 2), 2012/05/30 20:37:40]

If this user has not made her Chrome the default (suffix is empty) but another
user has (so there is an un-suffixed user-level Chrome registered in HKLM), this
will delete that other users' registrations, will it not?  Likewise, if there is
a system-level Chrome registered and this user is doing an ordinary uninstall
(not a self-destruct), then this will attempt to remove the system-level
Chrome's registrations.  The previous code was careful not to touch HKLM unless
it was truly this user's Chrome that was registered there.

[gab, 2012/05/31 06:36:01]

Ah right, good catch.

I now in fact think that |remove_all| is superfluous (now and in the current
code) as, if the system-level install is there, the current user no longer has
the unsuffixed HKLM registration... I left it there anyways for now, to be safe
as we don't EVER want to kill the new system-level install... (and |remove_all|
is a cheap check which we already know the value of before doing I/O to check
the current user's ownership status of the unsuffixed install I guess...)

+    DeleteChromeRegistrationKeys(browser_dist, HKEY_LOCAL_MACHINE, suffix,
+                                 installer_state.target_path(), &ret);
   }
 
   ProcessDelegateExecuteWorkItems(installer_state, product);