Skip to content

Commit

Permalink
[Cronet] Make HistogramManager public APIs thread-safe.
Browse files Browse the repository at this point in the history
CronetEngine.getGlobalMetricsDeltas() exposes the underlying
HistogramManager public APIs on multiple threads, so crashes
can occur if accessed concurrently. The current public APIs
are GetInstance() which simply accesses a thread-safe
LazyInstance, and GetDeltas() which doesn't look thread-safe
so I've added a Lock.

BUG=603028

Review URL: https://codereview.chromium.org/1882563004

Cr-Commit-Position: refs/heads/master@{#387009}
  • Loading branch information
JensenPaul authored and Commit bot committed Apr 13, 2016
1 parent 136fbbc commit 487de2d
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 13 deletions.
27 changes: 15 additions & 12 deletions components/cronet/histogram_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -53,18 +53,21 @@ void HistogramManager::InconsistencyDetectedInLoggedCount(int amount) {
}

bool HistogramManager::GetDeltas(std::vector<uint8_t>* data) {
// 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;
if (get_deltas_lock_.Try()) {
base::AutoLock lock(get_deltas_lock_, base::AutoLock::AlreadyAcquired());
// 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;
}
Expand Down
7 changes: 6 additions & 1 deletion components/cronet/histogram_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,14 @@
#include "base/memory/scoped_ptr.h"
#include "base/metrics/histogram_flattener.h"
#include "base/metrics/histogram_snapshot_manager.h"
#include "base/synchronization/lock.h"
#include "components/metrics/proto/chrome_user_metrics_extension.pb.h"

namespace cronet {

// A HistogramManager instance is created by the app. It is the central
// controller for the acquisition of log data, and recording deltas for
// transmission to an external server.
// transmission to an external server. Public APIs are all thread-safe.
class HistogramManager : public base::HistogramFlattener {
public:
HistogramManager();
Expand Down Expand Up @@ -52,6 +53,10 @@ class HistogramManager : public base::HistogramFlattener {
// Stores the protocol buffer representation for this log.
metrics::ChromeUserMetricsExtension uma_proto_;

// Should be acquired whenever GetDeltas() is executing to maintain
// thread-safety.
base::Lock get_deltas_lock_;

DISALLOW_COPY_AND_ASSIGN(HistogramManager);
};

Expand Down

0 comments on commit 487de2d

Please sign in to comment.