Improve persistent histogram storage's usage in uninstall and crashpad

Disable histogram persistence during uninstall since there's neither a directory
in which to write them nor a browser to subsequently upload them. This allows
to add LOG(ERROR) without breaking the installation tests when it detects the
storage directory does not exist.

Also, do not attempt to store histograms from setup.exe executions dedicated to
crashpad. Otherwise, this will trigger LOG(ERROR) in the destructor of class
PersistentHistogramStorage.

Bug: 734095
Change-Id: Iffff0fab25eedb7f934144802e805ba2d047a42a
Reviewed-on: https://chromium-review.googlesource.com/987541Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Commit-Queue: Xi Cheng <chengx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547957}

base/metrics/persistent_histogram_storage.cc

@@ -39,6 +39,9 @@ PersistentHistogramStorage::~PersistentHistogramStorage() {
   PersistentHistogramAllocator* allocator = GlobalHistogramAllocator::Get();
   allocator->UpdateTrackingHistograms();
 
+  if (disabled_)
+    return;
+
   // Stop if the storage base directory has not been properly set.
   if (storage_base_dir_.empty()) {
     LOG(ERROR)
@@ -50,22 +53,28 @@ PersistentHistogramStorage::~PersistentHistogramStorage() {
 
   FilePath storage_dir = storage_base_dir_.AppendASCII(allocator->Name());
 
-  if (storage_dir_create_action_ == StorageDirCreation::kEnable) {
-    // Stop if |storage_dir| does not exist and cannot be created after an
-    // attempt.
-    if (!CreateDirectory(storage_dir)) {
-      LOG(ERROR) << "Could not write \"" << allocator->Name()
-                 << "\" persistent histograms to file as the storage directory "
-                    "cannot be created.";
-      return;
-    }
-  } else if (!DirectoryExists(storage_dir)) {
-    // Stop if |storage_dir| does not exist. That can happen if the product
-    // hasn't been installed yet or if it has been uninstalled.
-    // TODO(chengx): Investigate if there is a need to update setup_main.cc or
-    // test_installer.py so that a LOG(ERROR) statement can be added here
-    // without breaking the test.
-    return;
+  switch (storage_dir_create_action_) {
+    case StorageDirCreation::kEnable:
+      if (!CreateDirectory(storage_dir)) {
+        LOG(ERROR)
+            << "Could not write \"" << allocator->Name()
+            << "\" persistent histograms to file as the storage directory "
+               "cannot be created.";
+        return;
+      }
+      break;
+    case StorageDirCreation::kDisable:
+      if (!DirectoryExists(storage_dir)) {
+        // When the consumer of this class disables storage directory creation
+        // by the class instance, it should ensure the directory's existence if
+        // it's essential.
+        LOG(ERROR)
+            << "Could not write \"" << allocator->Name()
+            << "\" persistent histograms to file as the storage directory "
+               "does not exist.";
+        return;
+      }
+      break;
   }
 
   // Save data using the current time as the filename. The actual filename

base/metrics/persistent_histogram_storage.h

@@ -31,7 +31,8 @@ class BASE_EXPORT PersistentHistogramStorage {
   // well as the leaf directory name for the file to which the histograms are
   // persisted. The string must be ASCII.
   // |storage_dir_create_action| determines that if this instance is
-  // responsible for creating the storage directory.
+  // responsible for creating the storage directory. If kDisable is supplied,
+  // it's the consumer's responsibility to create the storage directory.
   PersistentHistogramStorage(StringPiece allocator_name,
                              StorageDirCreation storage_dir_create_action);
 
@@ -44,6 +45,9 @@ class BASE_EXPORT PersistentHistogramStorage {
     storage_base_dir_ = storage_base_dir;
   }
 
+  // Disables histogram storage.
+  void Disable() { disabled_ = true; }
+
  private:
   // Metrics files are written into directory
   // |storage_base_dir_|/|allocator_name| (see the ctor for allocator_name).
@@ -53,6 +57,11 @@ class BASE_EXPORT PersistentHistogramStorage {
   // storage directory.
   const StorageDirCreation storage_dir_create_action_;
 
+  // A flag indicating if histogram storage is disabled. It starts with false,
+  // but can be set to true by the caller who decides to throw away its
+  // histogram data.
+  bool disabled_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(PersistentHistogramStorage);
 };
 

chrome/installer/setup/setup_main.cc

@@ -1306,6 +1306,7 @@ int WINAPI wWinMain(HINSTANCE instance, HINSTANCE prev_instance,
           switches::kProcessType);
 
   if (process_type == crash_reporter::switches::kCrashpadHandler) {
+    persistent_histogram_storage.Disable();
     return crash_reporter::RunAsCrashpadHandler(
         *base::CommandLine::ForCurrentProcess(), base::FilePath(),
         switches::kProcessType, switches::kUserDataDir);
@@ -1350,6 +1351,11 @@ int WINAPI wWinMain(HINSTANCE instance, HINSTANCE prev_instance,
 
   const bool is_uninstall = cmd_line.HasSwitch(installer::switches::kUninstall);
 
+  // Disable histogram storage during uninstall since there's neither a
+  // directory in which to write them nor a browser to subsequently upload them.
+  if (is_uninstall)
+    persistent_histogram_storage.Disable();
+
   // Check to make sure current system is Win7 or later. If not, log
   // error message and get out.
   if (!InstallUtil::IsOSSupported()) {

notification_helper/notification_helper.cc

@@ -38,8 +38,10 @@ extern "C" int WINAPI wWinMain(HINSTANCE instance,
   // "-Embedding" flag to the command line. If this flag is not found, the
   // process should exit immediately.
   // https://msdn.microsoft.com/en-us/library/windows/desktop/ms683844.aspx
-  if (!base::CommandLine::ForCurrentProcess()->HasSwitch("embedding"))
+  if (!base::CommandLine::ForCurrentProcess()->HasSwitch("embedding")) {
+    persistent_histogram_storage.Disable();
     return 0;
+  }
 
   install_static::InitializeProductDetailsForPrimaryModule();