mini_installer code cleanups (no functional changes).

chrome/installer/mini_installer/mini_installer.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // mini_installer.exe is the first exe that is run when chrome is being
 // installed or upgraded. It is designed to be extremely small (~5KB with no
 // extra resources linked) and it has two main jobs:
 //   1) unpack the resources (possibly decompressing some)
 //   2) run the real installer (setup.exe) with appropriate flags.
 //
 // In order to be really small the app doesn't link against the CRT and
 // defines the following compiler/linker flags:
 //   EnableIntrinsicFunctions="true" compiler: /Oi
 //   BasicRuntimeChecks="0"
 //   BufferSecurityCheck="false" compiler: /GS-
 //   EntryPointSymbol="MainEntryPoint" linker: /ENTRY
 //   IgnoreAllDefaultLibraries="true" linker: /NODEFAULTLIB
 //   OptimizeForWindows98="1"  liker: /OPT:NOWIN98
 //   linker: /SAFESEH:NO
 
 // have the linker merge the sections, saving us ~500 bytes.
 #pragma comment(linker, "/MERGE:.rdata=.text")
 
 #include <windows.h>
 #include <shellapi.h>
 
 #include "chrome/installer/mini_installer/appid.h"
 #include "chrome/installer/mini_installer/configuration.h"
 #include "chrome/installer/mini_installer/decompress.h"
