Add UMA metrics to recovery component.

chrome/browser/component_updater/recovery_component_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.
 
 #include "chrome/browser/component_updater/recovery_component_installer.h"
 
 #include <stdint.h>
 #include <string>
 
 #include "base/base_paths.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/json/json_file_value_serializer.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/metrics/histogram.h"
 #include "base/path_service.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
 #include "base/process/kill.h"
 #include "base/process/launch.h"
 #include "base/process/process.h"
 #include "base/threading/worker_pool.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "components/component_updater/component_updater_paths.h"
@@ skipping 23 matching lines @@
 
 const char kRecoveryManifestName[] = "ChromeRecovery";
 
 // ChromeRecovery process exit codes.
 enum ChromeRecoveryExitCode {
   EXIT_CODE_RECOVERY_SUCCEEDED = 0,
   EXIT_CODE_RECOVERY_SKIPPED = 1,
   EXIT_CODE_ELEVATION_NEEDED = 2,
 };
 
+enum RecoveryComponentEvent {

[gab, 2015/01/09 20:08:44]

If you make this an enum class (woot C++11!!) then you can explicitly use
RecoveryComponentEvent::RUN_NON_ELEVATED, etc. everywhere rather than polluting
this namespace and have to use RCE_ prefixes.

I prefer this, more explicit and less namespace pollution.

[robertshield, 2015/01/09 21:37:21]

Sadly without the implicit conversion to int, that would require explicit casts
to play nicely with the UMA_HISTOGRAM macros afaict. Will leave as is unless you
know of a nice fix for this.

[gab, 2015/01/09 22:41:22]

Hmmm, really? I was sure I'd done this in the past, but maybe not...

[robertshield, 2015/01/09 22:45:32]

I tried it and it complained about requiring a cast to convert between
RecoveryComponentEvent and the type required. Could do this, but that is uglier
than the current solution. 
Also,
http://stackoverflow.com/questions/8357240/how-to-automatically-convert-stron...

[gab, 2015/01/12 19:12:20]

Ah ok, right, for some reason I thought I'd done this, but now that I think
further what I have is a switch based on a enum class and then the histogram
themselves don't report the enumeration value itself but a specific histogram
(w/, e.g., a count) for each value.

[robertshield, 2015/01/12 23:04:19]

Acknowledged.

+  RCE_RUN_NON_ELEVATED = 0,
+  RCE_ELEVATION_NEEDED = 1,
+  RCE_RUN_ELEVATED = 2,
+  RCE_COMPONENT_DOWNLOAD_ERROR = 3,
+  RCE_MAX = 4

[gab, 2015/01/09 20:08:44]

Don't explicitly label the max.

(I also prefer to call it _COUNT or _END than _MAX -- as _MAX to me sounds like
it's the value of the last item, not a past-the-end value).

[robertshield, 2015/01/09 21:37:21]

Done.

+};
+
 #if !defined(OS_CHROMEOS)
 // Checks if elevated recovery simulation switch was present on the command
 // line. This is for testing purpose.
 bool SimulatingElevatedRecovery() {
   return base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kSimulateElevatedRecovery);
 }
-#endif
+#endif  // !defined(OS_CHROMEOS)
 
 #if defined(OS_WIN)
 scoped_ptr<base::DictionaryValue> ReadManifest(const base::FilePath& manifest) {
   JSONFileValueSerializer serializer(manifest);
   std::string error;
   scoped_ptr<base::Value> root(serializer.Deserialize(NULL, &error));
   if (root.get() && root->IsType(base::Value::TYPE_DICTIONARY)) {
     return scoped_ptr<base::DictionaryValue>(
         static_cast<base::DictionaryValue*>(root.release()));
   }
@@ skipping 21 matching lines @@
   base::CommandLine cmdline(main_file);
   std::string arguments;
   if (manifest->GetStringASCII("x-recovery-args", &arguments))
     cmdline.AppendArg(arguments);
   std::string add_version;
   if (manifest->GetStringASCII("x-recovery-add-version", &add_version) &&
       add_version == "yes") {
     cmdline.AppendSwitchASCII("version", version.GetString());
   }
 
+  UMA_HISTOGRAM_ENUMERATION("RecoveryComponent.Event", RCE_RUN_ELEVATED,
+                            RCE_MAX);

[gab, 2015/01/09 20:08:44]

This is slightly misleading, this event doesn't actually mean that it was *ran*
elevated, only that the UAC was brought up, right?

Perhaps we should log an attempt here and then another event when succeeding
(i.e. we get a handle from base::LaunchElevatedProcess()?), this would also help
quantify how bad the UAC really is?

[robertshield, 2015/01/09 21:37:21]

Right, the intent of this patch initially was to record what Chrome thought it
was trying to do, not whether the recovery component actually worked. 

On further reflection, why not do both. Expanded CL accordingly.

+
   base::LaunchOptions options;
   options.start_hidden = true;
   base::LaunchElevatedProcess(cmdline, options);
 }
 
 void ElevatedInstallRecoveryComponent(const base::FilePath& installer_path) {
   base::WorkerPool::PostTask(
       FROM_HERE,
       base::Bind(&DoElevatedInstallRecoveryComponent, installer_path),
       true);
 }
