Return Windows error code when create-process fails.

chrome/installer/mini_installer/mini_installer.cc

Index: chrome/installer/mini_installer/mini_installer.cc
diff --git a/chrome/installer/mini_installer/mini_installer.cc b/chrome/installer/mini_installer/mini_installer.cc
index acbdc260ac6c8ed2c744bb29a7e25897dc6d3d1f..2247f0999a1c6f7ff2b033933bd7acc6c02116ff 100644
--- a/chrome/installer/mini_installer/mini_installer.cc
+++ b/chrome/installer/mini_installer/mini_installer.cc
@@ -39,6 +39,8 @@ typedef DWORD ProcessExitCode;
 typedef StackString<MAX_PATH> PathString;
 typedef StackString<MAX_PATH * 4> CommandString;
 
+DWORD LastWindowsError = 0;

[grt (UTC plus 2), 2015/07/28 02:30:07]

can you do away with this? i don't think it's necessary to have a global
variable.

[bcwhite, 2015/07/28 13:14:01]

Yes, though the global was the easiest.  I could change all the return codes to
be 64-bits and combine both exit-code and last-error DWORDs into the single
value.  Or return a pair<>.  Any preference?

[grt (UTC plus 2), 2015/07/28 14:00:03]

Pair or a POD struct w/ value semantics both sound fine. A struct may be more
clear since the reader won't need to remember who's on first.