-#include "chrome/installer/mini_installer/mini_installer.h"
+#include "chrome/installer/mini_installer/exit_code.h"
+#include "chrome/installer/mini_installer/mini_installer_constants.h"
 #include "chrome/installer/mini_installer/mini_string.h"
 #include "chrome/installer/mini_installer/pe_resource.h"
 
 namespace mini_installer {
 
+// The type of a process exit code.

[gab, 2014/07/07 13:45:01]

Remove this comment, the variable name is explicit enough.

[grt (UTC plus 2), 2014/07/07 14:24:06]

Done.

+typedef DWORD ProcessExitCode;
 typedef StackString<MAX_PATH> PathString;
 typedef StackString<MAX_PATH * 4> CommandString;
 
 // This structure passes data back and forth for the processing
 // of resource callbacks.
 struct Context {
   // Input to the call back method. Specifies the dir to save resources.
   const wchar_t* base_path;
   // First output from call back method. Full path of Chrome archive.
   PathString* chrome_resource_path;
@@ skipping 90 matching lines @@
       key.ReadValue(value_name, value, size) == ERROR_SUCCESS) {
     return true;
   }
   return false;
 }
 
 // Opens the Google Update ClientState key for a product.
 bool OpenClientStateKey(HKEY root_key, const wchar_t* app_guid, REGSAM access,
                         RegKey* key) {
   PathString client_state_key;
-  return client_state_key.assign(kApRegistryKeyBase) &&
+  return client_state_key.assign(kClientStateKeyBase) &&
          client_state_key.append(app_guid) &&
          (key->Open(root_key,
                     client_state_key.get(),
                     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,
@@ skipping 20 matching lines @@
   //    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.
   // 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 (configuration.is_multi_install()) {
     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(kApRegistryValueName, value.get(), value.capacity());
+      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;
     }
   }
 
   if (!key.is_valid()) {
     if (!OpenClientStateKey(root_key, app_guid, key_access, &key))
       return;
 
     value.clear();
-    ret = key.ReadValue(kApRegistryValueName, value.get(), value.capacity());
+    ret = key.ReadValue(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
   //    not present.
   // 2. When ap value is missing, we are going to create it with the required
   //    tag.
   if ((ret == ERROR_SUCCESS) || (ret == ERROR_FILE_NOT_FOUND)) {
     if (ret == ERROR_FILE_NOT_FOUND)
       value.clear();
 
     if (!StrEndsWith(value.get(), kFullInstallerSuffix) &&
         value.append(kFullInstallerSuffix)) {
-      key.WriteValue(kApRegistryValueName, value.get());
+      key.WriteValue(kApRegistryValue, value.get());
     }
   }
 }
 
 // Gets the setup.exe path from Registry by looking the value of Uninstall
 // string.  |size| is measured in wchar_t units.
 bool GetSetupExePathForGuidFromRegistry(bool system_level,
                                         const wchar_t* app_guid,
                                         wchar_t* path,
                                         size_t size) {
   const HKEY root_key = system_level ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER;
   RegKey key;
   return OpenClientStateKey(root_key, app_guid, KEY_QUERY_VALUE, &key) &&
-      (key.ReadValue(kUninstallRegistryValueName, path, size) == ERROR_SUCCESS);
+      (key.ReadValue(kUninstallRegistryValue, path, size) == ERROR_SUCCESS);
 }
 
 // Gets the setup.exe path from Registry by looking the value of Uninstall
 // string.  |size| is measured in wchar_t units.
 bool GetSetupExePathFromRegistry(const Configuration& configuration,
                                  wchar_t* path,
                                  size_t size) {
   bool system_level = configuration.is_system_level();
 
   // If this is a multi install, first try looking in the binaries for the path.
@@ skipping 16 matching lines @@
     return true;
   }
   if (configuration.has_app_host() && GetSetupExePathForGuidFromRegistry(
           system_level, google_update::kChromeAppHostAppGuid, path, size)) {
     return true;
   }
 
   return false;
 }
 
-// Calls CreateProcess with good default parameters and waits for the process
-// to terminate returning the process exit code.
+// Calls CreateProcess with good default parameters and waits for the process to
+// terminate returning the process exit code. |exit_code|, if non-NULL, is
+// populated with the process exit code.
 bool RunProcessAndWait(const wchar_t* exe_path, wchar_t* cmdline,
-                       int* exit_code) {
+                       ProcessExitCode* exit_code) {
   STARTUPINFOW si = {sizeof(si)};
   PROCESS_INFORMATION pi = {0};
   if (!::CreateProcess(exe_path, cmdline, NULL, NULL, FALSE, CREATE_NO_WINDOW,
                        NULL, NULL, &si, &pi)) {
     return false;
   }
 
   ::CloseHandle(pi.hThread);
 
   bool ret = true;
   DWORD wr = ::WaitForSingleObject(pi.hProcess, INFINITE);
   if (WAIT_OBJECT_0 != wr) {
     ret = false;
   } else if (exit_code) {
-    if (!::GetExitCodeProcess(pi.hProcess,
-                              reinterpret_cast<DWORD*>(exit_code))) {
+    if (!::GetExitCodeProcess(pi.hProcess, exit_code))
       ret = false;
-    }
   }
 
   ::CloseHandle(pi.hProcess);
 
   return ret;
 }
 
 // Append any command line params passed to mini_installer to the given buffer
 // so that they can be passed on to setup.exe. We do not return any error from
 // this method and simply skip making any changes in case of error.
@@ skipping 41 matching lines @@
       (resource.Size() > kMaxResourceSize)) {
     return FALSE;
   }
 
   PathString full_path;
   if (!full_path.assign(ctx->base_path) ||
       !full_path.append(name) ||
       !resource.WriteToDisk(full_path.get()))
     return FALSE;
 
