Pile of nits for tracked object enablement

base/tracked_objects.cc

 // Copyright (c) 2011 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.
 
 #include "base/tracked_objects.h"
 
 #include <math.h>
 
 #include "base/format_macros.h"
 #include "base/message_loop.h"
@@ skipping 202 matching lines @@
 void ThreadData::OnThreadTerminationCleanup() const {
   tls_index_.Set(NULL);
   if (!is_a_worker_thread_)
     return;
   base::AutoLock lock(*list_lock_);
   unregistered_thread_data_pool_->push(this);
 }
 
 // static
 void ThreadData::WriteHTML(const std::string& query, std::string* output) {
-  if (!ThreadData::tracking_status())
+  if (status_ == UNINITIALIZED)
     return;  // Not yet initialized.
 
   DataCollector collected_data;  // Gather data.
   collected_data.AddListOfLivingObjects();  // Add births that are still alive.
 
   // Data Gathering is complete. Now to sort/process/render.
   DataCollector::Collection* collection = collected_data.collection();
 
   // Create filtering and sort comparison object.
   Comparator comparator;
@@ skipping 165 matching lines @@
   // To avoid conflating our stats with the delay duration in a PostDelayedTask,
   // we identify such tasks, and replace their post_time with the time they
   // were scheduled (requested?) to emerge from the delayed task queue. This
   // means that queueing delay for such tasks will show how long they went
   // unserviced, after they *could* be serviced.  This is the same stat as we
   // have for non-delayed tasks, and we consistently call it queueing delay.
   TrackedTime effective_post_time = completed_task.delayed_run_time.is_null()
       ? tracked_objects::TrackedTime(completed_task.time_posted)
       : tracked_objects::TrackedTime(completed_task.delayed_run_time);
 
-  Duration queue_duration = start_of_run - effective_post_time;
-  Duration run_duration = end_of_run - start_of_run;
+  // Watch out for a race where status_ is changing, and hence one or both
+  // of start_of_run or end_of_run is zero.  IN that case, we didn't bother to
+  // get a time value since we "weren't tracking" and we were trying to be
+  // efficient by not calling for a genuine time value. For simplicity, we'll
+  // use a default zero duration when we can't calculate a true value.
+  Duration queue_duration;
+  Duration run_duration;
+  if (!start_of_run.is_null()) {
+    queue_duration = start_of_run - effective_post_time;
+    if (!end_of_run.is_null())
+      run_duration = end_of_run - start_of_run;
+  }
   current_thread_data->TallyADeath(*birth, queue_duration, run_duration);
 }
 
 // static
 void ThreadData::TallyRunOnWorkerThreadIfTracking(
     const Births* birth,
     const TrackedTime& time_posted,
     const TrackedTime& start_of_run,
     const TrackedTime& end_of_run) {
   if (!kTrackAllTaskObjects)
@@ skipping 11 matching lines @@
   // (since we'll use locks on TallyBirth and TallyDeath).  The good news is
   // that the locks on TallyDeath will be *after* the worker thread has run, and
   // hence nothing will be waiting for the completion (... besides some other
   // thread that might like to run).  Also, the worker threads tasks are
   // generally longer, and hence the cost of the lock may perchance be amortized
   // over the long task's lifetime.
   ThreadData* current_thread_data = Get();
   if (!current_thread_data)
     return;
 
-  Duration queue_duration = start_of_run - time_posted;
-  Duration run_duration = end_of_run - start_of_run;
+  Duration queue_duration;
+  Duration run_duration;
+  if (!start_of_run.is_null()) {
+    queue_duration = start_of_run - time_posted;
+    if (!end_of_run.is_null())
+      run_duration = end_of_run - start_of_run;
+  }
   current_thread_data->TallyADeath(*birth, queue_duration, run_duration);
 }
 
 // static
 ThreadData* ThreadData::first() {
   base::AutoLock lock(*list_lock_);
   return all_thread_data_list_head_;
 }
 
 // This may be called from another thread.
@@ skipping 63 matching lines @@
   return true;
 }
 
 // static
 bool ThreadData::tracking_status() {
   return status_ == ACTIVE;
 }
 
 // static
 TrackedTime ThreadData::Now() {
-  if (!kTrackAllTaskObjects || status_ != ACTIVE)
-    return TrackedTime();  // Super fast when disabled, or not compiled.
-  return TrackedTime::Now();
+  if (kTrackAllTaskObjects && tracking_status())
+    return TrackedTime::Now();
+  return TrackedTime();  // Super fast when disabled, or not compiled.
 }
 
 // static
 void ThreadData::ShutdownSingleThreadedCleanup() {
   // This is only called from test code, where we need to cleanup so that
   // additional tests can be run.
   // We must be single threaded... but be careful anyway.
   if (!InitializeAndSetTrackingStatus(false))
     return;
   ThreadData* thread_data_list;
   ThreadDataPool* final_pool;
   {
     base::AutoLock lock(*list_lock_);
     thread_data_list = all_thread_data_list_head_;
     all_thread_data_list_head_ = NULL;
     final_pool = unregistered_thread_data_pool_;
     unregistered_thread_data_pool_ = NULL;
   }
 
+  // Put most global static back in pristine shape.
+  thread_number_counter_ = 0;
+  tls_index_.Set(NULL);
+  status_ = UNINITIALIZED;
+
+  // To avoid any chance of racing in unit tests, which is the only place we
+  // call this function, we will leak all the data structures we recovered.
+  // These structures could plausibly be used by other threads in earlier tests
+  // that are still running.
+  return;
+
+  // If we wanted to cleanup (on a single thread), here is what we would do.
+
   if (final_pool) {
     // The thread_data_list contains *all* the instances, and we'll use it to
     // delete them.  This pool has pointers to some instances, and we just
     // have to drop those pointers (and not do the deletes here).
     while (!final_pool->empty())
       final_pool->pop();
     delete final_pool;
   }
 
   // Do actual recursive delete in all ThreadData instances.
   while (thread_data_list) {
     ThreadData* next_thread_data = thread_data_list;
     thread_data_list = thread_data_list->next();
 
     for (BirthMap::iterator it = next_thread_data->birth_map_.begin();
          next_thread_data->birth_map_.end() != it; ++it)
       delete it->second;  // Delete the Birth Records.
     next_thread_data->birth_map_.clear();
     next_thread_data->death_map_.clear();
     delete next_thread_data;  // Includes all Death Records.
   }
-  // Put most global static back in pristine shape.
-  thread_number_counter_ = 0;
-  tls_index_.Set(NULL);
-  status_ = UNINITIALIZED;
 }
 
 //------------------------------------------------------------------------------
 // Individual 3-tuple of birth (place and thread) along with death thread, and
 // the accumulated stats for instances (DeathData).
 
 Snapshot::Snapshot(const BirthOnThread& birth_on_thread,
                    const ThreadData& death_thread,
                    const DeathData& death_data)
     : birth_(&birth_on_thread),
@@ skipping 530 matching lines @@
                         (combined_selectors_ & BIRTH_THREAD) ? "*" :
                           sample.birth().birth_thread()->thread_name().c_str(),
                         (combined_selectors_ & DEATH_THREAD) ? "*" :
                           sample.DeathThreadName().c_str());
   sample.birth().location().Write(!(combined_selectors_ & BIRTH_FILE),
                                   !(combined_selectors_ & BIRTH_FUNCTION),
                                   output);
 }
 
 }  // namespace tracked_objects