Always persist histograms from setup.exe.

chrome/installer/setup/persistent_histogram_storage.cc

Index: chrome/installer/setup/persistent_histogram_storage.cc
diff --git a/chrome/installer/setup/installer_metrics.cc b/chrome/installer/setup/persistent_histogram_storage.cc
similarity index 36%
rename from chrome/installer/setup/installer_metrics.cc
rename to chrome/installer/setup/persistent_histogram_storage.cc
index 7cc3108a4cc3627daa1f4433ea15f3818a1d5cf6..c40b511e10d6076c185fbfe7826358be2caa1346 100644
--- a/chrome/installer/setup/installer_metrics.cc
+++ b/chrome/installer/setup/persistent_histogram_storage.cc
@@ -2,15 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/installer/setup/installer_metrics.h"
+#include "chrome/installer/setup/persistent_histogram_storage.h"
 
-#include <windows.h>  // NOLINT
-#include <atlsecurity.h>
-
-#include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/important_file_writer.h"
+#include "base/logging.h"
 #include "base/metrics/histogram_base.h"
 #include "base/metrics/persistent_histogram_allocator.h"
 #include "base/metrics/persistent_memory_allocator.h"
@@ -21,70 +18,7 @@
 
 namespace installer {
 
-namespace {
-
-// This is duplicated from components/metrics/file_metrics_provider.h which is
-// not accessible from setup code.
-const base::FilePath::CharType MetricsFileExtension[] =
-    FILE_PATH_LITERAL(".pma");
-
-// Add to the ACL of an object on disk. This follows the method from MSDN:
-// https://msdn.microsoft.com/en-us/library/windows/desktop/aa379283.aspx
-// This is done using explicit flags rather than the "security string" format
-// because strings do not necessarily read what is written which makes it
-// difficult to de-dup. Working with the binary format is always exact and
-// the system libraries will properly ignore duplicate ACL entries.
-bool AddAclToPath(const base::FilePath& path,
-                  const CSid& trustee,
-                  ACCESS_MASK access_mask,
-                  BYTE ace_flags) {
-  DCHECK(!path.empty());
-  DCHECK(trustee);
-
-  // Get the existing DACL.
-  ATL::CDacl dacl;
-  if (!ATL::AtlGetDacl(path.value().c_str(), SE_FILE_OBJECT, &dacl)) {
-    DPLOG(ERROR) << "Failed getting DACL for path \"" << path.value() << "\"";
-    return false;
-  }
-
-  // Check if the requested access already exists and return if so.
-  for (UINT i = 0; i < dacl.GetAceCount(); ++i) {
-    ATL::CSid sid;
-    ACCESS_MASK mask = 0;
-    BYTE type = 0;
-    BYTE flags = 0;
-    dacl.GetAclEntry(i, &sid, &mask, &type, &flags);
-    if (sid == trustee && type == ACCESS_ALLOWED_ACE_TYPE &&
-        (flags & ace_flags) == ace_flags &&
-        (mask & access_mask) == access_mask) {
-      return true;
-    }
-  }
-
-  // Add the new access to the DACL.
-  if (!dacl.AddAllowedAce(trustee, access_mask, ace_flags)) {
-    DPLOG(ERROR) << "Failed adding ACE to DACL";
-    return false;
-  }
-
-  // Attach the updated ACL as the object's DACL.
-  if (!ATL::AtlSetDacl(path.value().c_str(), SE_FILE_OBJECT, dacl)) {
-    DPLOG(ERROR) << "Failed setting DACL for path \"" << path.value() << "\"";
-    return false;
-  }
-
-  return true;
-}
-
-}  // namespace
-
-base::FilePath GetPersistentHistogramStorageDir(
-    const base::FilePath& target_path) {
-  return target_path.AppendASCII(kSetupHistogramAllocatorName);
-}
-
-void BeginPersistentHistogramStorage() {
+PersistentHistogramStorage::PersistentHistogramStorage() {
   base::GlobalHistogramAllocator::CreateWithLocalMemory(
       1 << 20,  // 1 MiB
       0,        // No identifier.
@@ -92,52 +26,33 @@ void BeginPersistentHistogramStorage() {
   base::GlobalHistogramAllocator::Get()->CreateTrackingHistograms(
       kSetupHistogramAllocatorName);
 
-  // This can't be enabled until after the allocator is configured because
-  // there is no other reporting out of setup other than persistent memory.
+  // This can't be enabled until after the allocator is configured because there
+  // is no other reporting out of setup other than persistent memory.
   base::HistogramBase::EnableActivityReportHistogram("setup");
 }
 
-void EndPersistentHistogramStorage(const base::FilePath& target_path,
-                                   bool system_install) {
+PersistentHistogramStorage::~PersistentHistogramStorage() {
   base::PersistentHistogramAllocator* allocator =
       base::GlobalHistogramAllocator::Get();
   allocator->UpdateTrackingHistograms();
 
-  // Allocator dumps are saved to a directory that was created earlier. Stop
-  // now if that directory has been removed because an uninstall has happened.
-  base::FilePath dir_path = GetPersistentHistogramStorageDir(target_path);
-  if (!base::DirectoryExists(dir_path))
+  // Stop if |storage_dir_| isn't set or does not exist. That can happen if the
+  // product hasn't been installed yet or if it has been uninstalled.
+  if (storage_dir_.empty() || !base::DirectoryExists(storage_dir_))
     return;
 
-  // Set permissions on the directory that allows other processes, including
-  // non-privileged ones, to read and delete the files stored there. This
-  // allows the browser process to remove the metrics files once it's done
-  // reading them. This is only done for system-level installs; user-level
-  // installs already provide delete-file access to the browser process.
-  if (system_install) {
-    if (!AddAclToPath(dir_path, ATL::Sids::AuthenticatedUser(),
-                      FILE_GENERIC_READ | FILE_DELETE_CHILD,
-                      CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE)) {
-      PLOG(ERROR) << "Could not set \"delete\" permission for metrics directory"
-                  << " \"" << dir_path.value() << "\"";
-    }
-  }
-
-  // Remove any existing metrics file at the old (non-subdir) pathname.
-  base::DeleteFile(dir_path.AddExtension(MetricsFileExtension), false);
-
   // Save data using the current time as the filename. The actual filename
   // doesn't matter (so long as it ends with the correct extension) but this
   // works as well as anything.
   base::Time::Exploded exploded;
   base::Time::Now().LocalExplode(&exploded);
-  base::FilePath file_path =
-      dir_path
+  const base::FilePath file_path =
+      storage_dir_
           .AppendASCII(base::StringPrintf("%04d%02d%02d%02d%02d%02d",
                                           exploded.year, exploded.month,
                                           exploded.day_of_month, exploded.hour,
                                           exploded.minute, exploded.second))
-          .AddExtension(MetricsFileExtension);
+          .AddExtension(base::PersistentMemoryAllocator::kFileExtension);
 
   base::StringPiece contents(static_cast<const char*>(allocator->data()),
                              allocator->used());
@@ -145,4 +60,9 @@ void EndPersistentHistogramStorage(const base::FilePath& target_path,
     VLOG(1) << "Persistent histograms saved in file: " << file_path.value();
 }
 
+base::FilePath PersistentHistogramStorage::GetStorageDir(

[grt (UTC plus 2), 2016/09/13 19:42:59]

nit:
// static
before this

[fdoray, 2016/09/13 20:18:25]

Done.

+    const base::FilePath& target_path) {
+  return target_path.AppendASCII(kSetupHistogramAllocatorName);
+}
+
 }  // namespace installer