-  if (StrStartsWith(name, kChromePrefix)) {
+  if (StrStartsWith(name, kChromeArchivePrefix)) {
     if (!ctx->chrome_resource_path->assign(full_path.get()))
       return FALSE;
   } else if (StrStartsWith(name, kSetupPrefix)) {
     if (!ctx->setup_resource_path->assign(full_path.get()))
       return FALSE;
   } else {
     // Resources should either start with 'chrome' or 'setup'. We don't handle
     // anything else.
     return FALSE;
   }
 
   return TRUE;
 }
 
 // Finds and writes to disk resources of various types. Returns false
 // if there is a problem in writing any resource to disk. setup.exe resource
 // can come in one of three possible forms:
 // - Resource type 'B7', compressed using LZMA (*.7z)
 // - Resource type 'BL', compressed using LZ (*.ex_)
 // - Resource type 'BN', uncompressed (*.exe)
 // If setup.exe is present in more than one form, the precedence order is
 // BN < BL < B7
 // For more details see chrome/tools/build/win/create_installer_archive.py.
 bool UnpackBinaryResources(const Configuration& configuration, HMODULE module,
                            const wchar_t* base_path, PathString* archive_path,
                            PathString* setup_path) {
   // Generate the setup.exe path where we patch/uncompress setup resource.
   PathString setup_dest_path;
   if (!setup_dest_path.assign(base_path) ||
-      !setup_dest_path.append(kSetupName))
+      !setup_dest_path.append(kSetupExe))
     return false;
 
   // Prepare the input to OnResourceFound method that needs a location where
   // it will write all the resources.
   Context context = {
     base_path,
     archive_path,
     setup_path,
   };
 
   // Get the resources of type 'B7' (7zip archive).
   // We need a chrome archive to do the installation. So if there
   // 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)
     return false;
 
   // If we found setup 'B7' resource, handle it.
   if (setup_path->length() > 0) {
     CommandString cmd_line;
     // Get the path to setup.exe first.
     bool success = true;
     if (!GetSetupExePathFromRegistry(configuration, cmd_line.get(),
                                      cmd_line.capacity()) ||
+        !cmd_line.append(L" --") ||
         !cmd_line.append(kCmdUpdateSetupExe) ||
         !cmd_line.append(L"=\"") ||
         !cmd_line.append(setup_path->get()) ||
-        !cmd_line.append(L"\"") ||
+        !cmd_line.append(L"\" --") ||
         !cmd_line.append(kCmdNewSetupExe) ||
         !cmd_line.append(L"=\"") ||
         !cmd_line.append(setup_dest_path.get()) ||
         !cmd_line.append(L"\"")) {

[gab, 2014/07/07 13:45:01]

Use the CommandLine API to build the command line string rather than building
the string manually?

[grt (UTC plus 2), 2014/07/07 14:24:06]

base/ is not available here.

       success = false;
     }
 
     // Get any command line option specified for mini_installer and pass them
     // on to setup.exe.  This is important since switches such as
     // --multi-install and --chrome-frame affect where setup.exe will write
     // installer results for consumption by Google Update.
     AppendCommandLineFlags(configuration, &cmd_line);
 
-    int exit_code = 0;
+    ProcessExitCode exit_code = SUCCESS_EXIT_CODE;
     if (success &&
         (!RunProcessAndWait(NULL, cmd_line.get(), &exit_code) ||
-         exit_code != ERROR_SUCCESS)) {
+         exit_code != SUCCESS_EXIT_CODE)) {
       success = false;
     }
 
     if (!success)
       DeleteFile(setup_path->get());
 
     return success && setup_path->assign(setup_dest_path.get());
   }
 
   // setup.exe wasn't sent as 'B7', lets see if it was sent as 'BL'
