[Cronet] Make HistogramManager public APIs thread-safe.

components/cronet/histogram_manager.cc

 // Copyright 2014 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/cronet/histogram_manager.h"
 
 #include <string>
 #include <vector>
 
 #include "base/lazy_instance.h"
@@ skipping 35 matching lines @@
   UMA_HISTOGRAM_ENUMERATION("Histogram.InconsistenciesBrowserUnique.Cronet",
                             problem, base::HistogramBase::NEVER_EXCEEDED_VALUE);
 }
 
 void HistogramManager::InconsistencyDetectedInLoggedCount(int amount) {
   UMA_HISTOGRAM_COUNTS("Histogram.InconsistentSnapshotBrowser.Cronet",
                        std::abs(amount));
 }
 
 bool HistogramManager::GetDeltas(std::vector<uint8_t>* data) {
+  base::AutoLock lock(lock_);

[bcwhite, 2016/04/13 14:12:52]

It doesn't make sense to make multiple, parallel calls to PrepareDeltas while
passing the begin/end iterators from the StatisticsRecorder.  The second thread
through the method will find an empty (or very small) delta.

I suggest explicit Try/Release of the lock and return immediately if the Try
fails.  And rename the lock to indicate that it applies only to the GetDeltas
method.

[pauljensen, 2016/04/13 14:29:45]

It may not make sense to do this, but we shouldn't crash if Cronet users use
this API in a supported way.
We document this.  See the documentation:
 
https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/...
Again, it may not make sense, but we shouldn't crash if Cronet users use this
API in a supported way.
Good idea, done.
I'd rather we keep it with a broader usage connotation so that future changes
can use it...rather than fall into the prior inadvertently thread-unsafe
mechanisms.

[bcwhite, 2016/04/13 14:42:18]

If this method were to be called while some "future" method owned the lock, the
Try() would fail (and return) and filling of the protobuf might never happen.

   // Clear the protobuf between calls.
   uma_proto_.Clear();
   // "false" to StatisticsRecorder::begin() indicates to *not* include
   // histograms held in persistent storage on the assumption that they will be
   // visible to the recipient through other means.
   histogram_snapshot_manager_.PrepareDeltas(
       base::StatisticsRecorder::begin(false), base::StatisticsRecorder::end(),
       base::Histogram::kNoFlags, base::Histogram::kUmaTargetedHistogramFlag);
   int32_t data_size = uma_proto_.ByteSize();
   data->resize(data_size);
   if (uma_proto_.SerializeToArray(&(*data)[0], data_size))
     return true;
   data->clear();
   return false;
 }
 
 }  // namespace cronet