Add support for --multi-install to mini_installer, fixed a memory leak, and r...

chrome/installer/mini_installer/mini_installer.cc

Index: chrome/installer/mini_installer/mini_installer.cc
===================================================================
--- chrome/installer/mini_installer/mini_installer.cc	(revision 69125)
+++ chrome/installer/mini_installer/mini_installer.cc	(working copy)
@@ -127,6 +127,19 @@
   return true;
 }
 
+// Function to check if a string (specified by str) contains another string
+// (specified by sub_str).
+bool StrContains(const wchar_t* str, const wchar_t *sub_str) {

[tommi (sloooow) - chröme, 2010/12/15 20:15:43]

Use StrStr[I] instead?

wchar_t *
->
wchar_t*

[robertshield, 2010/12/15 20:25:45]

delete me (me as in this function, not me as in Robert).

[grt (UTC plus 2), 2010/12/16 05:59:13]

Replaced with a call to StrStr, thanks.

[grt (UTC plus 2), 2010/12/16 05:59:13]

Done.

+  if (str == NULL || sub_str == NULL)
+    return false;
+
+  for (const wchar_t* scan_str = str; *scan_str != L'\0'; ++scan_str) {
+    if (StrStartsWith(scan_str, sub_str))
+      return true;
+  }
+  return false;
+}
+
 // Helper function to read a value from registry. Returns true if value
 // is read successfully and stored in parameter value. Returns false otherwise.
 bool ReadValueFromRegistry(HKEY root_key, const wchar_t *sub_key,
@@ -144,46 +157,84 @@
   return false;
 }
 