@@ skipping 38 matching lines @@
         ::DeleteFile(setup_dest_path.get());
       }
     }
   }
 
   return setup_path->length() > 0;
 }
 
 // Executes setup.exe, waits for it to finish and returns the exit code.
 bool RunSetup(const Configuration& configuration, const wchar_t* archive_path,
-              const wchar_t* setup_path, int* exit_code) {
+              const wchar_t* setup_path, ProcessExitCode* exit_code) {
   // There could be three full paths in the command line for setup.exe (path
   // to exe itself, path to archive and path to log file), so we declare
   // total size as three + one additional to hold command line options.
   CommandString cmd_line;
 
   // Get the path to setup.exe first.
   if (::lstrlen(setup_path) > 0) {
     if (!cmd_line.assign(L"\"") ||
         !cmd_line.append(setup_path) ||
         !cmd_line.append(L"\""))
       return false;
   } else if (!GetSetupExePathFromRegistry(configuration, cmd_line.get(),
                                           cmd_line.capacity())) {
     return false;
   }
 
   // Append the command line param for chrome archive file
-  if (!cmd_line.append(kCmdInstallArchive) ||
+  if (!cmd_line.append(L" --") ||
+      !cmd_line.append(kCmdInstallArchive) ||
       !cmd_line.append(L"=\"") ||
       !cmd_line.append(archive_path) ||
       !cmd_line.append(L"\""))

[gab, 2014/07/07 13:45:01]

Use CommandLine API here too.

[grt (UTC plus 2), 2014/07/07 14:24:06]

same comment

     return false;
 
   // Get any command line option specified for mini_installer and pass them
   // on to setup.exe
   AppendCommandLineFlags(configuration, &cmd_line);
 
   return RunProcessAndWait(NULL, cmd_line.get(), exit_code);
 }
 
 // Deletes given files and working dir.
@@ skipping 185 matching lines @@
   for (int i = 0; i < arraysize(kDirectoryPrefixes); ++i) {
     DeleteDirectoriesWithPrefix(temp.get(), kDirectoryPrefixes[i]);
   }
 }
 
 // Checks the command line for specific mini installer flags.
 // If the function returns true, the command line has been processed and all
 // required actions taken.  The installer must exit and return the returned
 // |exit_code|.
 bool ProcessNonInstallOperations(const Configuration& configuration,
-                                 int* exit_code) {
+                                 ProcessExitCode* exit_code) {
   bool ret = false;
 
   switch (configuration.operation()) {
     case Configuration::CLEANUP:
       // Cleanup has already taken place in DeleteOldChromeTempDirectories at
       // this point, so just tell our caller to exit early.
-      *exit_code = 0;
+      *exit_code = SUCCESS_EXIT_CODE;
       ret = true;
       break;
 
     default: break;
   }
 
   return ret;
 }
 
 // Returns true if we should delete the temp files we create (default).
 // Returns false iff the user has manually created a ChromeInstallerCleanup
 // string value in the registry under HKCU\\Software\\[Google|Chromium]
 // and set its value to "0".  That explicitly forbids the mini installer from
 // deleting these files.
 // Support for this has been publicly mentioned in troubleshooting tips so
 // we continue to support it.
 bool ShouldDeleteExtractedFiles() {
   wchar_t value[2] = {0};
   if (ReadValueFromRegistry(HKEY_CURRENT_USER, kCleanupRegistryKey,
-                            kCleanupRegistryValueName, value,
-                            arraysize(value)) &&
+                            kCleanupRegistryValue, value, arraysize(value)) &&
       value[0] == L'0') {
     return false;
   }
 
   return true;
 }
 
 // Main function. First gets a working dir, unpacks the resources and finally
 // executes setup.exe to do the install/upgrade.
