Chromium Code Reviews Chromium Code Reviews
Issue 5784005:
Properly lock access to static variables.... (Closed)
Base URL: svn://svn.chromium.org/chrome/trunk/src/| Index: base/metrics/histogram.cc |
| =================================================================== |
| --- base/metrics/histogram.cc (revision 69412) |
| +++ base/metrics/histogram.cc (working copy) |
| @@ -904,12 +904,14 @@ |
| // provide support for all future calls. |
| StatisticsRecorder::StatisticsRecorder() { |
| DCHECK(!histograms_); |
| - lock_ = new Lock; |
| + if (lock_ == NULL) |
| + lock_ = new Lock; |
|
jar (doing other things)
2010/12/22 00:05:16
nit: Please add comment about the leak here, as th
MAD
2010/12/22 21:15:14
Done.
|
| + AutoLock auto_lock(*lock_); |
| histograms_ = new HistogramMap; |
| } |
| StatisticsRecorder::~StatisticsRecorder() { |
| - DCHECK(histograms_); |
| + DCHECK(histograms_ && lock_); |
| if (dump_on_exit_) { |
| std::string output; |
| @@ -917,14 +919,22 @@ |
| 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() { |
| + if (lock_ == NULL) |
| + return false; |
| + AutoLock auto_lock(*lock_); |
| return NULL != histograms_; |
| } |
| @@ -935,10 +945,12 @@ |
| // 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,8 +959,13 @@ |
| // static |
| void StatisticsRecorder::WriteHTMLGraph(const std::string& query, |
| std::string* output) { |
| - if (!histograms_) |
| + if (lock_ == NULL) |
| return; |
| + { |
| + AutoLock auto_lock(*lock_); |
| + if (!histograms_) |
| + return; |
| + } |
|
jar (doing other things)
2010/12/22 00:05:16
nit: Please replace lines 962-968 with:
if (!WasSt
MAD
2010/12/22 21:15:14
Done.
|
| output->append("<html><head><title>About Histograms"); |
| if (!query.empty()) |
| output->append(" - " + query); |
| @@ -971,8 +988,13 @@ |
| // static |
| void StatisticsRecorder::WriteGraph(const std::string& query, |
| std::string* output) { |
| - if (!histograms_) |
| + if (lock_ == NULL) |
| return; |
| + { |
| + AutoLock auto_lock(*lock_); |
| + if (!histograms_) |
| + return; |
| + } |
|
jar (doing other things)
2010/12/22 00:05:16
nit: Please replace lines 991-997 with:
if (!WasSt
MAD
2010/12/22 21:15:14
Done.
|
| if (query.length()) |
| StringAppendF(output, "Collections of histograms for %s\n", query.c_str()); |
| else |
| @@ -990,9 +1012,11 @@ |
| // static |
| void StatisticsRecorder::GetHistograms(Histograms* output) { |
| + if (lock_ == NULL) |
| + return; |
| + AutoLock auto_lock(*lock_); |
| if (!histograms_) |
| return; |
| - AutoLock auto_lock(*lock_); |
| for (HistogramMap::iterator it = histograms_->begin(); |
| histograms_->end() != it; |
| ++it) { |
| @@ -1003,9 +1027,11 @@ |
| bool StatisticsRecorder::FindHistogram(const std::string& name, |
| scoped_refptr<Histogram>* histogram) { |
| + if (lock_ == NULL) |
| + return false; |
| + AutoLock auto_lock(*lock_); |
| if (!histograms_) |
| return false; |
| - AutoLock auto_lock(*lock_); |
| HistogramMap::iterator it = histograms_->find(name); |
| if (histograms_->end() == it) |
| return false; |
| @@ -1016,7 +1042,11 @@ |
| // 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) { |
