Support saving browser metrics to disk and reading them during next run.

base/metrics/persistent_histogram_allocator.cc

Index: base/metrics/persistent_histogram_allocator.cc
diff --git a/base/metrics/persistent_histogram_allocator.cc b/base/metrics/persistent_histogram_allocator.cc
index 62e8cd6d68baa4ca567e97f3c095ce093969eb1c..6570808276ed51601e9ff7a205adf8d30c552823 100644
--- a/base/metrics/persistent_histogram_allocator.cc
+++ b/base/metrics/persistent_histogram_allocator.cc
@@ -6,6 +6,7 @@
 
 #include <memory>
 
+#include "base/files/important_file_writer.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
@@ -46,6 +47,7 @@ enum : uint32_t {
 // anything essential at exit anyway due to the fact that they depend on data
 // managed elsewhere and which could be destructed first.
 GlobalHistogramAllocator* g_allocator;
+GlobalHistogramAllocator* g_allocator_disabled;
 
 // Take an array of range boundaries and create a proper BucketRanges object
 // which is returned to the caller. A return of nullptr indicates that the
@@ -651,12 +653,41 @@ void GlobalHistogramAllocator::CreateWithSharedMemoryHandle(
 }
 
 // static
+void GlobalHistogramAllocator::Enable() {
+  if (g_allocator) {
+    // Already enabled.
+    DCHECK(!g_allocator_disabled);
+    return;
+  }
+
+  DCHECK(g_allocator_disabled);
+  std::swap(g_allocator, g_allocator_disabled);
+}
+
+// static
+void GlobalHistogramAllocator::Disable() {
+  if (g_allocator_disabled) {
+    // Already disabled.
+    DCHECK(!g_allocator);
+    return;
+  }
+
+  DCHECK(g_allocator);
+  std::swap(g_allocator, g_allocator_disabled);
+}
+
+// static
+bool GlobalHistogramAllocator::IsEnabled() {
+  return g_allocator != nullptr;
+}
+
+// static
 void GlobalHistogramAllocator::Set(
     std::unique_ptr<GlobalHistogramAllocator> allocator) {
   // Releasing or changing an allocator is extremely dangerous because it
   // likely has histograms stored within it. If the backing memory is also
   // also released, future accesses to those histograms will seg-fault.
-  CHECK(!g_allocator);
+  CHECK(!g_allocator && !g_allocator_disabled);
   g_allocator = allocator.release();
   size_t existing = StatisticsRecorder::GetHistogramCount();
 
@@ -670,9 +701,14 @@ GlobalHistogramAllocator* GlobalHistogramAllocator::Get() {
 }
 
 // static
+GlobalHistogramAllocator* GlobalHistogramAllocator::GetEvenIfDisabled() {
+  return g_allocator ? g_allocator : g_allocator_disabled;
+}
+
+// static
 std::unique_ptr<GlobalHistogramAllocator>
 GlobalHistogramAllocator::ReleaseForTesting() {
-  GlobalHistogramAllocator* histogram_allocator = g_allocator;
+  GlobalHistogramAllocator* histogram_allocator = GetEvenIfDisabled();
   if (!histogram_allocator)
     return nullptr;
   PersistentMemoryAllocator* memory_allocator =
@@ -700,10 +736,41 @@ GlobalHistogramAllocator::ReleaseForTesting() {
     DCHECK_NE(kResultHistogram, histogram_data->name);
   }
 
+  DCHECK_NE(g_allocator != nullptr, g_allocator_disabled != nullptr);
   g_allocator = nullptr;
+  g_allocator_disabled = nullptr;
   return WrapUnique(histogram_allocator);
 };
 
+void GlobalHistogramAllocator::SetPersistentLocation(const FilePath& location) {
+  persistent_location_ = location;
+}
+
+bool GlobalHistogramAllocator::WriteToPersistentLocation() {
+#if defined(OS_NACL)
+  // NACL doesn't support file operations, including ImportantFileWriter.
+  NOTREACHED();
+  return false;
+#else
+  // Stop if no destination is set, perhaps because it has not been enabled.

[Ilya Sherman, 2016/04/25 19:48:40]

I was kind of expecting that you'd check the enabled state explicitly, and
return early (sans warning) in just that case.

[bcwhite, 2016/04/25 20:37:17]

"Enabled" has nothing to do with being able to save the state.  It could have
been enabled and then disabled.  If it was never enabled then the amount of data
to save will be very small.

[Ilya Sherman, 2016/04/25 21:12:11]

Why would we enable and then subsequently disable it?  The only reason we even
have enable/disable state both as viable possibilities is that we're not
shipping to 100% of users yet.  But that's a one-time decision, and should only
be able to flip in a single direction.

[bcwhite, 2016/04/26 13:32:30]

The PHA is also used in setup.exe and in subprocesses.  In those cases, they
always start enabled and remain that way.  The many tests that use the PHA also
work that way.  To start disabled would to go against the standard use-case.

Supporting enabled/disabled is a hack to deal with the limitations of not being
able to determine "should I" or "shouldn't I" at the moment when the decision
needs to be made.

When the experiment is removed, I expect enable/disable will also be removed. 
Perhaps Activate/Deactivate would have been a better choice in names.  It's
always operational; it's just not always hooked in.

[Ilya Sherman, 2016/04/26 20:01:24]

Okay, I see your point about this code being used both by Chrome and setup.exe. 
Thanks for explaining that to me.

+  if (persistent_location_.empty()) {
+    LOG(WARNING) << "Could not write \"" << Name() << "\" persistent histograms"
+                 << " to file because no location was set.";

[Ilya Sherman, 2016/04/25 19:48:40]

I still think this should be an error rather than a warning.  What is a code
path that could result in WriteToPersistentLocation() being called without a
persistent location being set, and why is it desirable for that code path to
exist?

[bcwhite, 2016/04/25 20:37:17]

Unlike below, where there is a specific local failure, there is no "error" here.
 This condition is dealing, safely, with how an external caller is using this
module.  This module shouldn't be judging if something outside is wrong.

[Ilya Sherman, 2016/04/25 21:12:11]

If I'm understanding you correctly, then I disagree: IMO this module absolutely
should document its invariants, and fail ungracefully if those invariants are
not satisfied.  I would much rather find broken invariants due to failing
DCHECKs and crash reports rather than by eventually noticing that we seem to
sometimes be dropping data on the floor.

[bcwhite, 2016/04/26 13:32:30]

I can't say I agree but I understand your point.  Done.

+    return false;
+  }
+
+  StringPiece contents(static_cast<const char*>(data()), used());
+  if (!ImportantFileWriter::WriteFileAtomically(persistent_location_,
+                                                contents)) {
+    LOG(ERROR) << "Could not write \"" << Name() << "\" persistent histograms"
+               << " to file: " << persistent_location_.value();
+    return false;
+  }
+
+  return true;
+#endif
+}
+
 GlobalHistogramAllocator::GlobalHistogramAllocator(
     std::unique_ptr<PersistentMemoryAllocator> memory)
     : PersistentHistogramAllocator(std::move(memory)),