-#endif
+#endif  // defined(OS_WIN)
 
 }  // namespace
 
 // Component installer that is responsible to repair the chrome installation
 // or repair the Google update installation. This is a last resort safety
 // mechanism.
 // For user Chrome, recovery component just installs silently. For machine
 // Chrome, elevation may be needed. If that happens, the installer will set
 // preference flag prefs::kRecoveryComponentNeedsElevation to request that.
 // There is a global error service monitors this flag and will pop up
@@ skipping 54 matching lines @@
   prefs->SetBoolean(prefs::kRecoveryComponentNeedsElevation, true);
 }
 
 RecoveryComponentInstaller::RecoveryComponentInstaller(const Version& version,
                                                        PrefService* prefs)
     : current_version_(version), prefs_(prefs) {
   DCHECK(version.IsValid());
 }
 
 void RecoveryComponentInstaller::OnUpdateError(int error) {
+  UMA_HISTOGRAM_ENUMERATION("RecoveryComponent.Event",
+                            RCE_COMPONENT_DOWNLOAD_ERROR, RCE_MAX);
   NOTREACHED() << "Recovery component update error: " << error;
 }
 
 #if defined(OS_WIN)
 void WaitForInstallToComplete(base::Process process,
                               const base::FilePath& installer_folder,
                               PrefService* prefs) {
   int installer_exit_code = 0;
   const base::TimeDelta kMaxWaitTime = base::TimeDelta::FromSeconds(600);
   if (process.WaitForExitWithTimeout(kMaxWaitTime, &installer_exit_code) &&
       installer_exit_code == EXIT_CODE_ELEVATION_NEEDED) {
+    UMA_HISTOGRAM_ENUMERATION("RecoveryComponent.Event", RCE_ELEVATION_NEEDED,
+                              RCE_MAX);
+
     BrowserThread::PostTask(
         BrowserThread::UI,
         FROM_HERE,
         base::Bind(&SetPrefsForElevatedRecoveryInstall,
                    installer_folder,
                    prefs));
   }
 }
 
 bool RecoveryComponentInstaller::RunInstallCommand(
     const base::CommandLine& cmdline,
     const base::FilePath& installer_folder) const {
+  UMA_HISTOGRAM_ENUMERATION("RecoveryComponent.Event", RCE_RUN_NON_ELEVATED,
+                            RCE_MAX);
+
   base::LaunchOptions options;
   options.start_hidden = true;
   base::Process process = base::LaunchProcess(cmdline, options);
   if (!process.IsValid())
     return false;
 
   // Let worker pool thread wait for us so we don't block Chrome shutdown.
   base::WorkerPool::PostTask(
       FROM_HERE,
       base::Bind(&WaitForInstallToComplete,
                  base::Passed(&process), installer_folder, prefs_),
       true);
 
   // Returns true regardless of install result since from updater service
   // perspective the install is done, even we may need to do elevated
   // install later.
   return true;
 }
 #else
 bool RecoveryComponentInstaller::RunInstallCommand(
     const base::CommandLine& cmdline,
     const base::FilePath&) const {
   return base::LaunchProcess(cmdline, base::LaunchOptions()).IsValid();
 }
-#endif
+#endif  // defined(OS_WIN)
 
 bool RecoveryComponentInstaller::Install(const base::DictionaryValue& manifest,
                                          const base::FilePath& unpack_path) {
   std::string name;
   manifest.GetStringASCII("name", &name);
   if (name != kRecoveryManifestName)
     return false;
   std::string proposed_version;
   manifest.GetStringASCII("version", &proposed_version);
   Version version(proposed_version.c_str());
@@ skipping 60 matching lines @@
         base::Bind(&SimulateElevatedRecoveryHelper, prefs));
   }
 
   // We delay execute the registration because we are not required in
   // the critical path during browser startup.
   BrowserThread::PostDelayedTask(
       BrowserThread::UI,
       FROM_HERE,
       base::Bind(&RecoveryRegisterHelper, cus, prefs),
       base::TimeDelta::FromSeconds(6));
-#endif
+#endif  // !defined(OS_CHROMEOS)
 }
 
 void RegisterPrefsForRecoveryComponent(PrefRegistrySimple* registry) {
   registry->RegisterStringPref(prefs::kRecoveryComponentVersion, "0.0.0.0");
   registry->RegisterFilePathPref(prefs::kRecoveryComponentUnpackPath,
                                  base::FilePath());
   registry->RegisterBooleanPref(prefs::kRecoveryComponentNeedsElevation, false);
 }
 
 void AcceptedElevatedRecoveryInstall(PrefService* prefs) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
 #if defined(OS_WIN)
   ElevatedInstallRecoveryComponent(
       prefs->GetFilePath(prefs::kRecoveryComponentUnpackPath));
-#endif
+#endif  // OS_WIN
   prefs->SetBoolean(prefs::kRecoveryComponentNeedsElevation, false);
 }
 
 void DeclinedElevatedRecoveryInstall(PrefService* prefs) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   prefs->SetBoolean(prefs::kRecoveryComponentNeedsElevation, false);
 }
 
 }  // namespace component_updater