Cleanup histogram synchronizer

Some of the assertions in histogram synchronizer, specifying
thread-to-run-on, were breaking. We used to use a
fancy mix of threads and locks to access the data
involving which sequence was pending, and this
transitions to clean use of locking only for
all members of the class.

BUG=62810
r=wtc
Review URL: http://codereview.chromium.org/5189002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@66861 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 188 insertions, 171 deletions

diff --git a/chrome/browser/metrics/histogram_synchronizer.cc b/chrome/browser/metrics/histogram_synchronizer.cc
index a84d6c7..4b82505 100644
--- a/chrome/browser/metrics/histogram_synchronizer.cc
+++ b/chrome/browser/metrics/histogram_synchronizer.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -16,6 +16,11 @@ using base::Time;
 using base::TimeDelta;
 using base::TimeTicks;
 
+// Negative numbers are never used as sequence numbers.  We explicitly pick a
+// negative number that is "so negative" that even when we add one (as is done
+// when we generated the next sequence number) that it will still be negative.
+// We have code that handles wrapping around on an overflow into negative
+// territory.
 static const int kNeverUsableSequenceNumber = -2;
 
 HistogramSynchronizer::HistogramSynchronizer()
@@ -23,10 +28,10 @@ HistogramSynchronizer::HistogramSynchronizer()
     received_all_renderer_histograms_(&lock_),
     callback_task_(NULL),
     callback_thread_(NULL),
-    next_available_sequence_number_(kNeverUsableSequenceNumber),
+    last_used_sequence_number_(kNeverUsableSequenceNumber),
     async_sequence_number_(kNeverUsableSequenceNumber),
     async_renderers_pending_(0),
-    async_callback_start_time_(TimeTicks::Now()),
+    async_callback_start_time_(TimeTicks::TimeTicks()),
     synchronous_sequence_number_(kNeverUsableSequenceNumber),
     synchronous_renderers_pending_(0) {
   DCHECK(histogram_synchronizer_ == NULL);
@@ -34,10 +39,8 @@ HistogramSynchronizer::HistogramSynchronizer()
 }
 
 HistogramSynchronizer::~HistogramSynchronizer() {
-  // Clean up.
-  delete callback_task_;
-  callback_task_ = NULL;
-  callback_thread_ = NULL;
+  // Just in case we have any pending tasks, clear them out.
+  SetCallbackTaskAndThread(NULL, NULL);
   histogram_synchronizer_ = NULL;
 }
 
