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