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
hylschwy · Dec 23, 2010 · d144255 · d144255
1 parent 6d5e825
commit d144255
Show file tree

Hide file tree

Showing 5 changed files with 43 additions and 16 deletions.
diff --git a/base/message_loop.cc b/base/message_loop.cc
@@ -639,7 +639,7 @@ void MessageLoop::EnableHistogrammer(bool enable) {
 
 void MessageLoop::StartHistogrammer() {
   if (enable_histogrammer_ && !message_histogram_.get()
-      && base::StatisticsRecorder::WasStarted()) {
+      && base::StatisticsRecorder::IsActive()) {
     DCHECK(!thread_name_.empty());
     message_histogram_ = base::LinearHistogram::FactoryGet(
         "MsgLoop:" + thread_name_,

diff --git a/base/metrics/histogram.cc b/base/metrics/histogram.cc
@@ -904,27 +904,44 @@ 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;
     WriteGraph("", &output);
     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
@@ -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);

diff --git a/chrome/renderer/renderer_main.cc b/chrome/renderer/renderer_main.cc
@@ -251,7 +251,7 @@ int RendererMain(const MainFunctionParams& parameters) {
   // Initialize histogram statistics gathering system.
   // Don't create StatisticsRecorder in the single process mode.
   scoped_ptr<base::StatisticsRecorder> statistics;
-  if (!base::StatisticsRecorder::WasStarted()) {
+  if (!base::StatisticsRecorder::IsActive()) {
     statistics.reset(new base::StatisticsRecorder());
   }
 

diff --git a/net/socket_stream/socket_stream_metrics_unittest.cc b/net/socket_stream/socket_stream_metrics_unittest.cc
@@ -15,7 +15,7 @@ using base::StatisticsRecorder;
 namespace net {
 
 TEST(SocketStreamMetricsTest, Initialize) {
-  if (!StatisticsRecorder::WasStarted()) {
+  if (!StatisticsRecorder::IsActive()) {
     // Create the recorder if not yet started, as SocketStreamMetrics
     // relys on the StatisticsRecorder to be present. This is useful when
     // tests are run with --gtest_filter='SocketStreamMetricsTest*'.