Add crash keys for the installer covering simple InstallerState fields.

chrome/installer/setup/setup_main.cc

Index: chrome/installer/setup/setup_main.cc
diff --git a/chrome/installer/setup/setup_main.cc b/chrome/installer/setup/setup_main.cc
index c3c575a8a7b87c8b30a1d06cda15428b5cca8b3c..0c7af260ca3dcfdd8fc88cbc0ef1d581a75ccfc9 100644
--- a/chrome/installer/setup/setup_main.cc
+++ b/chrome/installer/setup/setup_main.cc
@@ -1334,6 +1334,92 @@ void ConfigureCrashReporting(const InstallerState& installer_state) {
   // setting it here for Chrome to use.
 }
 
+void SetStartingCrashKeys(const InstallerState& installer_state,

[grt (UTC plus 2), 2015/11/25 16:44:09]

wdyt of moving this and the impl of
InstallerCrashReporterClient::RegisterCrashKeys() out into an
installer_crash_keys.{cc,h} pair? this file can expose helper functions called
at various points of installation to set/clear crash keys as appropriate. this
mirrors chrome/common/crash_keys.cc a bit, so it will also help keep the
installer's handling of crash keys more conceptually similar to Chrome's.

[Joe Mason, 2015/11/25 17:16:34]

Good idea. It will help with the more invasive keys like AP value. I'll do it in
a followup when I implement that.

+                          const InstallationState& original_state)
+{
+  scoped_ptr<base::Version> current_version(

[grt (UTC plus 2), 2015/11/25 16:44:09]

CheckPreInstallConditions mutates |installer_state|, so GetCurrentVersion should
not be pulled from it until after that func is called in InstallProducts.

[Joe Mason, 2015/11/25 20:12:52]

installer_state is also mutated by HandleNonInstallCmdLineOptions and
UninstallMultiChromeFrameIfPresent. Do you think it's worth it to set the
current version key at startup and then update it with each mutation?

[grt (UTC plus 2), 2015/11/25 20:34:30]

I think it would be fine to set this crash key within install.cc's
InstallNewVersion when GetCurrentVersion is ordinarily called. I don't think
it's necessary to repeatedly update this particular key.

+      installer_state.GetCurrentVersion(original_state));
+  if (current_version) {
+    base::debug::SetCrashKeyValue(installer::crash_keys::kCurrentVersion,
+                                  current_version->GetString());
+  }
+
+  switch (installer_state.state_type()) {

[grt (UTC plus 2), 2015/11/25 16:44:09]

i think this is cleaner like so:

const char* DistributionTypeToString(BrowserDistribution::Type type) {
  switch (type) {
    case BrowserDistribution::CHROME_BROWSER:
      return "chrome browser";
    case BrowserDistribution::CHROME_FRAME:
      return "chrome frame";
    case BrowserDistribution::CHROME_BINARIES:
      return "chrome binaries";
    case BrowserDistribution::NUM_TYPES:
      break;
  }
  NOTREACHED();
  return "";
}

void SetStartingCrashKeys(...)
{
  base::debug::SetCrashKeyValue(
      installer::crash_keys::kDistributionType,
      DistributionTypeToString(installer_state.state_type()));
}

[Joe Mason, 2015/11/25 17:16:34]

What do you think of making this a BrowserDistribution method?

[grt (UTC plus 2), 2015/11/25 17:44:01]

I think of this mapping being specific to the crash keys, so I think it belongs
with the crash key impl.

+    case BrowserDistribution::CHROME_BROWSER:
+      base::debug::SetCrashKeyValue(installer::crash_keys::kDistributionType,
+                                    "browser");
+      break;
+    case BrowserDistribution::CHROME_FRAME:
+      base::debug::SetCrashKeyValue(installer::crash_keys::kDistributionType,
+                                    "frame");
+      break;
+    case BrowserDistribution::CHROME_BINARIES:
+      base::debug::SetCrashKeyValue(installer::crash_keys::kDistributionType,
+                                    "binaries");
+      break;
+    case BrowserDistribution::NUM_TYPES:
+      // Not a real enum value.
+      assert(false);

[grt (UTC plus 2), 2015/11/25 16:44:09]

use NOTREACHED() (to crash debug builds) or CHECK(FALSE) (to crash debug and
release builds) rather than assert. CHECK(FALSE) should generally be used
sparingly for things that represent security vulns where continuing execution
would be bad. in this case, the invalid value should never happen in practice,
so NOTREACHED() is the right choice.

+      break;
+  }
+
+  switch (installer_state.operation()) {
+    case InstallerState::SINGLE_INSTALL_OR_UPDATE:
+      base::debug::SetCrashKeyValue(installer::crash_keys::kOperation,
+                                    "single-install-or-update");
+      break;
+    case InstallerState::MULTI_INSTALL:
+      base::debug::SetCrashKeyValue(installer::crash_keys::kOperation,
+                                    "multi-install");
+      break;
+    case InstallerState::MULTI_UPDATE:
+      base::debug::SetCrashKeyValue(installer::crash_keys::kOperation,
+                                    "multi-update");
+      break;
+    case InstallerState::UNINSTALL:
+      base::debug::SetCrashKeyValue(installer::crash_keys::kOperation,
+                                    "uninstall");
+      break;
+    case InstallerState::UNINITIALIZED:
+      // Do nothing.
+      break;
+  }
+
+  switch (installer_state.package_type()) {
+    case InstallerState::SINGLE_PACKAGE:
+      base::debug::SetCrashKeyValue(installer::crash_keys::kPackageType,
+                                    "single");
+      break;
+    case InstallerState::MULTI_PACKAGE:
+      base::debug::SetCrashKeyValue(installer::crash_keys::kPackageType,
+                                    "multi");
+      break;
+    case InstallerState::UNKNOWN_PACKAGE_TYPE:
+      // Do nothing.
+      break;
+  }
+
+  switch (installer_state.level()) {
+    case InstallerState::USER_LEVEL:
+      base::debug::SetCrashKeyValue(installer::crash_keys::kPrivilegeLevel,
+                                    "user");
+      break;
+    case InstallerState::SYSTEM_LEVEL:
+      base::debug::SetCrashKeyValue(installer::crash_keys::kPrivilegeLevel,
+                                    "system");
+      break;
+    case InstallerState::UNKNOWN_LEVEL:
+      // Do nothing.
+      break;
+  }
+
+  const std::wstring state_key = installer_state.state_key();
+  if (!state_key.empty()) {
+    base::debug::SetCrashKeyValue(installer::crash_keys::kStateKey,
+                                  base::UTF16ToUTF8(state_key));
+  }
+}
+
 // Uninstalls multi-install Chrome Frame if the current operation is a
 // multi-install install or update. The operation is performed directly rather
 // than delegated to the existing install since there is no facility in older
@@ -1705,6 +1791,7 @@ int WINAPI wWinMain(HINSTANCE instance, HINSTANCE prev_instance,
   installer_state.Initialize(cmd_line, prefs, original_state);
 
   ConfigureCrashReporting(installer_state);
+  SetStartingCrashKeys(installer_state, original_state);
 
   // Make sure the process exits cleanly on unexpected errors.
   base::EnableTerminationOnHeapCorruption();