Make BootTimesLoader marker usage thread safe

chrome/browser/chromeos/boot_times_loader.cc

 // Copyright (c) 2012 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 "chrome/browser/chromeos/boot_times_loader.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 275 matching lines @@
   // Append numbers to the files.
   AppendFile(uptime_output, uptime.data(), uptime.size());
   AppendFile(disk_output, disk.data(), disk.size());
 }
 
 // static
 void BootTimesLoader::WriteTimes(
     const std::string base_name,
     const std::string uma_name,
     const std::string uma_prefix,
-    const std::vector<TimeMarker> login_times) {
+    std::vector<TimeMarker> login_times) {
   const int kMinTimeMillis = 1;
   const int kMaxTimeMillis = 30000;
   const int kNumBuckets = 100;
   const FilePath log_path(kLoginLogPath);
 
+  // Need to sort by time since the entries may have been pushed onto the
+  // vector (on the UI thread) in a different order from which they were
+  // created (potentially on other threads).
+  std::sort(login_times.begin(), login_times.end());
+
   base::Time first = login_times.front().time();
   base::Time last = login_times.back().time();
   base::TimeDelta total = last - first;
   base::Histogram* total_hist = base::Histogram::FactoryTimeGet(
       uma_name,
       base::TimeDelta::FromMilliseconds(kMinTimeMillis),
       base::TimeDelta::FromMilliseconds(kMaxTimeMillis),
       kNumBuckets,
       base::Histogram::kUmaTargetedHistogramFlag);
   total_hist->AddTime(total);
@@ skipping 26 matching lines @@
             name.data());
     prev = tm.time();
   }
   output += '\n';
 
   file_util::WriteFile(
       log_path.Append(base_name), output.data(), output.size());
 }
 
 void BootTimesLoader::LoginDone() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   AddLoginTimeMarker("LoginDone", true);
   RecordCurrentStats(kChromeFirstRender);
   registrar_.Remove(this, content::NOTIFICATION_LOAD_START,
                     content::NotificationService::AllSources());
   registrar_.Remove(this, content::NOTIFICATION_LOAD_STOP,
                     content::NotificationService::AllSources());
   registrar_.Remove(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
                     content::NotificationService::AllSources());
   registrar_.Remove(this, content::NOTIFICATION_RENDER_WIDGET_HOST_DID_PAINT,
                     content::NotificationService::AllSources());
   // Don't swamp the FILE thread right away.
   BrowserThread::PostDelayedTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&WriteTimes, kLoginTimes, kUmaLogin, kUmaLoginPrefix,
                  login_time_markers_),
       kLoginTimeWriteDelayMs);
 }
 
 void BootTimesLoader::WriteLogoutTimes() {
+  // Either we're on the browser thread, or (more likely) Chrome is in the
+  // process of shutting down and we're on the main thread but the message loop
+  // has already been terminated.
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
+         !BrowserThread::IsMessageLoopValid(BrowserThread::UI));
+
   WriteTimes(kLogoutTimes,
              kUmaLogout,
              kUmaLogoutPrefix,
              logout_time_markers_);
 }
 
 void BootTimesLoader::RecordStats(const std::string& name, const Stats& stats) {
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&RecordStatsDelayed, name, stats.uptime, stats.disk));
@@ skipping 15 matching lines @@
 
 void BootTimesLoader::SaveChromeMainStats() {
   chrome_main_stats_ = GetCurrentStats();
 }
 
 void BootTimesLoader::RecordChromeMainStats() {
   RecordStats(kChromeMain, chrome_main_stats_);
 }
 
 void BootTimesLoader::RecordLoginAttempted() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   login_time_markers_.clear();
   AddLoginTimeMarker("LoginStarted", false);
   if (!have_registered_) {
     have_registered_ = true;
     registrar_.Add(this, chrome::NOTIFICATION_LOGIN_AUTHENTICATION,
                    content::NotificationService::AllSources());
     registrar_.Add(this, content::NOTIFICATION_LOAD_START,
                    content::NotificationService::AllSources());
     registrar_.Add(this, content::NOTIFICATION_LOAD_STOP,
                    content::NotificationService::AllSources());
     registrar_.Add(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
                    content::NotificationService::AllSources());
     registrar_.Add(this, content::NOTIFICATION_RENDER_WIDGET_HOST_DID_PAINT,
                    content::NotificationService::AllSources());
   }
 }
 
 void BootTimesLoader::AddLoginTimeMarker(
     const std::string& marker_name, bool send_to_uma) {
-  login_time_markers_.push_back(TimeMarker(marker_name, send_to_uma));
+  AddMarker(&login_time_markers_, TimeMarker(marker_name, send_to_uma));
 }
 
 void BootTimesLoader::AddLogoutTimeMarker(
     const std::string& marker_name, bool send_to_uma) {
-  logout_time_markers_.push_back(TimeMarker(marker_name, send_to_uma));
+  AddMarker(&logout_time_markers_, TimeMarker(marker_name, send_to_uma));
+}
+
+// static
+void BootTimesLoader::AddMarker(std::vector<TimeMarker>* vector,
+                                const TimeMarker& marker)
+{

[DaveMoore, 2012/02/27 21:52:19]

Why can't this all be done in AddMarker (with no AddMarkerImpl)? Is this a const
problem?

[Rick Byers, 2012/02/27 21:58:23]

No, AddMarkerImpl is the function that runs on the UI thread (see PostTask
below).  In AddMarker we may be on any thread.   I'm happy to restructure this
to be clearer if you have any suggestions.

+  // The marker vectors can only be safely manipulated on the main thread.
+  // If we're late in the process of shutting down (eg. as can be the case at
+  // logout), then we have to assume we're on the main thread already.
+  if (BrowserThread::CurrentlyOn(BrowserThread::UI) ||
+      !BrowserThread::IsMessageLoopValid(BrowserThread::UI)) {
+    AddMarkerImpl(vector, marker);
+  } else {
+    // Add the marker on the UI thread.
+    // Note that it's safe to use an unretained pointer to the vector because
+    // BootTimesLoader's lifetime exceeds that of the UI thread message loop.
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&BootTimesLoader::AddMarkerImpl,
+                   base::Unretained(vector),
+                   marker));
+  }
+}
+
+// static
+void BootTimesLoader::AddMarkerImpl(std::vector<TimeMarker>* vector,
+                                    TimeMarker marker)
+{
+  vector->push_back(marker);
 }
 
 void BootTimesLoader::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   switch (type) {
     case chrome::NOTIFICATION_LOGIN_AUTHENTICATION: {
       content::Details<AuthenticationNotificationDetails> auth_details(details);
       if (auth_details->success()) {
@@ skipping 38 matching lines @@
           GetRenderWidgetHost(&web_contents->GetController());
       render_widget_hosts_loading_.erase(render_widget_host);
       break;
     }
     default:
       break;
   }
 }
 
 }  // namespace chromeos