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 {
+  RCE_RUNNING_NON_ELEVATED = 0,
+  RCE_ELEVATION_NEEDED = 1,
+  RCE_FAILED = 2,
+  RCE_SUCCEEDED = 3,
+  RCE_SKIPPED = 4,
+  RCE_RUNNING_ELEVATED = 5,
+  RCE_ELEVATED_FAILED = 6,
+  RCE_ELEVATED_SUCCEEDED = 7,
+  RCE_ELEVATED_SKIPPED = 8,
+  RCE_COMPONENT_DOWNLOAD_ERROR = 9,
+  RCE_COUNT
+};
+
 #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()));
   }
   return scoped_ptr<base::DictionaryValue>();
 }
 
+void WaitForElevatedInstallToComplete(base::Process process) {
+  int installer_exit_code = 0;
+  const base::TimeDelta kMaxWaitTime = base::TimeDelta::FromSeconds(600);
+  if (!process.WaitForExitWithTimeout(kMaxWaitTime, &installer_exit_code)) {
+    UMA_HISTOGRAM_ENUMERATION("RecoveryComponent.Event", RCE_ELEVATED_FAILED,
+                              RCE_COUNT);
+  } else {
+    if (installer_exit_code == EXIT_CODE_RECOVERY_SUCCEEDED) {
+      UMA_HISTOGRAM_ENUMERATION("RecoveryComponent.Event",
+                                RCE_ELEVATED_SUCCEEDED, RCE_COUNT);
+    } else {
+      UMA_HISTOGRAM_ENUMERATION("RecoveryComponent.Event",
+                                RCE_ELEVATED_SKIPPED, RCE_COUNT);

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

So if the exit code is not success it means "skip"?

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

That's right.

+    }
+  }
+}
+
 void DoElevatedInstallRecoveryComponent(const base::FilePath& path) {
   const base::FilePath main_file = path.Append(kRecoveryFileName);
   const base::FilePath manifest_file =
       path.Append(FILE_PATH_LITERAL("manifest.json"));
   if (!base::PathExists(main_file) || !base::PathExists(manifest_file))
     return;
 
   scoped_ptr<base::DictionaryValue> manifest(ReadManifest(manifest_file));
   std::string name;
   manifest->GetStringASCII("name", &name);
   if (name != kRecoveryManifestName)
     return;
   std::string proposed_version;
   manifest->GetStringASCII("version", &proposed_version);
   const Version version(proposed_version.c_str());
   if (!version.IsValid())
     return;
 
   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_RUNNING_ELEVATED,
+                            RCE_COUNT);
+
   base::LaunchOptions options;
   options.start_hidden = true;
-  base::LaunchElevatedProcess(cmdline, options);
+  base::Process process = base::LaunchElevatedProcess(cmdline, options);
+
+  base::WorkerPool::PostTask(

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

Cool, didn't know of WorkerPool..! Are you sure that histograms are 100% safe to
call at any point in the browser's lifetime (I think so but not 100% sure).

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

As sure as I am of anything. Alexei will do an OWNERS review on this, and I will
ask him then. 

+      FROM_HERE,
+      base::Bind(&WaitForElevatedInstallToComplete, base::Passed(&process)),
+      true);
 }
 
 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_COUNT);
   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) {
-    BrowserThread::PostTask(
-        BrowserThread::UI,
-        FROM_HERE,
-        base::Bind(&SetPrefsForElevatedRecoveryInstall,
-                   installer_folder,
-                   prefs));
+  if (!process.WaitForExitWithTimeout(kMaxWaitTime, &installer_exit_code)) {

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

Hmmm, the ! case should be the failure case in which you don't have an exit
code, no?

In general, I prefer having the basic case as the basic condition and the else{}
handling the ! (here and in other changes in this file).

Handling the ! first often makes for weird logic.

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

Hrmm, actually, the ! there looks like a typo. Thanks for catching, removing. 

+    if (installer_exit_code == EXIT_CODE_ELEVATION_NEEDED) {
+      UMA_HISTOGRAM_ENUMERATION("RecoveryComponent.Event", RCE_ELEVATION_NEEDED,
+                                RCE_COUNT);
+
+      BrowserThread::PostTask(
+          BrowserThread::UI,
+          FROM_HERE,
+          base::Bind(&SetPrefsForElevatedRecoveryInstall,
+                     installer_folder,
+                     prefs));
+    } else if (installer_exit_code == EXIT_CODE_RECOVERY_SUCCEEDED) {
+      UMA_HISTOGRAM_ENUMERATION("RecoveryComponent.Event", RCE_SUCCEEDED,
+                                RCE_COUNT);
+    } else if (installer_exit_code == EXIT_CODE_RECOVERY_SKIPPED) {
+      UMA_HISTOGRAM_ENUMERATION("RecoveryComponent.Event", RCE_SKIPPED,
+                                RCE_COUNT);
+    }
+  } else {
+    UMA_HISTOGRAM_ENUMERATION("RecoveryComponent.Event", RCE_FAILED, RCE_COUNT);
   }
 }
 
 bool RecoveryComponentInstaller::RunInstallCommand(
     const base::CommandLine& cmdline,
     const base::FilePath& installer_folder) const {
+  UMA_HISTOGRAM_ENUMERATION("RecoveryComponent.Event", RCE_RUNNING_NON_ELEVATED,
+                            RCE_COUNT);
+
   base::LaunchOptions options;
   options.start_hidden = true;
   base::Process process = base::LaunchProcess(cmdline, options);
   if (!process.IsValid())
     return false;

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

Do we care about this failure?

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

The existing code does, what motivates you to want to change the logic here?

[gab, 2015/01/13 18:23:32]

Yes, I was asking whether this should also result in a histogram event?

 
   // 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