diff options
| author | mad@chromium.org <mad@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-12-23 14:40:10 +0000 |
|---|---|---|
| committer | mad@chromium.org <mad@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-12-23 14:40:10 +0000 |
| commit | d144255434dd27109c30c4e7a8124d76d7e83a4b (patch) | |
| tree | 2d3c9768230161683305bbac9cae4d60cb3b6a06 /base/metrics | |
| parent | 6d5e8255ed5b7e8865fd20af2530953990376f9f (diff) | |
| download | chromium_src-d144255434dd27109c30c4e7a8124d76d7e83a4b.zip chromium_src-d144255434dd27109c30c4e7a8124d76d7e83a4b.tar.gz chromium_src-d144255434dd27109c30c4e7a8124d76d7e83a4b.tar.bz2 | |
Properly lock access to static variables.
There were a few race conditions where the static method would test the validity of the hitograms_ static variable before attempting to use the lock_ to protect access to it but nothing then prevented the destructor to free both the lock_ and the hitograms_ memory.
So I decided to not use a dynamic allocation of the static lock_ to resolve this problem.
BUG=38354
TEST=Hard to repro exit scenario crashes.
Review URL: http://codereview.chromium.org/5784005
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@70054 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'base/metrics')
| -rw-r--r-- | base/metrics/histogram.cc | 51 | ||||
| -rw-r--r-- | base/metrics/histogram.h | 2 |
2 files changed, 40 insertions, 13 deletions
diff --git a/base/metrics/histogram.cc b/base/metrics/histogram.cc index d87640f..75df12e 100644 --- a/base/metrics/histogram.cc +++ b/base/metrics/histogram.cc @@ -904,12 +904,21 @@ double CustomHistogram::GetBucketSize(Count current, size_t i) const { // provide support for all future calls. StatisticsRecorder::StatisticsRecorder() { DCHECK(!histograms_); - lock_ = new Lock; + if (lock_ == NULL) { + // This will leak on purpose. It's the only way to make sure we won't race + // against the static uninitialization of the module while one of our + // static methods relying on the lock get called at an inappropriate time + // during the termination phase. Since it's a static data member, we will + // leak one per process, which would be similar to the instance allocated + // during static initialization and released only on process termination. + lock_ = new Lock; + } + AutoLock auto_lock(*lock_); histograms_ = new HistogramMap; } StatisticsRecorder::~StatisticsRecorder() { - DCHECK(histograms_); + DCHECK(histograms_ && lock_); if (dump_on_exit_) { std::string output; @@ -917,14 +926,22 @@ StatisticsRecorder::~StatisticsRecorder() { LOG(INFO) << output; } // Clean up. - delete histograms_; - histograms_ = NULL; - delete lock_; - lock_ = NULL; + HistogramMap* histograms = NULL; + { + AutoLock auto_lock(*lock_); + histograms = histograms_; + histograms_ = NULL; + } + delete histograms; + // We don't delete lock_ on purpose to avoid having to properly protect + // against it going away after we checked for NULL in the static methods. } // static -bool StatisticsRecorder::WasStarted() { +bool StatisticsRecorder::IsActive() { + if (lock_ == NULL) + return false; + AutoLock auto_lock(*lock_); return NULL != histograms_; } @@ -935,10 +952,12 @@ bool StatisticsRecorder::WasStarted() { // destroyed before assignment (when value was returned by new). // static void StatisticsRecorder::Register(Histogram* histogram) { + if (lock_ == NULL) + return; + AutoLock auto_lock(*lock_); if (!histograms_) return; const std::string name = histogram->histogram_name(); - AutoLock auto_lock(*lock_); // Avoid overwriting a previous registration. if (histograms_->end() == histograms_->find(name)) (*histograms_)[name] = histogram; @@ -947,7 +966,7 @@ void StatisticsRecorder::Register(Histogram* histogram) { // static void StatisticsRecorder::WriteHTMLGraph(const std::string& query, std::string* output) { - if (!histograms_) + if (!IsActive()) return; output->append("<html><head><title>About Histograms"); if (!query.empty()) @@ -971,7 +990,7 @@ void StatisticsRecorder::WriteHTMLGraph(const std::string& query, // static void StatisticsRecorder::WriteGraph(const std::string& query, std::string* output) { - if (!histograms_) + if (!IsActive()) return; if (query.length()) StringAppendF(output, "Collections of histograms for %s\n", query.c_str()); @@ -990,9 +1009,11 @@ void StatisticsRecorder::WriteGraph(const std::string& query, // static void StatisticsRecorder::GetHistograms(Histograms* output) { - if (!histograms_) + if (lock_ == NULL) return; AutoLock auto_lock(*lock_); + if (!histograms_) + return; for (HistogramMap::iterator it = histograms_->begin(); histograms_->end() != it; ++it) { @@ -1003,9 +1024,11 @@ void StatisticsRecorder::GetHistograms(Histograms* output) { bool StatisticsRecorder::FindHistogram(const std::string& name, scoped_refptr<Histogram>* histogram) { - if (!histograms_) + if (lock_ == NULL) return false; AutoLock auto_lock(*lock_); + if (!histograms_) + return false; HistogramMap::iterator it = histograms_->find(name); if (histograms_->end() == it) return false; @@ -1016,7 +1039,11 @@ bool StatisticsRecorder::FindHistogram(const std::string& name, // private static void StatisticsRecorder::GetSnapshot(const std::string& query, Histograms* snapshot) { + if (lock_ == NULL) + return; AutoLock auto_lock(*lock_); + if (!histograms_) + return; for (HistogramMap::iterator it = histograms_->begin(); histograms_->end() != it; ++it) { diff --git a/base/metrics/histogram.h b/base/metrics/histogram.h index 562f252..6b09aa3 100644 --- a/base/metrics/histogram.h +++ b/base/metrics/histogram.h @@ -643,7 +643,7 @@ class StatisticsRecorder { ~StatisticsRecorder(); // Find out if histograms can now be registered into our list. - static bool WasStarted(); + static bool IsActive(); // Register, or add a new histogram to the collection of statistics. static void Register(Histogram* histogram); |