-int WMain(HMODULE module) {
+ProcessExitCode WMain(HMODULE module) {
 #if defined(COMPONENT_BUILD)
   if (::GetEnvironmentVariable(L"MINI_INSTALLER_TEST", NULL, 0) == 0) {
     static const wchar_t kComponentBuildIncompatibleMessage[] =
         L"mini_installer.exe is incompatible with the component build, please"
         L" run setup.exe with the same command line instead. See"
         L" http://crbug.com/127233#c17 for details.";
     ::MessageBox(NULL, kComponentBuildIncompatibleMessage, NULL, MB_ICONERROR);
-    return 1;
+    return GENERIC_ERROR;
   }
 #endif
 
   // Always start with deleting potential leftovers from previous installations.
   // This can make the difference between success and failure.  We've seen
   // many installations out in the field fail due to out of disk space problems
   // so this could buy us some space.
   DeleteOldChromeTempDirectories();
 
   // TODO(grt): Make the exit codes more granular so we know where the popular
   // errors truly are.
-  int exit_code = 101;
+  ProcessExitCode exit_code = GENERIC_INITIALIZATION_FAILURE;
 
   // Parse the command line.
   Configuration configuration;
   if (!configuration.Initialize())
     return exit_code;
 
   if (configuration.query_component_build()) {
     // Exit immediately with an exit code of 1 to indicate component build and 0
     // to indicate static build. This is used by the tests in
     // /src/chrome/test/mini_installer/.
 #if defined(COMPONENT_BUILD)
-    return 1;
+    return GENERIC_ERROR;

[gab, 2014/07/07 13:45:01]

This doesn't match what the comment above says (i.e. this is a signal about
component_build, not an error -- I agree that 0/1 are inappropriate choices of
error codes for this, please introduce specific error codes for this).

[grt (UTC plus 2), 2014/07/07 14:24:06]

0/1 are the generic success/error codes for any executable, so the names do
match what the values traditionally mean. i've updated the comment accordingly.

 #else
-    return 0;
+    return SUCCESS_EXIT_CODE;
 #endif
   }
 
   // If the --cleanup switch was specified on the command line, then that means
   // we should only do the cleanup and then exit.
   if (ProcessNonInstallOperations(configuration, &exit_code))
     return exit_code;
 
   // First get a path where we can extract payload
   PathString base_path;
   if (!GetWorkDir(module, &base_path))
-    return 101;
+    return GENERIC_INITIALIZATION_FAILURE;
 
 #if defined(GOOGLE_CHROME_BUILD)
   // Set the magic suffix in registry to try full installer next time. We ignore
   // any errors here and we try to set the suffix for user level unless
   // --system-level is on the command line in which case we set it for system
   // level instead. This only applies to the Google Chrome distribution.
   SetInstallerFlags(configuration);
 #endif
 
   PathString archive_path;
   PathString setup_path;
   if (!UnpackBinaryResources(configuration, module, base_path.get(),
                              &archive_path, &setup_path)) {
-    exit_code = 102;
+    exit_code = GENERIC_UNPACKING_FAILURE;
   } else {
     // While unpacking the binaries, we paged in a whole bunch of memory that
     // we don't need anymore.  Let's give it back to the pool before running
     // setup.
     ::SetProcessWorkingSetSize(::GetCurrentProcess(), -1, -1);
     if (!RunSetup(configuration, archive_path.get(), setup_path.get(),
                   &exit_code)) {
-      exit_code = 103;
+      exit_code = GENERIC_SETUP_FAILURE;
     }
   }
 
   if (ShouldDeleteExtractedFiles())
     DeleteExtractedFiles(base_path.get(), archive_path.get(), setup_path.get());
 
   return exit_code;
 }
 
 }  // namespace mini_installer
 
 int MainEntryPoint() {
-  int result = mini_installer::WMain(::GetModuleHandle(NULL));
+  mini_installer::ProcessExitCode result =
+      mini_installer::WMain(::GetModuleHandle(NULL));
   ::ExitProcess(result);
 }
 
 // VC Express editions don't come with the memset CRT obj file and linking to
 // the obj files between versions becomes a bit problematic. Therefore,
 // simply implement memset.
 //
 // This also avoids having to explicitly set the __sse2_available hack when
 // linking with both the x64 and x86 obj files which is required when not
 // linking with the std C lib in certain instances (including Chromium) with
 // MSVC.  __sse2_available determines whether to use SSE2 intructions with
 // std C lib routines, and is set by MSVC's std C lib implementation normally.
 extern "C" {
 #pragma function(memset)
 void* memset(void* dest, int c, size_t count) {
   void* start = dest;
   while (count--) {
     *reinterpret_cast<char*>(dest) = static_cast<char>(c);
     dest = reinterpret_cast<char*>(dest) + 1;
   }
   return start;
 }
 }  // extern "C"