Persist setup metrics and have Chrome report them during UMA upload.

components/metrics/file_metrics_provider.cc

Index: components/metrics/file_metrics_provider.cc
diff --git a/components/metrics/file_metrics_provider.cc b/components/metrics/file_metrics_provider.cc
new file mode 100644
index 0000000000000000000000000000000000000000..54970887b13803877c07e38920d4bb9d3d2bb745
--- /dev/null
+++ b/components/metrics/file_metrics_provider.cc
@@ -0,0 +1,209 @@
+// Copyright 2016 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 "components/metrics/file_metrics_provider.h"
+
+#include "base/command_line.h"
+#include "base/files/file.h"
+#include "base/files/file_util.h"
+#include "base/files/memory_mapped_file.h"
+#include "base/logging.h"
+#include "base/metrics/histogram_base.h"
+#include "base/metrics/histogram_persistence.h"
+#include "base/metrics/persistent_memory_allocator.h"
+#include "base/task_runner.h"
+#include "base/threading/worker_pool.h"
+#include "base/time/time.h"
+#include "components/metrics/metrics_pref_names.h"
+#include "components/metrics/metrics_service.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/pref_service.h"
+
+namespace metrics {
+
+// Out-of-line constructor and destructor needed for code efficiency.
+FileMetricsProvider::FileInformation::FileInformation() {}
+FileMetricsProvider::FileInformation::~FileInformation() {}
+
+FileMetricsProvider::FileMetricsProvider(
+    const scoped_refptr<base::TaskRunner>& task_runner,
+    PrefService* local_state)
+    : task_runner_(task_runner),
+      pref_service_(local_state) {
+}
+
+FileMetricsProvider::~FileMetricsProvider() {
+}
+
+void FileMetricsProvider::RegisterFile(const base::FilePath& path,
+                                       FileType type,
+                                       const base::StringPiece& prefs_key) {
+  FileInformation* file = new FileInformation();
+  file->path = path;
+  file->type = type;
+  file->prefs_key = prefs_key.as_string();
+
+  if (pref_service_ && !prefs_key.empty()) {
+    file->last_seen = base::Time::FromInternalValue(
+        pref_service_->GetInt64(metrics::prefs::kMetricsLastSeenPrefix +
+                                prefs_key.as_string()));
+  }
+
+  files_to_check_.push_back(make_scoped_ptr(file));
+}
+
+// static
+void FileMetricsProvider::RegisterPrefs(PrefRegistrySimple* prefs,
+                                        const base::StringPiece& key) {
+  prefs->RegisterInt64Pref(metrics::prefs::kMetricsLastSeenPrefix +
+                           key.as_string(), 0);
+}
+
+// static
+void FileMetricsProvider::CheckAndMapNewMetricFilesOnTaskRunner(
+    FileMetricsProvider::FileInformationList* files) {
+  for (auto& file : *files)
+    CheckAndMapNewMetrics(file.get());
+}
+
+// static
+bool FileMetricsProvider::CheckAndMapNewMetrics(
+    FileMetricsProvider::FileInformation* file) {
+  DCHECK(!file->mapped);
+
+  // Un-mapping a file can only be done on an I/O thread which is why it is
+  // only marked "to release" with the actual release being done here.
+  file->file_to_release.reset();
+  file->allocator_to_release.reset();
+
+  base::File::Info info;
+  if (!base::GetFileInfo(file->path, &info) ||
+      info.is_directory || info.size == 0) {
+    return false;
+  }
+
+  if (file->last_seen >= info.last_modified)
+    return false;
+
+  // A new file of metrics has been found. Map it into memory.
+  file->mapped.reset(new base::MemoryMappedFile());
+  if (!file->mapped->Initialize(file->path)) {
+    NOTREACHED();
+    file->mapped.reset();
+    return false;
+  }
+  return true;
+}
+
+void FileMetricsProvider::ScheduleFilesCheck() {
+  if (files_to_check_.empty())
+    return;
+
+  FileInformationList* check_list = new FileInformationList();
+  std::swap(files_to_check_, *check_list);
+  bool posted = task_runner_->PostTaskAndReply(
+      FROM_HERE,
+      base::Bind(&FileMetricsProvider::CheckAndMapNewMetricFilesOnTaskRunner,
+                 base::Unretained(check_list)),
+      base::Bind(&FileMetricsProvider::RecordFilesChecked,
+                 base::Unretained(this), base::Owned(check_list)));

[grt (UTC plus 2), 2016/02/15 15:42:39]

IIUC, this FileMetricsProvider instance will be deleted during shutdown in the
MetricsService's dtor. if i'm reading the tea leaves, this happens in
BrowserProcessImpl::StartTearDown. the reply task here could potentially be run
after that point, leading to a use-after-free. to protect against this, add a
WeakPtrFactory member to this class, and bind the instance with
weak_factory_.GetWeakPtr() rather than Unretained. this way, the reply is safely
dropped if it is run after the provider is destroyed.

[bcwhite, 2016/02/15 19:22:11]

Done.

+  DCHECK(posted);

[grt (UTC plus 2), 2016/02/15 15:42:39]

i don't think this is a condition that should break developers (by causing their
browser to crash). it simply means that the browser is in its shutdown phase,
which is valid, no?

[bcwhite, 2016/02/15 19:22:10]

Done.

+}
+
+void FileMetricsProvider::RecordFilesChecked(FileInformationList* checked) {
+  // Move each processed file to either the "to-read" list (for processing) or

[grt (UTC plus 2), 2016/02/15 15:42:39]

  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
at the top of the function

[bcwhite, 2016/02/15 19:22:11]

This is in "components".  Can it access "content"?

Also, this breaks the tests because neither TestSimpleTaskRunner nor the test
thread match this check.

[grt (UTC plus 2), 2016/02/15 19:53:55]

Ah, bummer. Another way to ensure that methods aren't called on the wrong
threads is to add a ThreadChecker instance to the class, and sprinkle
DCHECK(thread_checker_.CalledOnValidThread()); here and there.

[bcwhite, 2016/02/15 21:46:00]

Done.

+  // the "to-check" list (for future checking).
+  for (auto iter = checked->begin(); iter != checked->end();) {
+    auto temp = iter++;
+    const FileInformation* file = temp->get();
+    if (file->mapped)
+      files_to_read_.splice(files_to_read_.end(), *checked, temp);
+    else
+      files_to_check_.splice(files_to_check_.end(), *checked, temp);
+  }
+}
+
+void FileMetricsProvider::RecordFileAsSeen(FileInformation* file) {
+  file->last_seen = base::Time::Now();
+  if (pref_service_ && !file->prefs_key.empty()) {
+    pref_service_->SetInt64(metrics::prefs::kMetricsLastSeenPrefix +
+                            file->prefs_key,
+                            file->last_seen.ToInternalValue());
+  }
+}
+
+void FileMetricsProvider::OnDidCreateMetricsLog() {
+  // Move finished metric files back to list of monitored files.

[grt (UTC plus 2), 2016/02/15 15:42:39]

  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
at the top of the function

[bcwhite, 2016/02/15 21:46:00]

Done.

+  for (auto iter = files_to_read_.begin(); iter != files_to_read_.end();) {
+    auto temp = iter++;
+    const FileInformation* file = temp->get();
+    if (!file->allocator && !file->mapped)
+      files_to_check_.splice(files_to_check_.end(), files_to_read_, temp);
+  }
+
+  // Schedule a check to see if there are new metrics to load. If so, they
+  // will be reported during the next collection run after this one. The
+  // check is run off of the worker-pool so as to not cause delays on the
+  // main UI thread (which is currently where metric collection is done).
+  ScheduleFilesCheck();
+}
+
+void FileMetricsProvider::RecordHistogramSnapshots(
+    base::HistogramSnapshotManager* hsm) {
+  for (auto& file : files_to_read_) {

[grt (UTC plus 2), 2016/02/15 15:42:39]

  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
at the top of the function

[bcwhite, 2016/02/15 21:46:00]

Done.

+    // If the file is mapped then it needs to have an allocator attached to
+    // it in order to read histograms out of it.
+    if (file->mapped) {
+      DCHECK(!file->allocator);
+      DCHECK(!file->file_to_release);
+      DCHECK(!file->allocator_to_release);
+      if (!base::FilePersistentMemoryAllocator::IsFileAcceptable(
+              *file->mapped)) {
+        // Something is fundamentally wrong with the file. Ignore it.
+        LOG(ERROR) << "Metrics file \"" << file->path.value()
+                   << "\" is not valid -- ignored.";
+        file->file_to_release.swap(file->mapped);
+        RecordFileAsSeen(file.get());
+        NOTREACHED();
+        continue;
+      }
+      file->allocator.reset(new base::FilePersistentMemoryAllocator(
+          std::move(file->mapped), 0, std::string()));
+    }
+
+    // A file should not be under "files to read" unless it has an allocator
+    // or is memory-mapped (at which point it will have received an allocator
+    // above). However, if this method gets called twice before the scheduled-
+    // files-check has a chance to clean up, this may trigger.
+    if (!file->allocator)
+      continue;
+
+    int histogram_count = 0;
+    base::PersistentMemoryAllocator::Iterator hist_iter;
+    file->allocator->CreateIterator(&hist_iter);
+    for (;;) {

[grt (UTC plus 2), 2016/02/15 15:42:39]

nit: while (true) is more common

[bcwhite, 2016/02/15 19:22:10]

Done.

+      scoped_ptr<base::HistogramBase> histogram(
+          base::GetNextPersistentHistogram(file->allocator.get(), &hist_iter));
+      if (!histogram)
+        break;
+      if (file->type == FILE_HISTOGRAMS_ATOMIC)
+        hsm->PrepareOnceTakingOwnership(std::move(histogram));
+      else
+        hsm->PrepareDeltaTakingOwnership(std::move(histogram));
+      histogram_count++;

[grt (UTC plus 2), 2016/02/15 15:42:39]

although either pre- or post-increment is allowed for fundamental types like
this
(https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement),
it's easier to read/review code when postincrement is a rare exception used only
when needed. so please use ++histogram_count here.

[bcwhite, 2016/02/15 19:22:11]

Done.

+    }
+    VLOG(1) << "Reported " << histogram_count << " histograms from "

[grt (UTC plus 2), 2016/02/15 15:42:39]

production logging carries weight in the form of the strings and the code
required for the logging. unless there is a compelling reason for this to be
present in the product, please use DVLOG. (note that the installer is one
exception where production logging is extraordinarily useful and is
allowed/encouraged.)

[bcwhite, 2016/02/15 19:22:10]

Done.

+            << file->path.value();
+
+    if (file->type == FILE_HISTOGRAMS_ATOMIC) {
+      DCHECK(!file->mapped);  // Ownership should have moved to allocator.
+      DCHECK(!file->file_to_release);
+      DCHECK(!file->allocator_to_release);
+      file->allocator_to_release.swap(file->allocator);

[grt (UTC plus 2), 2016/02/15 15:42:39]

since the allocator isn't released until the next pass through
OnDidCreateMetricsLog, doesn't this mean that chrome keeps the installer's
metrics file open pretty much all the time? this will prevent the installer from
providing new metrics at all while chrome is in use. i had thought this was
going to change so that OnDidCreateMetricsLog would trigger an immediate read of
the metrics file into a buffer so that the file could be closed immediately.

[bcwhite, 2016/02/15 19:22:10]

The call to OnDidCreateMetricsLog should be called every 30 minutes.  As it
happens, the RecordHistogramSnapshots is called at the end of the period, just
before OnDidCreateMetricsLog is called again.  This doesn't end up changing how
long the file is open.

Also, the file is only open if it's been modified so most of the time it will
not be open.

Yes, that is the plan.  Or to move it to its own thread where it call all be
done in RecordHistogramSnapshots.

[grt (UTC plus 2), 2016/02/15 19:53:55]

Ah, okay, so it'll generally be locked only during a single 30min window. That's
better than I'd first imagined.
Is there a reason not to do the "slurp into RAM" change now?

[bcwhite, 2016/02/15 21:46:00]

You know...  That was more trouble than each little but but now it'll just be
easier to rip out the hacks needed to manage multiple threads and do that.

Done.

+      RecordFileAsSeen(file.get());
+    }
+  }
+}
+
+}  // namespace metrics