@@ -49,17 +52,7 @@ HistogramSynchronizer* HistogramSynchronizer::CurrentSynchronizer() {
 
 void HistogramSynchronizer::FetchRendererHistogramsSynchronously(
     TimeDelta wait_time) {
-  DCHECK(MessageLoop::current()->type() == MessageLoop::TYPE_UI);
-
-  int sequence_number = GetNextAvailableSequenceNumber(SYNCHRONOUS_HISTOGRAMS);
-  for (RenderProcessHost::iterator it(RenderProcessHost::AllHostsIterator());
-       !it.IsAtEnd(); it.Advance()) {
-    IncrementPendingRenderers(SYNCHRONOUS_HISTOGRAMS);
-    it.GetCurrentValue()->Send(
-        new ViewMsg_GetRendererHistograms(sequence_number));
-  }
-  // Send notification that we're done sending messages to renderers.
-  DecrementPendingRenderers(sequence_number);
+  NotifyAllRenderers(SYNCHRONOUS_HISTOGRAMS);
 
   TimeTicks start = TimeTicks::Now();
   TimeTicks end_time = start + wait_time;
@@ -86,12 +79,10 @@ void HistogramSynchronizer::FetchRendererHistogramsAsynchronously(
     MessageLoop* callback_thread,
     Task* callback_task,
     int wait_time) {
-  DCHECK(MessageLoop::current()->type() == MessageLoop::TYPE_UI);
   DCHECK(callback_thread != NULL);
   DCHECK(callback_task != NULL);
 
-  HistogramSynchronizer* current_synchronizer =
-      HistogramSynchronizer::CurrentSynchronizer();
+  HistogramSynchronizer* current_synchronizer = CurrentSynchronizer();
 
   if (current_synchronizer == NULL) {
     // System teardown is happening.
@@ -99,30 +90,17 @@ void HistogramSynchronizer::FetchRendererHistogramsAsynchronously(
     return;
   }
 
-  // callback_task_ member can only be accessed on IO thread.
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      NewRunnableMethod(
-          current_synchronizer,
-          &HistogramSynchronizer::SetCallbackTaskToCallAfterGettingHistograms,
-          callback_thread,
-          callback_task));
+  current_synchronizer->SetCallbackTaskAndThread(callback_thread,
+                                                 callback_task);
 
-  // Tell all renderer processes to send their histograms.
   int sequence_number =
-      current_synchronizer->GetNextAvailableSequenceNumber(ASYNC_HISTOGRAMS);
-  for (RenderProcessHost::iterator it(RenderProcessHost::AllHostsIterator());
-       !it.IsAtEnd(); it.Advance()) {
-    current_synchronizer->IncrementPendingRenderers(ASYNC_HISTOGRAMS);
-    it.GetCurrentValue()->Send(
-        new ViewMsg_GetRendererHistograms(sequence_number));
-  }
-  // Send notification that we're done sending messages to renderers.
-  current_synchronizer->DecrementPendingRenderers(sequence_number);
+      current_synchronizer->NotifyAllRenderers(ASYNC_HISTOGRAMS);
 
-  // Post a task that would be called after waiting for wait_time.
+  // Post a task that would be called after waiting for wait_time.  This acts
+  // as a watchdog, to ensure that a non-responsive renderer won't block us from
+  // making the callback.
   BrowserThread::PostDelayedTask(
-      BrowserThread::IO, FROM_HERE,
+      BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(
           current_synchronizer,
           &HistogramSynchronizer::ForceHistogramSynchronizationDoneCallback,
@@ -134,145 +112,147 @@ void HistogramSynchronizer::FetchRendererHistogramsAsynchronously(
 void HistogramSynchronizer::DeserializeHistogramList(
     int sequence_number,
     const std::vector<std::string>& histograms) {
-  HistogramSynchronizer* current_synchronizer =
-      HistogramSynchronizer::CurrentSynchronizer();
-  if (current_synchronizer == NULL)
-    return;
-
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-
   for (std::vector<std::string>::const_iterator it = histograms.begin();
        it < histograms.end();
        ++it) {
     base::Histogram::DeserializeHistogramInfo(*it);
   }
 
+  HistogramSynchronizer* current_synchronizer = CurrentSynchronizer();
+  if (current_synchronizer == NULL)
+    return;
+
   // Record that we have received a histogram from renderer process.
   current_synchronizer->DecrementPendingRenderers(sequence_number);
 }
 
-bool HistogramSynchronizer::DecrementPendingRenderers(int sequence_number) {
-  if (sequence_number == async_sequence_number_) {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-    if ((async_renderers_pending_ == 0) ||
-        (--async_renderers_pending_ > 0))
-      return false;
-    DCHECK(callback_task_ != NULL);
-    CallCallbackTaskAndResetData();
-    return true;
+int HistogramSynchronizer::NotifyAllRenderers(
+    RendererHistogramRequester requester) {
+  // To iterate over RenderProcessHosts, or to send messages to the hosts, we
+  // need to be on the UI thread.
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  int notification_count = 0;
+  for (RenderProcessHost::iterator it(RenderProcessHost::AllHostsIterator());
+       !it.IsAtEnd(); it.Advance())
+     ++notification_count;
+
+  int sequence_number = GetNextAvailableSequenceNumber(requester,
+                                                       notification_count);
+  for (RenderProcessHost::iterator it(RenderProcessHost::AllHostsIterator());
+       !it.IsAtEnd(); it.Advance()) {
+    if (!it.GetCurrentValue()->Send(
+        new ViewMsg_GetRendererHistograms(sequence_number)))
+      DecrementPendingRenderers(sequence_number);
   }
 
+  return sequence_number;
+}
+
+void HistogramSynchronizer::DecrementPendingRenderers(int sequence_number) {
+  bool synchronous_completed = false;
+  bool asynchronous_completed = false;
+
   {
     AutoLock auto_lock(lock_);
-    if (sequence_number != synchronous_sequence_number_) {
-      // No need to do anything if the sequence_number does not match current
-      // synchronous_sequence_number_ or async_sequence_number_.
-      return true;
+    if (sequence_number == async_sequence_number_) {
+      if (--async_renderers_pending_ <= 0)
+        asynchronous_completed = true;
+    } else if (sequence_number == synchronous_sequence_number_) {
+      if (--synchronous_renderers_pending_ <= 0)
+        synchronous_completed = true;
     }
-    if (--synchronous_renderers_pending_ > 0)
-      return false;
-    DCHECK_EQ(synchronous_renderers_pending_, 0);
   }
 
-  // We can call Signal() without holding the lock.
-  received_all_renderer_histograms_.Signal();
-  return true;
+  if (asynchronous_completed)
+    ForceHistogramSynchronizationDoneCallback(sequence_number);
+  else if (synchronous_completed)
+    received_all_renderer_histograms_.Signal();
 }
 
-// This method is called on the IO thread.
-void HistogramSynchronizer::SetCallbackTaskToCallAfterGettingHistograms(
+void HistogramSynchronizer::SetCallbackTaskAndThread(
     MessageLoop* callback_thread,
     Task* callback_task) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-
-  // Test for the existence of a previous task, and post call to post it if it
-  // exists. We promised to post it after some timeout... and at this point, we
-  // should just force the posting.
-  if (callback_task_ != NULL) {
-    CallCallbackTaskAndResetData();
+  Task* old_task = NULL;
+  MessageLoop* old_thread = NULL;
+  TimeTicks old_start_time;
+  int unresponsive_renderers;
+  const TimeTicks now = TimeTicks::Now();
+  {
+    AutoLock auto_lock(lock_);
+    old_task = callback_task_;
+    callback_task_ = callback_task;
+    old_thread = callback_thread_;
+    callback_thread_ = callback_thread;
+    unresponsive_renderers = async_renderers_pending_;
+    old_start_time = async_callback_start_time_;
+    async_callback_start_time_ = now;
+    // Prevent premature calling of our new callbacks.
+    async_sequence_number_ = kNeverUsableSequenceNumber;
   }
-
-  // Assert there was no callback_task_ already.
-  DCHECK(callback_task_ == NULL);
-
-  // Save the thread and the callback_task.
-  DCHECK(callback_thread != NULL);
-  DCHECK(callback_task != NULL);
-  callback_task_ = callback_task;
-  callback_thread_ = callback_thread;
-  async_callback_start_time_ = TimeTicks::Now();
+  // Just in case there was a task pending....
+  InternalPostTask(old_thread, old_task, unresponsive_renderers,
+                   old_start_time);
 }
 
 void HistogramSynchronizer::ForceHistogramSynchronizationDoneCallback(
     int sequence_number) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-
-  if (sequence_number == async_sequence_number_) {
-     CallCallbackTaskAndResetData();
+  Task* task = NULL;
+  MessageLoop* thread = NULL;
+  TimeTicks started;
+  int unresponsive_renderers;
+  {
+    AutoLock lock(lock_);
+    if (sequence_number != async_sequence_number_)
+      return;
+    task = callback_task_;
+    thread = callback_thread_;
+    callback_task_ = NULL;
+    callback_thread_ = NULL;
+    started = async_callback_start_time_;
+    unresponsive_renderers = async_renderers_pending_;
   }
+  InternalPostTask(thread, task, unresponsive_renderers, started);
 }
 
-// If wait time has elapsed or if we have received all the histograms from all
-// the renderers, call the callback_task if a callback_task exists. This is
-// called on IO Thread.
-void HistogramSynchronizer::CallCallbackTaskAndResetData() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-
-  // callback_task_ would be set to NULL, if we have heard from all renderers
-  // and we would have called the callback_task already.
-  if (callback_task_ == NULL) {
+void HistogramSynchronizer::InternalPostTask(MessageLoop* thread, Task* task,
+                                             int unresponsive_renderers,
+                                             const base::TimeTicks& started) {
+  if (!task || !thread)
     return;
-  }
-
   UMA_HISTOGRAM_COUNTS("Histogram.RendersNotRespondingAsynchronous",
-                       async_renderers_pending_);
-  if (!async_renderers_pending_)
+                       unresponsive_renderers);
+  if (!unresponsive_renderers) {
     UMA_HISTOGRAM_TIMES("Histogram.FetchRendererHistogramsAsynchronously",
-                        TimeTicks::Now() - async_callback_start_time_);
+                        TimeTicks::Now() - started);
+  }
 
-  DCHECK(callback_thread_ != NULL);
-  DCHECK(callback_task_ != NULL);
-  callback_thread_->PostTask(FROM_HERE, callback_task_);
-  async_renderers_pending_ = 0;
-  async_callback_start_time_ = TimeTicks::Now();
-  callback_task_ = NULL;
-  callback_thread_ = NULL;
-  async_sequence_number_ = kNeverUsableSequenceNumber;
+  thread->PostTask(FROM_HERE, task);
 }
 
 int HistogramSynchronizer::GetNextAvailableSequenceNumber(
-    RendererHistogramRequester requester) {
+    RendererHistogramRequester requester,
+    int renderer_count) {
   AutoLock auto_lock(lock_);
-  ++next_available_sequence_number_;
-  if (0 > next_available_sequence_number_) {
-    // We wrapped around, so we need to bypass the reserved number.
-    next_available_sequence_number_ =
+  ++last_used_sequence_number_;
+  // Watch out for wrapping to a negative number.
+  if (last_used_sequence_number_ < 0) {
+    // Bypass the reserved number, which is used when a renderer spontaneously
+    // decides to send some histogram data.
+    last_used_sequence_number_ =
         chrome::kHistogramSynchronizerReservedSequenceNumber + 1;
   }
-  DCHECK_NE(next_available_sequence_number_,
+  DCHECK_NE(last_used_sequence_number_,
             chrome::kHistogramSynchronizerReservedSequenceNumber);
   if (requester == ASYNC_HISTOGRAMS) {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-    async_sequence_number_ = next_available_sequence_number_;
-    async_renderers_pending_ = 1;
+    async_sequence_number_ = last_used_sequence_number_;
+    async_renderers_pending_ = renderer_count;
   } else if (requester == SYNCHRONOUS_HISTOGRAMS) {
-    synchronous_sequence_number_ = next_available_sequence_number_;
-    synchronous_renderers_pending_ = 1;
-  }
-  return next_available_sequence_number_;
-}
-
-void HistogramSynchronizer::IncrementPendingRenderers(
-    RendererHistogramRequester requester) {
-  if (requester == ASYNC_HISTOGRAMS) {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-    DCHECK_GT(async_renderers_pending_, 0);
-    ++async_renderers_pending_;
-  } else {
-    AutoLock auto_lock(lock_);
-    DCHECK_GT(synchronous_renderers_pending_, 0);
-    ++synchronous_renderers_pending_;
+    synchronous_sequence_number_ = last_used_sequence_number_;
+    synchronous_renderers_pending_ = renderer_count;
   }
+  return last_used_sequence_number_;
 }
 
 // static
diff --git a/chrome/browser/metrics/histogram_synchronizer.h b/chrome/browser/metrics/histogram_synchronizer.h
index bebd6d2..2f75a63 100644
--- a/chrome/browser/metrics/histogram_synchronizer.h
+++ b/chrome/browser/metrics/histogram_synchronizer.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2006-2010 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.
 
@@ -18,6 +18,36 @@
 class MessageLoop;
 class Task;
 
+// This class maintains state that is used to upload histogram data from the
+// various renderer processes, into the browser process.  Such transactions are
+// usually instigated by the browser.  In general, a renderer process will
+// respond by gathering snapshots of all internal histograms, calculating what
+// has changed since its last upload, and transmitting a pickled collection of
+// deltas.
+//
+// There are actually two modes of update request.  One is synchronous (and
+// blocks the UI thread, waiting to populate an about:histograms tab) and the
+// other is asynchronous, and used by the metrics services in preparation for a
+// log upload.
+//
+// To assure that all the renderers have responded, a counter is maintained (for
+// each mode) to indicate the number of pending (not yet responsive) renderers.
+// To avoid confusion about a response (i.e., is the renderer responding to a
+// current request for an update, or to an old request for an update) we tag
+// each group of requests with a sequence number.  When an update arrives we can
+// ignore it (relative to the counter) if it does not relate to a current
+// outstanding sequence number.
+//
+// There is one final mode of use, where a renderer spontaneously decides to
+// transmit a collection of histogram data.  This is designed for use when the
+// renderer is terminating.  Unfortunately, renders may be terminated without
+// warning, and the best we can do is periodically acquire data from a tab, such
+// as when a page load has completed.  In this mode, the renderer uses a
+// reserved sequence number, different from any sequence number that might be
+// specified by a browser request.  Since this sequence number can't match an
+// outstanding sequence number, the pickled data is accepted into the browser,
+// but there is no impact on the counters.
+
 class HistogramSynchronizer : public
     base::RefCountedThreadSafe<HistogramSynchronizer> {
  public:
@@ -27,6 +57,10 @@ class HistogramSynchronizer : public
     SYNCHRONOUS_HISTOGRAMS
   };
 
+  // Construction also sets up the global singleton instance.  This instance is
+  // used to communicate between the IO and UI thread, and is destroyed only
+  // as the main thread (browser_main) terminates, which means the IO thread has
+  // already completed, and will not need this instance any further.
   HistogramSynchronizer();
 
   ~HistogramSynchronizer();
@@ -43,46 +77,49 @@ class HistogramSynchronizer : public
 
   // Contact all renderers, and get them to upload to the browser any/all
   // changes to histograms.  When all changes have been acquired, or when the
-  // wait time expires (whichever is sooner), post the callback_task to the UI
-  // thread. Note the callback_task is posted exactly once. This method is
-  // called on the IO thread from UMA via PostMessage.
+  // wait time expires (whichever is sooner), post the callback_task to the
+  // specified thread. Note the callback_task is posted exactly once.
   static void FetchRendererHistogramsAsynchronously(
       MessageLoop* callback_thread, Task* callback_task, int wait_time);
 
-  // This method is called on the IO thread. Desrializes the histograms and
+  // This method is called on the IO thread. Deserializes the histograms and
   // records that we have received histograms from a renderer process.
   static void DeserializeHistogramList(
       int sequence_number, const std::vector<std::string>& histograms);
 
  private:
-  // Records that we are waiting for one less histogram from a renderer for the
-  // given sequence number. If we have received a response from all histograms,
-  // either signal the waiting process or call the callback function. Returns
-  // true when we receive histograms from the last of N renderers that were
-  // contacted for an update.
-  bool DecrementPendingRenderers(int sequence_number);
+  // Establish a new sequence_number_, and use it to notify all the renderers of
+  // the need to supply, to the browser, any changes in their histograms.
+  // The argument indicates whether this will set async_sequence_number_ or
+  // synchronous_sequence_number_.
+  // Return the sequence number that was used.
+  int NotifyAllRenderers(RendererHistogramRequester requester);
 
-  void SetCallbackTaskToCallAfterGettingHistograms(
-      MessageLoop* callback_thread, Task* callback_task);
+  // Records that we are waiting for one less histogram from a renderer for the
+  // given sequence number. If we have received a response from all renderers,
+  // either signal the waiting process or call the callback function.
+  void DecrementPendingRenderers(int sequence_number);
+
+  // Set the callback_thread_ and callback_task_ members. If these members
+  // already had values, then as a side effect, post the old callback_task_ to
+  // the old callaback_thread_.  This side effect should not generally happen,
+  // but is in place to assure correctness (that any tasks that were set, are
+  // eventually called, and never merely discarded).
+  void SetCallbackTaskAndThread(MessageLoop* callback_thread,
+                                Task* callback_task);
 
   void ForceHistogramSynchronizationDoneCallback(int sequence_number);
 
-  // Calls the callback task, if there is a callback_task.
-  void CallCallbackTaskAndResetData();
-
-  // Gets a new sequence number to be sent to renderers from browser process.
-  // This will also reset the count of pending renderers for the given type to
-  // 1.  After all calls to renderers have been made, a call to
-  // DecrementPendingRenderers() must be mode to make it possible for the
-  // counter to go to zero (after all renderers have responded).
-  int GetNextAvailableSequenceNumber(RendererHistogramRequester requster);
+  // Gets a new sequence number to be sent to renderers from browser process and
+  // set the number of pending responses for the given type to renderer_count.
+  int GetNextAvailableSequenceNumber(RendererHistogramRequester requster,
+                                     int renderer_count);
 
-  // Increments the count of the renderers we're waiting for for the request
-  // of the given type.
-  void IncrementPendingRenderers(RendererHistogramRequester requester);
+  // Internal helper function, to post task, and record callback stats.
+  void InternalPostTask(MessageLoop* thread, Task* task,
+      int unresponsive_renderers, const base::TimeTicks& started);
 
-  // This lock_ protects access to next_sequence_number_,
-  // synchronous_renderers_pending_, and synchronous_sequence_number_.
+  // This lock_ protects access to all members.
   Lock lock_;
 
   // This condition variable is used to block caller of the synchronous request
@@ -101,28 +138,28 @@ class HistogramSynchronizer : public
   // sure a response from a renderer is associated with the current round of
   // requests (and not merely a VERY belated prior response).
   // All sequence numbers used are non-negative.
-  // next_available_sequence_number_ is the next available number (used to
-  // avoid reuse for a long time).  Access is protected by lock_.
-  int next_available_sequence_number_;
+  // last_used_sequence_number_ is the most recently used number (used to avoid
+  // reuse for a long time).
+  int last_used_sequence_number_;
 
   // The sequence number used by the most recent asynchronous update request to
-  // contact all renderers.  Access is only permitted on the IO thread.
+  // contact all renderers.
   int async_sequence_number_;
 
   // The number of renderers that have not yet responded to requests (as part of
-  // an asynchronous update).  Access is only permitted on the IO thread.
+  // an asynchronous update).
   int async_renderers_pending_;
 
   // The time when we were told to start the fetch histograms asynchronously
-  // from renderers.  Access is only permitted on the IO thread.
+  // from renderers.
   base::TimeTicks async_callback_start_time_;
 
   // The sequence number used by the most recent synchronous update request to
-  // contact all renderers.  Protected by lock_.
+  // contact all renderers.
   int synchronous_sequence_number_;
 
   // The number of renderers that have not yet responded to requests (as part of
-  // a synchronous update).  Protected by lock_.
+  // a synchronous update).
   int synchronous_renderers_pending_;
 
   // This singleton instance should be started during the single threaded