-// This function sets the flag in registry to indicate that Google Update
-// should try full installer next time. If the current installer works, this
-// flag is cleared by setup.exe at the end of install. The flag will by default
-// be written to HKCU, but if --system-level is included in the command line,
-// it will be written to HKLM instead.
-void SetFullInstallerFlag() {
-  HKEY key;
-  wchar_t ap_registry_key[128];
+// Opens the Google Update ClientState key for a product.
+bool OpenClientStateKey(HKEY root_key, const wchar_t* app_guid, REGSAM access,
+                        HKEY* key) {
+  wchar_t client_state_key[128];
+
+  return SafeStrCopy(client_state_key, _countof(client_state_key),

[tommi (sloooow) - chröme, 2010/12/15 20:15:43]

chrome code typically uses arraysize().  Can we use that here?

[grt (UTC plus 2), 2010/12/16 05:59:13]

No due to dependency issues.

+                     kApRegistryKeyBase) &&
+         SafeStrCat(client_state_key, _countof(client_state_key), app_guid) &&
+         ::RegOpenKeyEx(root_key, client_state_key, NULL, access, key) ==
+             ERROR_SUCCESS;

[robertshield, 2010/12/15 20:25:45]

For readability, please include () around the last condition, as in:

[...]
(::RegOpenKeyEx(...) == ERROR_SUCCESS);

[grt (UTC plus 2), 2010/12/16 05:59:13]

Done.

+}
+
+// TODO(grt): Write a unit test for this that uses registry virtualization.
+void SetFullInstallerFlagHelper(int args_num, const wchar_t* const* args) {
+  bool multi_install = false;
+  HKEY key = 0;
   const wchar_t* app_guid = google_update::kAppGuid;
   HKEY root_key = HKEY_CURRENT_USER;
+  wchar_t value[128];

[tommi (sloooow) - chröme, 2010/12/15 20:17:28]

zero initialize

[grt (UTC plus 2), 2010/12/16 05:59:13]

The old code didn't, so I'm inclined not to add it.  Do you see a problem that
doing so would fix?

+  DWORD size = UINT_MAX;
+  LONG ret;
 
-  int args_num;
-  wchar_t* cmd_line = ::GetCommandLine();
-  wchar_t** args = ::CommandLineToArgvW(cmd_line, &args_num);
   for (int i = 1; i < args_num; ++i) {
     if (0 == ::lstrcmpi(args[i], L"--chrome-sxs"))
       app_guid = google_update::kSxSAppGuid;
     else if (0 == ::lstrcmpi(args[i], L"--chrome-frame"))
       app_guid = google_update::kChromeFrameAppGuid;
+    else if (0 == ::lstrcmpi(args[i], L"--multi-install"))
+      multi_install = true;
     else if (0 == ::lstrcmpi(args[i], L"--system-level"))
       root_key = HKEY_LOCAL_MACHINE;
   }
 
-  if (!SafeStrCopy(ap_registry_key, _countof(ap_registry_key),
-                   kApRegistryKeyBase) ||
-      !SafeStrCat(ap_registry_key, _countof(ap_registry_key),
-                  app_guid)) {
-    return;
+  // When multi_install is true, we are potentially:
+  // 1. Performing a multi-install of some product(s) on a clean machine.
+  //    Neither the product(s) nor the multi-installer will have a ClientState
+  //    key in the registry, so there is nothing to be done.
+  // 2. Upgrading an existing multi-install.  The multi-installer will have a
+  //    ClientState key in the registry.  Only it need be modified.
+  // 3. Migrating a single-install into a multi-install.  The product will have
+  //    a ClientState key in the registry.  Only it need be modified.

[tommi (sloooow) - chröme, 2010/12/15 20:15:43]

is there a 4? :

Two products are installed, one in multi, the other single.  The single will be
migrated as in case 3, and the multi will be upgraded as in case 2.

?

[grt (UTC plus 2), 2010/12/16 05:59:13]

In this case, the single will only be migrated when --multi-install is added to
its Google Update config, which is case 3.

+  // To handle all cases, we inspect the product's ClientState to see if it
+  // exists and its "ap" value does not contain "-multi".  This is case 3, so we
+  // modify the product's ClientState.  Otherwise, we check the
+  // multi-installer's ClientState and modify it if it exists.
+  if (multi_install) {
+    if (OpenClientStateKey(root_key, app_guid, KEY_READ | KEY_SET_VALUE,
+                           &key)) {
+      // The app is installed.  See if it's a single-install.
+      size = _countof(value);
+      ret = ::RegQueryValueEx(key, kApRegistryValueName, NULL, NULL,
+                              reinterpret_cast<LPBYTE>(value), &size);
+      if (ret == ERROR_FILE_NOT_FOUND) {
+        // case 3: add "-full" to this value.
+        *value = L'\0';
+      } else if (ret != ERROR_SUCCESS || StrContains(value, kMultiInstallTag)) {
+        // Error or case 2: add "-full" to the multi-installer's value.
+        ::RegCloseKey(key);
+        key = 0;
+        app_guid = google_update::kMultiInstallAppGuid;
+      }  // else case 3: add "-full" to this value.
+    } else {
+      // case 1 or 2: add "-full" to the multi-installer's value.
+      key = 0;
+      app_guid = google_update::kMultiInstallAppGuid;
+    }
   }
-  if (::RegOpenKeyEx(root_key, ap_registry_key, NULL,
-                     KEY_READ | KEY_SET_VALUE, &key) != ERROR_SUCCESS)
-    return;
 
-  wchar_t value[128];
-  size_t size = _countof(value);
-  size_t buf_size = size;
-  LONG ret = ::RegQueryValueEx(key, kApRegistryValueName, NULL, NULL,
-                               reinterpret_cast<LPBYTE>(value),
-                               reinterpret_cast<LPDWORD>(&size));
+  if (key == 0) {

[tommi (sloooow) - chröme, 2010/12/15 20:15:43]

key == NULL

[grt (UTC plus 2), 2010/12/16 05:59:13]

Done.

+    if (!OpenClientStateKey(root_key, app_guid, KEY_READ | KEY_SET_VALUE, &key))
+      return;
 
+    size = _countof(value);
+    ret = ::RegQueryValueEx(key, kApRegistryValueName, NULL, NULL,
+                            reinterpret_cast<LPBYTE>(value),
+                            reinterpret_cast<LPDWORD>(&size));
+  }
+
   // The conditions below are handling two cases:
   // 1. When ap key is present, we want to make sure it doesn't already end
   //    in -full and then append -full to it.
@@ -193,7 +244,7 @@
       value[0] = L'\0';
 
     if (!StrEndsWith(value, kFullInstallerSuffix) &&
-        (SafeStrCat(value, buf_size, kFullInstallerSuffix)))
+        (SafeStrCat(value, _countof(value), kFullInstallerSuffix)))
       ::RegSetValueEx(key, kApRegistryValueName, 0, REG_SZ,
                       reinterpret_cast<LPBYTE>(value),
                       lstrlen(value) * sizeof(wchar_t));
@@ -202,6 +253,21 @@
   ::RegCloseKey(key);
 }
 
+// This function sets the flag in registry to indicate that Google Update
+// should try full installer next time. If the current installer works, this
+// flag is cleared by setup.exe at the end of install. The flag will by default
+// be written to HKCU, but if --system-level is included in the command line,
+// it will be written to HKLM instead.
+void SetFullInstallerFlag() {
+  int args_num;
+  wchar_t* cmd_line = ::GetCommandLine();
+  wchar_t** args = ::CommandLineToArgvW(cmd_line, &args_num);
+
+  SetFullInstallerFlagHelper(args_num, args);
+
+  ::LocalFree(args);
+}
+
 // Gets the setup.exe path from Registry by looking the value of Uninstall
 // string, strips the arguments for uninstall and returns only the full path
 // to setup.exe.