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..d65357fa17b989bc806f01c684070947205e6559
--- /dev/null
+++ b/components/metrics/file_metrics_provider.cc
@@ -0,0 +1,214 @@
+// Copyright 2015 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/prefs/pref_registry_simple.h"
+#include "base/prefs/pref_service.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"
+
+namespace metrics {
+
+// Out-of-line constructor and destructor needed for code efficiency.
+FileMetricsProvider::FileInformation::FileInformation() {}
+FileMetricsProvider::FileInformation::~FileInformation() {}
+
+FileMetricsProvider::FileMetricsProvider(
+    metrics::MetricsService* metrics_service,
+    PrefService* local_state)
+    : metrics_found_(0),

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

nit: remove this initializer

[bcwhite, 2016/02/04 21:51:14]

Leftover from a previous design.  Odd that it isn't a compile error.

+      metrics_service_(metrics_service),
+      pref_service_(local_state) {
+  // Start a background check for pending setup metrics so it can be uploaded

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

is it appropriate for this to be generalized so that it isn't hardcoded to be
specific to setup metrics? otherwise, should FileMetricsProvider be called
SetupMetricsProvider?

[bcwhite, 2016/02/04 21:51:14]

I started it as something specific for Setup but then others were asking if it
could be used to pass in other metrics from other locations so I rewrote it to
be more generic, basically handling a list of files rather than one file.

[grt (UTC plus 2), 2016/02/08 18:09:18]

In that case, could you update this and other references to "setup" in the
comments and constants?

[bcwhite, 2016/02/09 21:08:45]

Done.

+  // during the initial report.
+  ScheduleFilesCheck();
+}
+
+FileMetricsProvider::~FileMetricsProvider() {
+  // Destruction of this object can lead to failures because of a task
+  // posted on the Worker Pool or because the owned Memory Allocator has

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

the destruction problems are avoided by following my advice in
ScheduleFilesCheck.

+  // been passed to the Metrics Service. Both cases can only occur if
+  // a Metrics Service has been provided (i.e. not during testing).
+  DCHECK(!metrics_service_);
+}
+
+void FileMetricsProvider::RegisterFile(const base::FilePath& path,
+                                       FileType type,
+                                       const std::string& prefs_key) {
+  FileInformation* file = new FileInformation();
+  file->path = path;
+  file->type = type;
+  file->prefs_key = prefs_key;
+
+  if (!prefs_key.empty()) {

[grt (UTC plus 2), 2016/02/04 15:36:43]

&& pref_service_
since it could be null as per the comment above
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...

[bcwhite, 2016/02/04 21:51:14]

Done.

+    file->last_seen = base::Time::FromInternalValue(
+        pref_service_->GetInt64(metrics::prefs::kSetupMetricsLastSeenPrefix +
+                                prefs_key));
+  }
+
+  base::AutoLock lock(lock_);
+  metrics_files_.push_back(scoped_ptr<FileInformation>(file));

[grt (UTC plus 2), 2016/02/04 15:36:43]

nit: scoped_ptr<FileInformation>(file) -> make_scoped_ptr(file)

[bcwhite, 2016/02/04 21:51:14]

Done.

+}
+
+// static
+void FileMetricsProvider::RegisterPrefs(PrefRegistrySimple* prefs,
+                                        const std::string& key) {
+    prefs->RegisterInt64Pref(metrics::prefs::kSetupMetricsLastSeenPrefix +

[grt (UTC plus 2), 2016/02/04 15:36:43]

nit: indentation

[bcwhite, 2016/02/04 21:51:14]

Done.

+                             key, 0);
+}
+
+void FileMetricsProvider::OnDidCreateMetricsLog() {
+  // Hold off async updates to file lists while processing.
+  base::AutoLock lock(lock_);
+
+  // Move finished metric files back to list of monitored files.
+  FileInformationList::iterator iter;
+  for (iter = metrics_found_.begin(); iter != metrics_found_.end();) {

[grt (UTC plus 2), 2016/02/04 15:36:43]

nit: define |iter| here with "auto iter = ..."

[bcwhite, 2016/02/04 21:51:14]

Done.

+    auto temp = iter++;
+    const FileInformation* file = temp->get();

[grt (UTC plus 2), 2016/02/04 15:36:43]

nit: omit this local var and use temp-> directly below

[bcwhite, 2016/02/04 21:51:14]

I'd rather not.  It looks ugly with all the temp->get()-> and doesn't
self-document what is being iterated.

+    if (!file->allocator && !file->mapped) {
+      metrics_files_.push_back(std::move(*temp));

[grt (UTC plus 2), 2016/02/04 15:36:43]

i believe this line and the next can be replaced with:
      metrics_files_.splice(metrics_files_.end(), metrics_found_, temp);

[bcwhite, 2016/02/04 21:51:14]

Done.

+      metrics_found_.erase(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) {
+  // Hold off async updates to file lists while processing.
+  base::AutoLock lock(lock_);
+
+  for (auto& file_iter : metrics_found_) {

[grt (UTC plus 2), 2016/02/04 15:36:43]

nit: |file_iter| isn't an iterator here, but rather a reference to a
scoped_ptr<FileInformation>. i think |file| or |file_ptr| are both more
appropriate names for it. i'm leaning toward the former since we don't generally
put a _ptr suffix on variables of type scoped_ptr<>.

[bcwhite, 2016/02/04 21:51:14]

Done.

+    FileInformation* file = file_iter.get();

[grt (UTC plus 2), 2016/02/04 15:36:43]

nit: omit this local var and use the auto'd |file| below

[bcwhite, 2016/02/04 21:51:14]

Done.

+
+    if (!file->allocator) {
+      DCHECK(file->mapped);
+      if (!base::FilePersistentMemoryAllocator::IsFileAcceptable(
+               *file->mapped)) {
+        // Something is fundamentally wrong with the file. Ignore it.
+        LOG(ERROR) << "Metrics file \"" << file->path.AsUTF8Unsafe()

[grt (UTC plus 2), 2016/02/04 15:36:43]

file->path.value() should be fine here

[bcwhite, 2016/02/04 21:51:14]

Done.

+                   << "\" is not valid -- ignored.";
+        file->mapped.reset();
+        RecordFileAsSeen(file);
+        NOTREACHED();
+        continue;
+      }
+      file->allocator.reset(new base::FilePersistentMemoryAllocator(

[grt (UTC plus 2), 2016/02/04 15:36:43]

change FilePersistentMemoryAllocator so that it takes a scoped_ptr rather than a
raw ptr, and use std::move(file->mapped) here. this makes the ownership transfer
explicit and extremely difficult to get wrong.

[bcwhite, 2016/02/04 21:51:14]

Doing that in a different CL:
https://codereview.chromium.org/1660143009

+                              file->mapped.release(), 0, std::string()));
+    }
+
+    if (file->allocator) {

[grt (UTC plus 2), 2016/02/04 15:36:43]

remove this condition as it will always be true on the basis of lines 102, 112,
and 114

[bcwhite, 2016/02/04 21:51:14]

Done.

+      base::PersistentMemoryAllocator::Iterator hist_iter;
+      file->allocator->CreateIterator(&hist_iter);
+      for (;;) {
+        base::HistogramBase* histogram =

[grt (UTC plus 2), 2016/02/04 15:36:43]

use scoped_ptr<base::HistogramBase> rather than a naked pointer since ownership
is transferred

[bcwhite, 2016/02/09 21:08:45]

Done.

+            base::GetNextPersistentHistogram(file->allocator.get(), &hist_iter);

[grt (UTC plus 2), 2016/02/04 15:36:43]

please update this and the related functions to return a scoped_ptr rather than
a naked pointer. this makes the ownership explicit in the API

[bcwhite, 2016/02/09 21:08:45]

Okay.  It doesn't belong here, though.  I'll do it in a different CL.

+        if (!histogram)
+          break;
+        if (file->type == FILE_HISTOGRAMS_ATOMIC) {

[grt (UTC plus 2), 2016/02/04 15:36:43]

nit: omit braces

[bcwhite, 2016/02/04 21:51:14]

Done.

+          hsm->PrepareOnceTakingOwnership(histogram);
+        } else {
+          hsm->PrepareDeltaTakingOwnership(histogram);
+        }
+      }
+
+      if (file->type == FILE_HISTOGRAMS_ATOMIC) {
+        DCHECK(!file->mapped);  // Ownership should have moved to allocator.
+        file->allocator.reset();
+        RecordFileAsSeen(file);
+      }
+    }
+  }
+}
+
+void FileMetricsProvider::RecordFileAsSeen(FileInformation* file) {
+  file->last_seen = base::Time::Now();
+  if (!file->prefs_key.empty()) {

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

 && pref_service_

[bcwhite, 2016/02/04 21:51:14]

Done.

+    pref_service_->SetInt64(metrics::prefs::kSetupMetricsLastSeenPrefix +
+                            file->prefs_key,
+                            file->last_seen.ToInternalValue());
+  }
+}
+
+// static
+void FileMetricsProvider::CheckAndMapNewMetricFiles(

[grt (UTC plus 2), 2016/02/04 15:36:43]

nit: often a suffix such as "OnBlockingPool" is added to isolated functions like
this that run on a special thread.

[bcwhite, 2016/02/04 21:51:14]

Done.

+    FileMetricsProvider::FileInformationList* files,
+    base::Callback<void(FileInformationList*)> callback) {
+  for (auto& file : *files)
+    CheckAndMapNewMetrics(file.get());
+  callback.Run(files);
+}
+
+// static
+bool FileMetricsProvider::CheckAndMapNewMetrics(
+    FileMetricsProvider::FileInformation* file) {
+  DCHECK(!file->mapped);
+
+  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 setup metrics has been found. Map it into memory.
+  file->mapped.reset(new base::MemoryMappedFile());
+  file->mapped->Initialize(file->path);

[grt (UTC plus 2), 2016/02/04 15:36:43]

  if (file->mapped->Initialize(file->path))
    return true;
  file->mapped.reset();
  return false;

[bcwhite, 2016/02/04 21:51:14]

Done.

+  return true;
+}
+
+void FileMetricsProvider::ScheduleFilesCheck() {
+  // Don't do anything if metrics_service is not valid (for testing).
+  if (!metrics_service_)
+    return;
+
+  // TODO(bcwhite): Change this if metric collection moves off UI thread.
+  FileInformationList* check_list = new FileInformationList();
+  std::swap(metrics_files_, *check_list);
+  bool posted = base::WorkerPool::PostTask(

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

This should use the BrowserThread BlockingPool rather than the WorkerPool. One
way to do this is to have ChromeMetricsServiceClient::Initialize inject either a
TaskRunner or a SequencedWorkerPool obtained from
content::BrowserThread::GetBlockingPool(). Please read the WorkerShutdown
comments in sequenced_worker_pool.h to decide what shutdown semantics are called
for (SKIP_ON_SHUTDOWN would be good, I think). Also, consider using a
SequenceToken so that file checks are always serialized. You could use
GetSequencedTaskRunnerWithShutdownBehavior with a random token during
initialization and hold on to the resulting SequencedTaskRunner in the instance.

Additionally, it's cleaner to use PostTaskAndReply so that
CheckAndMapNewMetricFiles is run on an IO-safe thread, and RecordFileInformation
is run back on the UI thread. In this case, bind |check_list| as an unretained
pointer for the first callback and as an owned pointer for the second. This has
the benefit of allowing you to do away with the lock since the instance will
only be mutated on the UI thread. I suggest adding a base::ThreadChecker member
to the class, and sprinkling
  DCHECK(thread_checker_.CalledOnValidThread());
at the top of all of the functions so that it's clear that the class is
single-threaded and that things will 'splode if anyone ever forgets.

+    FROM_HERE,
+    base::Bind(&CheckAndMapNewMetricFiles,
+               base::Owned(check_list),
+               base::Bind(&FileMetricsProvider::RecordFileInformation,
+                          base::Unretained(this))),

[grt (UTC plus 2), 2016/02/04 15:36:43]

This will lead to a use-after-free if the metrics service is shut down while a
file check is running. WeakPtr is commonly used to deal with this. Add a
base::WeakPtrFactory instance member, then call weak_factory_.GetWeakPtr() here
when binding.

+    false);
+  DCHECK(posted);

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

I don't think this is needed. false from PostTask* is generally an indication
that Chrome is shutting down, which probably isn't a reason to crash the
browser.

+}
+
+void FileMetricsProvider::RecordFileInformation(FileInformationList* files) {
+  // This is a callback and thus possibly not on the same thread as the main
+  // metrics collection.
+  base::AutoLock lock(lock_);
+
+  // Move each processed file to either the "found" list (for processing) or
+  // the "files" list (for future checking).
+  for (auto& iter : *files) {
+    if (iter->mapped) {

[grt (UTC plus 2), 2016/02/04 15:36:43]

nit: omit braces

[bcwhite, 2016/02/09 21:08:45]

Done.

+      metrics_found_.push_back(std::move(iter));

[grt (UTC plus 2), 2016/02/04 15:36:43]

use splice to move the element. you'll have to change from the range-based loop
to an iterator-based one like you have on line 78, but this is a small price to
pay. for example:
    metrics_found_.splice(metrics_found_.end(), *files, temp);

[bcwhite, 2016/02/04 21:51:14]

Each move in that case has to delete the value from the head of the vector,
shifting all the other ones.  O(N^2)  This way allows just the internal pointer
to be moved and then the entire vector is deleted by the caller.

The code back on line #78 has the same issue but only applies to files that were
found and need to be recorded whereas this is every file.  Still, it could do
the same, moving the pointers and then .clear-ing the vector at the end.

[grt (UTC plus 2), 2016/02/08 18:09:18]

metrics_files_ and metrics_found_ are both linked lists. Using splice is the
most efficient, as it just does some simple pointer juggling with the list
items. I believe that anything else will be more costly.

+    } else {
+      metrics_files_.push_back(std::move(iter));

[grt (UTC plus 2), 2016/02/04 15:36:43]

    metrics_files_.splice(metrics_files_.end(), *files, temp);

[bcwhite, 2016/02/04 21:51:14]

Done.

+    }
+  }
+}
+
+}  // namespace metrics