+
 // This structure passes data back and forth for the processing
 // of resource callbacks.
 struct Context {
@@ -64,17 +66,19 @@ class RegKey {
   // Returns true if a key is open.
   bool is_valid() const { return key_ != NULL; }
 
-  // Read a REG_SZ value from the registry into the memory indicated by |value|
+  // Read a value from the registry into the memory indicated by |value|
   // (of |value_size| wchar_t units).  Returns ERROR_SUCCESS,
   // ERROR_FILE_NOT_FOUND, ERROR_MORE_DATA, or some other error.  |value| is
   // guaranteed to be null-terminated on success.
-  LONG ReadValue(const wchar_t* value_name,
-                 wchar_t* value,
-                 size_t value_size) const;
+  LONG ReadSZValue(const wchar_t* value_name,
+                   wchar_t* value,
+                   size_t value_size) const;
+  LONG ReadDWValue(const wchar_t* value_name, DWORD* value) const;
 
-  // Write a REG_SZ value to the registry.  |value| must be null-terminated.
+  // Write a value to the registry.  SZ |value| must be null-terminated.
   // Returns ERROR_SUCCESS or an error code.
-  LONG WriteValue(const wchar_t* value_name, const wchar_t* value);
+  LONG WriteSZValue(const wchar_t* value_name, const wchar_t* value);
+  LONG WriteDWValue(const wchar_t* value_name, DWORD value);
 
   // Closes the key if it was open.
   void Close();
@@ -91,9 +95,9 @@ LONG RegKey::Open(HKEY key, const wchar_t* sub_key, REGSAM access) {
   return ::RegOpenKeyEx(key, sub_key, NULL, access, &key_);
 }
 
-LONG RegKey::ReadValue(const wchar_t* value_name,
-                       wchar_t* value,
-                       size_t value_size) const {
+LONG RegKey::ReadSZValue(const wchar_t* value_name,
+                         wchar_t* value,
+                         size_t value_size) const {
   DWORD type;
   DWORD byte_length = static_cast<DWORD>(value_size * sizeof(wchar_t));
   LONG result = ::RegQueryValueEx(key_, value_name, NULL, &type,
@@ -114,12 +118,34 @@ LONG RegKey::ReadValue(const wchar_t* value_name,
   return result;
 }
 
-LONG RegKey::WriteValue(const wchar_t* value_name, const wchar_t* value) {
+LONG RegKey::ReadDWValue(const wchar_t* value_name, DWORD* value) const {
+  DWORD type;
+  DWORD byte_length = sizeof(*value);
+  LONG result = ::RegQueryValueEx(key_, value_name, NULL, &type,
+                                  reinterpret_cast<BYTE*>(value),
+                                  &byte_length);
+  if (result == ERROR_SUCCESS) {
+    if (type != REG_DWORD) {
+      result = ERROR_NOT_SUPPORTED;
+    } else if (byte_length != sizeof(*value)) {
+      result = ERROR_NO_DATA;
+    }
+  }
+  return result;
+}
+
+LONG RegKey::WriteSZValue(const wchar_t* value_name, const wchar_t* value) {
   return ::RegSetValueEx(key_, value_name, 0, REG_SZ,
                          reinterpret_cast<const BYTE*>(value),
                          (lstrlen(value) + 1) * sizeof(wchar_t));
 }
 
+LONG RegKey::WriteDWValue(const wchar_t* value_name, DWORD value) {
+  return ::RegSetValueEx(key_, value_name, 0, REG_DWORD,
+                         reinterpret_cast<const BYTE*>(&value),
+                         sizeof(value));
+}
+
 void RegKey::Close() {
   if (key_ != NULL) {
     ::RegCloseKey(key_);
@@ -130,13 +156,13 @@ void RegKey::Close() {
 // Helper function to read a value from registry. Returns true if value
 // is read successfully and stored in parameter value. Returns false otherwise.
 // |size| is measured in wchar_t units.
-bool ReadValueFromRegistry(HKEY root_key, const wchar_t *sub_key,
-                           const wchar_t *value_name, wchar_t *value,
-                           size_t size) {
+bool ReadSZValueFromRegistry(HKEY root_key, const wchar_t *sub_key,
+                             const wchar_t *value_name, wchar_t *value,
+                             size_t size) {
   RegKey key;
 
   if (key.Open(root_key, sub_key, KEY_QUERY_VALUE) == ERROR_SUCCESS &&
-      key.ReadValue(value_name, value, size) == ERROR_SUCCESS) {
+      key.ReadSZValue(value_name, value, size) == ERROR_SUCCESS) {
     return true;
   }
   return false;
@@ -155,29 +181,26 @@ bool OpenClientStateKey(HKEY root_key, const wchar_t* app_guid, REGSAM access,
                     access | KEY_WOW64_32KEY) == ERROR_SUCCESS);
 }
 
-// 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.
-// TODO(grt): Write a unit test for this that uses registry virtualization.
-void SetInstallerFlags(const Configuration& configuration) {
-  RegKey key;
-  const REGSAM key_access = KEY_QUERY_VALUE | KEY_SET_VALUE;
+// Opens the Google Update ClientState key for the current install
+// configuration.  This includes locating the correct key in the face of
+// multi-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.
+// TODO(bcwhite): Write a unit test for this that uses registry virtualization.
+bool OpenInstallStateKey(const Configuration& configuration, RegKey* key) {

[grt (UTC plus 2), 2015/07/28 02:30:06]

the logic in here that figures out which app_guid to use to communicate with
Google Update could be moved into Configuration and performed during its
initialization. this would make it testable in configuration_test.cc
(setup_unittests), which is finally being run on the trybots and waterfall
builders. the one hiccup that i see is that OpenClientStateKey (and maybe other
things) would need to be moved into some sort of utility file, which means
adding new source(s) to the build target. this requires an extra step, but i
think it's worth it due to the simplification here and the testability (that
TODO can become a DONE without too much effort). Configuration::app_guid() could
be the accessor that returns the guid by which the installer is to interact with
Google Update.

[bcwhite, 2015/07/28 13:14:01]

Okay.  Any objection to RegKey becoming more sophisticated and moving those
methods into it within a new file?

   const HKEY root_key =
       configuration.is_system_level() ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER;
-  // This is ignored if multi-install is true.
   const wchar_t* app_guid =
       configuration.has_chrome_frame() ?
           google_update::kChromeFrameAppGuid :
           configuration.chrome_app_guid();
-  StackString<128> value;
+  const REGSAM key_access = KEY_QUERY_VALUE | KEY_SET_VALUE;
   LONG ret = ERROR_SUCCESS;
 
   // 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.
+  //    key in the registry, so there is no key to be modified.
   // 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
@@ -187,30 +210,64 @@ void SetInstallerFlags(const Configuration& configuration) {
   // modify the product's ClientState.  Otherwise, we check the
   // multi-installer's ClientState and modify it if it exists.
   if (configuration.is_multi_install()) {
-    if (OpenClientStateKey(root_key, app_guid, key_access, &key)) {
+    if (OpenClientStateKey(root_key, app_guid, key_access, key)) {
       // The product has a client state key.  See if it's a single-install.
-      ret = key.ReadValue(kApRegistryValue, value.get(), value.capacity());
-      if (ret != ERROR_FILE_NOT_FOUND &&
-          (ret != ERROR_SUCCESS ||
-           FindTagInStr(value.get(), kMultiInstallTag, NULL))) {
-        // Error or case 2: modify the multi-installer's value.
-        key.Close();
-        app_guid = google_update::kMultiInstallAppGuid;
-      }  // else case 3: modify this value.
-    } else {
-      // case 1 or 2: modify the multi-installer's value.
-      key.Close();
-      app_guid = google_update::kMultiInstallAppGuid;
+      StackString<128> value;
+      ret = key->ReadSZValue(kApRegistryValue, value.get(), value.capacity());
+      if (ret == ERROR_FILE_NOT_FOUND ||
+          (ret == ERROR_SUCCESS &&
+           !FindTagInStr(value.get(), kMultiInstallTag, NULL))) {
+        // yes -- case 3: modify this key.
+        return true;
+      }
     }
+    // error, case 1, or case 2: modify the multi-installer's key.
+    key->Close();
+    app_guid = google_update::kMultiInstallAppGuid;
   }
 
-  if (!key.is_valid()) {
-    if (!OpenClientStateKey(root_key, app_guid, key_access, &key))
-      return;
+  return OpenClientStateKey(root_key, app_guid, key_access, key);
+}
 
-    value.clear();
-    ret = key.ReadValue(kApRegistryValue, value.get(), value.capacity());
+// Writes install results into registry where it is read by Google Update.
+// Don't write anything if there is already a result present, likely
+// written by setup.exe.
+void WriteInstallResults(const Configuration& configuration,
+                         DWORD exit_code, DWORD last_error) {
+#if defined(GOOGLE_CHROME_BUILD)
+  // Calls to setup.exe will write a "success" result if everything was good
+  // so we don't need to write anything from here.
+  if (exit_code == ERROR_SUCCESS)
+    return;
+
+  RegKey key;
+  DWORD value;
+  if (OpenInstallStateKey(configuration, &key)) {
+    if (key.ReadDWValue(kInstallerResultRegistryValue, &value)

[grt (UTC plus 2), 2015/07/28 02:30:06]

this adds a dependency on an implementation detail of Google Update, which seems
risky. can this be rejiggered so that it doesn't check for existing values?

[bcwhite, 2015/07/28 13:14:01]

I don't think so.  We have to check if there is a non-success value present
before writing our own.  These keys and values, as well as their behavior, are
documented.  We could instead make the decision based on the exit-code of
setup.exe but that would also be an implementation detail and an undocumented
one at that.

[grt (UTC plus 2), 2015/07/28 14:00:04]

Why? Does it not suffice to suppress writing when setup.exe is successfully
launched and exits?
Google Update is not under our control. Who knows when it will decide to move
the values to LastFoo?
setup.exe is part of Chrome, so we can establish any contract we want here.

[bcwhite, 2015/07/28 14:52:59]

If we set a constraint that the MI isn't allowed to have any kind of (reported)
failure after RunSetup completes, then sure.  It's complicated, though, by the
fact that setup runs twice and the "don't report" flag needs to be stored
somewhere that is either global or passed around.
Does it matter?  In the extremely unlikely event that Omaha reports in between
when setup completes and MI completes, we would report two results.  Omaha would
either show one additional successful install or two separate failures, both of
which could provide useful information.

Even ignoring the fact that most updates are started by Omaha which presumably
waits for completion before reporting, the number of split results would so
small as to be statistically insignificant, on the order of 0.01% (assuming 8s
of MI activity after setup completes and completely random update/omaha runs
throughout the day).

I don't think it warrants any additional complexity in order to try to mitigate
that.

+            != ERROR_SUCCESS || value == 0) {
+      key.WriteDWValue(kInstallerResultRegistryValue,
+                       exit_code ? 1 /* FAILED_CUSTOM_ERROR */
+                                 : 0 /* SUCCESS */);
+      key.WriteDWValue(kInstallerErrorRegistryValue, exit_code);
+      key.WriteDWValue(kInstallerExtraCode1RegistryValue, last_error);
+    }
+    key.Close();
   }
+#endif
+}
+
+// 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.
+void SetInstallerFlags(const Configuration& configuration) {
+  RegKey key;
+  StackString<128> value;
+  LONG ret = ERROR_SUCCESS;
+
+  if (!OpenInstallStateKey(configuration, &key))
+    return;
+
+  ret = key.ReadSZValue(kApRegistryValue, value.get(), value.capacity());
 
   // The conditions below are handling two cases:
   // 1. When ap value is present, we want to add the required tag only if it is
@@ -223,7 +280,7 @@ void SetInstallerFlags(const Configuration& configuration) {
 
     if (!StrEndsWith(value.get(), kFullInstallerSuffix) &&
         value.append(kFullInstallerSuffix)) {
-      key.WriteValue(kApRegistryValue, value.get());
+      key.WriteSZValue(kApRegistryValue, value.get());
     }
   }
 }
@@ -238,7 +295,7 @@ ProcessExitCode GetSetupExePathForAppGuid(bool system_level,
   const HKEY root_key = system_level ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER;
   RegKey key;
   if (!OpenClientStateKey(root_key, app_guid, KEY_QUERY_VALUE, &key) ||
-      (key.ReadValue(kUninstallRegistryValue, path, size) != ERROR_SUCCESS)) {
+      (key.ReadSZValue(kUninstallRegistryValue, path, size) != ERROR_SUCCESS)) {
     return UNABLE_TO_FIND_REGISTRY_KEY;
   }
 
@@ -294,6 +351,7 @@ ProcessExitCode RunProcessAndWait(const wchar_t* exe_path, wchar_t* cmdline) {
   PROCESS_INFORMATION pi = {0};
   if (!::CreateProcess(exe_path, cmdline, NULL, NULL, FALSE, CREATE_NO_WINDOW,
                        NULL, NULL, &si, &pi)) {
+    LastWindowsError = ::GetLastError();
     return COULD_NOT_CREATE_PROCESS;
   }
 
@@ -301,8 +359,10 @@ ProcessExitCode RunProcessAndWait(const wchar_t* exe_path, wchar_t* cmdline) {
 
   ProcessExitCode exit_code = SUCCESS_EXIT_CODE;
   DWORD wr = ::WaitForSingleObject(pi.hProcess, INFINITE);
-  if (WAIT_OBJECT_0 != wr || !::GetExitCodeProcess(pi.hProcess, &exit_code))
+  if (WAIT_OBJECT_0 != wr || !::GetExitCodeProcess(pi.hProcess, &exit_code)) {

[grt (UTC plus 2), 2015/07/28 02:30:07]

please add a comment that this code assumes that wr == WAIT_FAILED if it !=
WAIT_OBJECT_0. i think that this is a safe assumption, but if it isn't we'll get
noise from GetLastError.

[bcwhite, 2015/07/28 13:14:01]

Done.

+    LastWindowsError = ::GetLastError();
     exit_code = WAIT_FOR_PROCESS_FAILED;
+  }
 
   ::CloseHandle(pi.hProcess);
 
@@ -451,8 +511,10 @@ ProcessExitCode UnpackBinaryResources(const Configuration& configuration,
   // is a problem in fetching B7 resource, just return an error.
   if (!::EnumResourceNames(module, kLZMAResourceType, OnResourceFound,
                            reinterpret_cast<LONG_PTR>(&context)) ||
-      archive_path->length() == 0)
+      archive_path->length() == 0) {
+    LastWindowsError = ::GetLastError();

[grt (UTC plus 2), 2015/07/28 02:30:07]

the last error code is generally not reset to ERROR_SUCCESS (0) when an
arbitrary Win32 call succeeds, so GetLastError shouldn't be called for cases
where EnumResourceNames succeeds but the expected resource wasn't there. it'll
provide random noise which we won't be able to distinguish from the actual
signal.

[bcwhite, 2015/07/28 13:14:01]

Done.

     return UNABLE_TO_EXTRACT_CHROME_ARCHIVE;
+  }
 
   ProcessExitCode exit_code = SUCCESS_EXIT_CODE;
 
@@ -501,8 +563,10 @@ ProcessExitCode UnpackBinaryResources(const Configuration& configuration,
   // (compressed setup).
   if (!::EnumResourceNames(module, kLZCResourceType, OnResourceFound,
                            reinterpret_cast<LONG_PTR>(&context)) &&
-      ::GetLastError() != ERROR_RESOURCE_TYPE_NOT_FOUND)
+      ::GetLastError() != ERROR_RESOURCE_TYPE_NOT_FOUND) {
+    LastWindowsError = ::GetLastError();
     return UNABLE_TO_EXTRACT_SETUP_B7;
+  }
 
   if (setup_path->length() > 0) {
     // Uncompress LZ compressed resource. Setup is packed with 'MSCF'
@@ -522,8 +586,10 @@ ProcessExitCode UnpackBinaryResources(const Configuration& configuration,
 #if defined(COMPONENT_BUILD)
     // Extract the (uncompressed) modules required by setup.exe.
     if (!::EnumResourceNames(module, kBinResourceType, WriteResourceToDirectory,
-                             reinterpret_cast<LONG_PTR>(base_path)))
+                             reinterpret_cast<LONG_PTR>(base_path))) {
+      LastWindowsError = ::GetLastError();
       return UNABLE_TO_EXTRACT_SETUP;
+    }
 #endif
 
     return exit_code;
@@ -535,8 +601,10 @@ ProcessExitCode UnpackBinaryResources(const Configuration& configuration,
   // it from create_installer_archive.py).
   if (!::EnumResourceNames(module, kBinResourceType, OnResourceFound,
                            reinterpret_cast<LONG_PTR>(&context)) &&
-      ::GetLastError() != ERROR_RESOURCE_TYPE_NOT_FOUND)
+      ::GetLastError() != ERROR_RESOURCE_TYPE_NOT_FOUND) {
+    LastWindowsError = ::GetLastError();
     return UNABLE_TO_EXTRACT_SETUP_BN;
+  }
 
   if (setup_path->length() > 0) {
     if (setup_path->comparei(setup_dest_path.get()) != 0) {
@@ -827,8 +895,8 @@ bool ProcessNonInstallOperations(const Configuration& configuration,
 // we continue to support it.
 bool ShouldDeleteExtractedFiles() {
   wchar_t value[2] = {0};
-  if (ReadValueFromRegistry(HKEY_CURRENT_USER, kCleanupRegistryKey,
-                            kCleanupRegistryValue, value, _countof(value)) &&
+  if (ReadSZValueFromRegistry(HKEY_CURRENT_USER, kCleanupRegistryKey,
+                              kCleanupRegistryValue, value, _countof(value)) &&
       value[0] == L'0') {
     return false;
   }
@@ -886,6 +954,7 @@ ProcessExitCode WMain(HMODULE module) {
   if (ShouldDeleteExtractedFiles())
     DeleteExtractedFiles(base_path.get(), archive_path.get(), setup_path.get());
 
+  WriteInstallResults(configuration, exit_code, LastWindowsError);
   return exit_code;
 }
 
@@ -894,6 +963,7 @@ ProcessExitCode WMain(HMODULE module) {
 int MainEntryPoint() {
   mini_installer::ProcessExitCode result =
       mini_installer::WMain(::GetModuleHandle(NULL));
+
   ::ExitProcess(result);
 }