Refactored DeviceStatusCollector to enable truely asynchronous status queries

chrome/browser/chromeos/policy/device_status_collector.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/policy/device_status_collector.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
 #include <cstdio>
 #include <limits>
 #include <memory>
 #include <sstream>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
 #include "base/format_macros.h"
-#include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/sys_info.h"
 #include "base/task_runner_util.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/values.h"
 #include "base/version.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/app_mode/kiosk_app_manager.h"
 #include "chrome/browser/chromeos/login/users/chrome_user_manager.h"
 #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
 #include "chrome/browser/chromeos/policy/device_local_account.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/update_engine_client.h"
 #include "chromeos/disks/disk_mount_manager.h"
 #include "chromeos/network/device_state.h"
 #include "chromeos/network/network_handler.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
 #include "chromeos/settings/cros_settings_names.h"
 #include "chromeos/system/statistics_provider.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #include "components/policy/proto/device_management_backend.pb.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
 #include "components/user_manager/user.h"
 #include "components/user_manager/user_manager.h"
 #include "components/user_manager/user_type.h"
 #include "components/version_info/version_info.h"
+#include "content/public/browser/browser_thread.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
 #include "storage/browser/fileapi/external_mount_points.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 using base::Time;
 using base::TimeDelta;
 
 namespace em = enterprise_management;
 
 namespace {
 // How many seconds of inactivity triggers the idle state.
 const int kIdleStateThresholdSeconds = 300;
 
 // How many days in the past to store active periods for.
 const unsigned int kMaxStoredPastActivityDays = 30;
 
 // How many days in the future to store active periods for.
 const unsigned int kMaxStoredFutureActivityDays = 2;
 
-// How often, in seconds, to update the device location.
-const unsigned int kGeolocationPollIntervalSeconds = 30 * 60;
-
-// How often, in seconds, to sample the hardware state.
-const unsigned int kHardwareStatusSampleIntervalSeconds = 120;
-
-// Keys for the geolocation status dictionary in local state.
-const char kLatitude[] = "latitude";
-const char kLongitude[] = "longitude";
-const char kAltitude[] = "altitude";
-const char kAccuracy[] = "accuracy";
-const char kAltitudeAccuracy[] = "altitude_accuracy";
-const char kHeading[] = "heading";
-const char kSpeed[] = "speed";
-const char kTimestamp[] = "timestamp";
+// How often, in seconds, to sample the hardware resource usage.
+const unsigned int kResourceUsageSampleIntervalSeconds = 120;
 
 // The location we read our CPU statistics from.
 const char kProcStat[] = "/proc/stat";
 
 // The location we read our CPU temperature and channel label from.
 const char kHwmonDir[] = "/sys/class/hwmon/";
 const char kDeviceDir[] = "device";
 const char kHwmonDirectoryPattern[] = "hwmon*";
 const char kCPUTempFilePattern[] = "temp*_input";
 
@@ skipping 162 matching lines @@
   // (crbug.com/463334).
   DCHECK_GT(signal_strength, 0);
   DCHECK_LE(signal_strength, 100);
   return signal_strength - 120;
 }
 
 }  // namespace
 
 namespace policy {
 
+// Helper class for state tracking of async status queries.

[Andrew T Wilson (Slow), 2016/09/26 16:33:42]

This does clever stuff in the destructor - you should have class-level
documentation of that behavior and how this class is intended to be used.

[ljusten (tachyonic), 2016/09/27 16:53:54]

Done.

+class GetStatusState : public base::RefCountedThreadSafe<GetStatusState> {
+ public:
+  explicit GetStatusState(
+      const scoped_refptr<base::SequencedTaskRunner> task_runner,
+      const policy::DeviceStatusCollector::StatusCallback& response)
+      : task_runner_(task_runner),
+        response_(response),
+        device_status_(base::MakeUnique<em::DeviceStatusReportRequest>()),

[Andrew T Wilson (Slow), 2016/09/26 16:33:42]

I thought unique_ptr initialized itself to null - do you have to have an
explicit initialization (and nullptr isn't sufficient, you actually need a
MakeUnique)?

Not saying this is wrong - I haven't written enough code with unique_ptr to have
a good grasp of the semantics, just checking.

[Thiemo Nagel, 2016/09/27 16:39:29]

+1 to this.  Also, for new code we prefer in-class initialization over
in-constructor initialization.  But a unique_ptr doesn't require any
initialization at all.

[ljusten (tachyonic), 2016/09/27 16:53:55]

unique_ptr does initialize itself to nullptr, but in this case I want to create
empty status blobs because the logic is easier this way. The logic roughly is:

- Create status blobs
- Call all Get* methods and record whether they actually wrote status
- If none of them wrote any status, delete the blob again

This will create a blob and destroy it again if none of the Get* methods reports
anything, but this is much easier and cleaner to do than the alternative:

- Initialize status blobs with null
- Check whether any of the Get* methods wants to report anything
- If so, create status blob and actually call the Get* methods

+        session_status_(base::MakeUnique<em::SessionStatusReportRequest>()) {}
+
+  inline em::DeviceStatusReportRequest* device_status() {
+    return device_status_.get();
+  }
+
+  inline em::SessionStatusReportRequest* session_status() {
+    return session_status_.get();
+  }
+
+  inline void ResetDeviceStatus() { device_status_.reset(); }
+
+  inline void ResetSessionStatus() { session_status_.reset(); }
+
+  // Queues an async callback to query disk volume information.
+  void SampleVolumeInfo(const policy::DeviceStatusCollector::VolumeInfoFetcher&
+                            volume_info_fetcher) {
+    // Create list of mounted disk volumes to query status.
+    std::vector<storage::MountPoints::MountPointInfo> external_mount_points;
+    storage::ExternalMountPoints::GetSystemInstance()->AddMountPointInfosTo(
+        &external_mount_points);
+
+    std::vector<std::string> mount_points;
+    for (const auto& info : external_mount_points)
+      mount_points.push_back(info.path.value());
+
+    for (const auto& mount_info :
+         chromeos::disks::DiskMountManager::GetInstance()->mount_points()) {
+      // Extract a list of mount points to populate.
+      mount_points.push_back(mount_info.first);
+    }
+
+    // Call out to the blocking pool to sample disk volume info.
+    base::PostTaskAndReplyWithResult(
+        content::BrowserThread::GetBlockingPool(), FROM_HERE,
+        base::Bind(volume_info_fetcher, mount_points),
+        base::Bind(&GetStatusState::OnVolumeInfoReceived, this));
+  }
+
+  // Queues an async callback to query CPU temperator information.

[Andrew T Wilson (Slow), 2016/09/26 16:33:42]

nit: temperator->temperature

[ljusten (tachyonic), 2016/09/27 16:53:55]

Done.

+  void SampleCPUTempInfo(
+      const policy::DeviceStatusCollector::CPUTempFetcher& cpu_temp_fetcher) {
+    // Call out to the blocking pool to sample CPU temp.
+    base::PostTaskAndReplyWithResult(
+        content::BrowserThread::GetBlockingPool(), FROM_HERE, cpu_temp_fetcher,
+        base::Bind(&GetStatusState::OnCPUTempInfoReceived, this));
+  }
+
+ private:
+  friend class RefCountedThreadSafe<GetStatusState>;
+
+  // Posts the response on the UI thread. As long as there is an outstanding
+  // async query, the query holds a reference to us, so the destructor is
+  // not called.
+  ~GetStatusState() {
+    task_runner_->PostTask(FROM_HERE,
+                           base::Bind(response_, base::Passed(&device_status_),
+                                      base::Passed(&session_status_)));
+  }
+
+  void OnVolumeInfoReceived(const std::vector<em::VolumeInfo>& volume_info) {
+    device_status_->clear_volume_info();
+    for (const em::VolumeInfo& info : volume_info)
+      *device_status_->add_volume_info() = info;
+  }
+
+  void OnCPUTempInfoReceived(
+      const std::vector<em::CPUTempInfo>& cpu_temp_info) {
+    if (cpu_temp_info.empty())
+      DLOG(WARNING) << "Unable to read CPU temp information.";
+
+    device_status_->clear_cpu_temp_info();
+    for (const em::CPUTempInfo& info : cpu_temp_info)
+      *device_status_->add_cpu_temp_info() = info;
+  }
+
+  const scoped_refptr<base::SequencedTaskRunner> task_runner_;
+  policy::DeviceStatusCollector::StatusCallback response_;
+  std::unique_ptr<em::DeviceStatusReportRequest> device_status_;
+  std::unique_ptr<em::SessionStatusReportRequest> session_status_;
+};
+
 DeviceStatusCollector::DeviceStatusCollector(
     PrefService* local_state,
     chromeos::system::StatisticsProvider* provider,
-    const LocationUpdateRequester& location_update_requester,
     const VolumeInfoFetcher& volume_info_fetcher,
     const CPUStatisticsFetcher& cpu_statistics_fetcher,
     const CPUTempFetcher& cpu_temp_fetcher)
     : max_stored_past_activity_days_(kMaxStoredPastActivityDays),
       max_stored_future_activity_days_(kMaxStoredFutureActivityDays),
       local_state_(local_state),
       last_idle_check_(Time()),
       volume_info_fetcher_(volume_info_fetcher),
       cpu_statistics_fetcher_(cpu_statistics_fetcher),
       cpu_temp_fetcher_(cpu_temp_fetcher),
       statistics_provider_(provider),
       cros_settings_(chromeos::CrosSettings::Get()),
-      location_update_requester_(location_update_requester),
+      task_runner_(nullptr),
       weak_factory_(this) {
-  CHECK(content::BrowserThread::GetCurrentThreadIdentifier(&creation_thread_));
+  // Get the task runner of the current thread, so we can queue status responses
+  // on this thread.
+  CHECK(base::SequencedTaskRunnerHandle::IsSet());
+  task_runner_ = base::SequencedTaskRunnerHandle::Get();
 
   if (volume_info_fetcher_.is_null())
     volume_info_fetcher_ = base::Bind(&GetVolumeInfo);
 
   if (cpu_statistics_fetcher_.is_null())
     cpu_statistics_fetcher_ = base::Bind(&ReadCPUStatistics);
 
   if (cpu_temp_fetcher_.is_null())
     cpu_temp_fetcher_ = base::Bind(&ReadCPUTempInfo);
 
   idle_poll_timer_.Start(FROM_HERE,
                          TimeDelta::FromSeconds(kIdlePollIntervalSeconds),
                          this, &DeviceStatusCollector::CheckIdleState);
-  hardware_status_sampling_timer_.Start(
-      FROM_HERE,
-      TimeDelta::FromSeconds(kHardwareStatusSampleIntervalSeconds),
-      this, &DeviceStatusCollector::SampleHardwareStatus);
+  resource_usage_sampling_timer_.Start(
+      FROM_HERE, TimeDelta::FromSeconds(kResourceUsageSampleIntervalSeconds),
+      this, &DeviceStatusCollector::SampleResourceUsage);
 
   // Watch for changes to the individual policies that control what the status
   // reports contain.
   base::Closure callback =
       base::Bind(&DeviceStatusCollector::UpdateReportingSettings,
                  base::Unretained(this));
   version_info_subscription_ = cros_settings_->AddSettingsObserver(
       chromeos::kReportDeviceVersionInfo, callback);
   activity_times_subscription_ = cros_settings_->AddSettingsObserver(
       chromeos::kReportDeviceActivityTimes, callback);
   boot_mode_subscription_ = cros_settings_->AddSettingsObserver(
       chromeos::kReportDeviceBootMode, callback);
-  location_subscription_ = cros_settings_->AddSettingsObserver(
-      chromeos::kReportDeviceLocation, callback);
   network_interfaces_subscription_ = cros_settings_->AddSettingsObserver(
       chromeos::kReportDeviceNetworkInterfaces, callback);
   users_subscription_ = cros_settings_->AddSettingsObserver(
       chromeos::kReportDeviceUsers, callback);
   hardware_status_subscription_ = cros_settings_->AddSettingsObserver(
       chromeos::kReportDeviceHardwareStatus, callback);
   session_status_subscription_ = cros_settings_->AddSettingsObserver(
       chromeos::kReportDeviceSessionStatus, callback);
   os_update_status_subscription_ = cros_settings_->AddSettingsObserver(
       chromeos::kReportOsUpdateStatus, callback);
   running_kiosk_app_subscription_ = cros_settings_->AddSettingsObserver(
       chromeos::kReportRunningKioskApp, callback);
 
-  // The last known location is persisted in local state. This makes location
-  // information available immediately upon startup and avoids the need to
-  // reacquire the location on every user session change or browser crash.
-  device::Geoposition position;
-  std::string timestamp_str;
-  int64_t timestamp;
-  const base::DictionaryValue* location =
-      local_state_->GetDictionary(prefs::kDeviceLocation);
-  if (location->GetDouble(kLatitude, &position.latitude) &&
-      location->GetDouble(kLongitude, &position.longitude) &&
-      location->GetDouble(kAltitude, &position.altitude) &&
-      location->GetDouble(kAccuracy, &position.accuracy) &&
-      location->GetDouble(kAltitudeAccuracy, &position.altitude_accuracy) &&
-      location->GetDouble(kHeading, &position.heading) &&
-      location->GetDouble(kSpeed, &position.speed) &&
-      location->GetString(kTimestamp, &timestamp_str) &&
-      base::StringToInt64(timestamp_str, &timestamp)) {
-    position.timestamp = Time::FromInternalValue(timestamp);
-    position_ = position;
-  }
-
   // Fetch the current values of the policies.
   UpdateReportingSettings();
 
   // Get the the OS and firmware version info.
   base::PostTaskAndReplyWithResult(
       content::BrowserThread::GetBlockingPool(),
       FROM_HERE,
       base::Bind(&chromeos::version_loader::GetVersion,
                  chromeos::version_loader::VERSION_FULL),
       base::Bind(&DeviceStatusCollector::OnOSVersion,
                  weak_factory_.GetWeakPtr()));
   base::PostTaskAndReplyWithResult(
       content::BrowserThread::GetBlockingPool(),
       FROM_HERE,
       base::Bind(&chromeos::version_loader::GetFirmware),
       base::Bind(&DeviceStatusCollector::OnOSFirmware,
                  weak_factory_.GetWeakPtr()));
 }
 
 DeviceStatusCollector::~DeviceStatusCollector() {
 }
 
 // static
 void DeviceStatusCollector::RegisterPrefs(PrefRegistrySimple* registry) {
   registry->RegisterDictionaryPref(prefs::kDeviceActivityTimes,
                                    new base::DictionaryValue);
-  registry->RegisterDictionaryPref(prefs::kDeviceLocation,
-                                   new base::DictionaryValue);
 }
 
 void DeviceStatusCollector::CheckIdleState() {
   CalculateIdleState(kIdleStateThresholdSeconds,
       base::Bind(&DeviceStatusCollector::IdleStateCallback,
                  base::Unretained(this)));
 }
 
 void DeviceStatusCollector::UpdateReportingSettings() {
   // Attempt to fetch the current value of the reporting settings.
@@ skipping 33 matching lines @@
   if (!cros_settings_->GetBoolean(
           chromeos::kReportDeviceHardwareStatus, &report_hardware_status_)) {
     report_hardware_status_ = true;
   }
 
   if (!cros_settings_->GetBoolean(
           chromeos::kReportDeviceSessionStatus, &report_session_status_)) {
     report_session_status_ = true;
   }
 
-  // Device location reporting is disabled by default because it is
-  // not launched yet.
-  if (!cros_settings_->GetBoolean(
-      chromeos::kReportDeviceLocation, &report_location_)) {
-    report_location_ = false;
-  }
-
-  if (report_location_) {
-    ScheduleGeolocationUpdateRequest();
-  } else {
-    geolocation_update_timer_.Stop();
-    position_ = device::Geoposition();
-    local_state_->ClearPref(prefs::kDeviceLocation);
-  }
-
   if (!report_hardware_status_) {
-    ClearCachedHardwareStatus();
+    ClearCachedResourceUsage();
   } else if (!already_reporting_hardware_status) {
     // Turning on hardware status reporting - fetch an initial sample
     // immediately instead of waiting for the sampling timer to fire.
-    SampleHardwareStatus();
+    SampleResourceUsage();
   }
 
   // Os update status and running kiosk app reporting are disabled by default.
   if (!cros_settings_->GetBoolean(chromeos::kReportOsUpdateStatus,
                                   &report_os_update_status_)) {
     report_os_update_status_ = false;
   }
   if (!cros_settings_->GetBoolean(chromeos::kReportRunningKioskApp,
                                   &report_running_kiosk_app_)) {
     report_running_kiosk_app_ = false;
@@ skipping 57 matching lines @@
     midnight += TimeDelta::FromDays(1);
     int64_t activity = (std::min(end, midnight) - start).InMilliseconds();
     std::string day_key = base::Int64ToString(TimestampToDayKey(start));
     int previous_activity = 0;
     activity_times->GetInteger(day_key, &previous_activity);
     activity_times->SetInteger(day_key, previous_activity + activity);
     start = midnight;
   }
 }
 
-void DeviceStatusCollector::ClearCachedHardwareStatus() {
-  volume_info_.clear();
+void DeviceStatusCollector::ClearCachedResourceUsage() {
   resource_usage_.clear();
   last_cpu_active_ = 0;
   last_cpu_idle_ = 0;
 }
 
 void DeviceStatusCollector::IdleStateCallback(ui::IdleState state) {
   // Do nothing if device activity reporting is disabled.
   if (!report_activity_times_)
     return;
 
@@ skipping 26 matching lines @@
     if (chromeos::KioskAppManager::Get()->GetApp(account->kiosk_app_id,
                                                  &current_app) &&
         current_app.was_auto_launched_with_zero_delay) {
       return account;
     }
   }
   // No auto-launched kiosk session active.
   return std::unique_ptr<DeviceLocalAccount>();
 }
 
-void DeviceStatusCollector::SampleHardwareStatus() {
+void DeviceStatusCollector::SampleResourceUsage() {
   // Results must be written in the creation thread since that's where they
   // are read from in the Get*StatusAsync methods.
-  CHECK(content::BrowserThread::CurrentlyOn(creation_thread_));
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   // If hardware reporting has been disabled, do nothing here.
   if (!report_hardware_status_)
     return;
 
-  // Create list of mounted disk volumes to query status.
-  std::vector<storage::MountPoints::MountPointInfo> external_mount_points;
-  storage::ExternalMountPoints::GetSystemInstance()->AddMountPointInfosTo(
-      &external_mount_points);
-
-  std::vector<std::string> mount_points;
-  for (const auto& info : external_mount_points)
-    mount_points.push_back(info.path.value());
-
-  for (const auto& mount_info :
-           chromeos::disks::DiskMountManager::GetInstance()->mount_points()) {
-    // Extract a list of mount points to populate.
-    mount_points.push_back(mount_info.first);
-  }
-
-  // Call out to the blocking pool to measure disk, CPU usage and CPU temp.
-  base::PostTaskAndReplyWithResult(
-      content::BrowserThread::GetBlockingPool(),
-      FROM_HERE,
-      base::Bind(volume_info_fetcher_, mount_points),
-      base::Bind(&DeviceStatusCollector::ReceiveVolumeInfo,
-                 weak_factory_.GetWeakPtr()));
-
+  // Call out to the blocking pool to sample CPU stats.
   base::PostTaskAndReplyWithResult(
       content::BrowserThread::GetBlockingPool(), FROM_HERE,
       cpu_statistics_fetcher_,
       base::Bind(&DeviceStatusCollector::ReceiveCPUStatistics,
                  weak_factory_.GetWeakPtr()));
-
-  base::PostTaskAndReplyWithResult(
-      content::BrowserThread::GetBlockingPool(), FROM_HERE, cpu_temp_fetcher_,
-      base::Bind(&DeviceStatusCollector::StoreCPUTempInfo,
-                 weak_factory_.GetWeakPtr()));
 }
 
 void DeviceStatusCollector::ReceiveCPUStatistics(const std::string& stats) {
   int cpu_usage_percent = 0;
   if (stats.empty()) {
     DLOG(WARNING) << "Unable to read CPU statistics";
   } else {
     // Parse the data from /proc/stat, whose format is defined at
     // https://www.kernel.org/doc/Documentation/filesystems/proc.txt.
     //
@@ skipping 35 matching lines @@
                          base::SysInfo::AmountOfAvailablePhysicalMemory()};
 
   resource_usage_.push_back(usage);
 
   // If our cache of samples is full, throw out old samples to make room for new
   // sample.
   if (resource_usage_.size() > kMaxResourceUsageSamples)
     resource_usage_.pop_front();
 }
 
-void DeviceStatusCollector::StoreCPUTempInfo(
-    const std::vector<em::CPUTempInfo>& info) {
-  if (info.empty()) {
-    DLOG(WARNING) << "Unable to read CPU temp information.";
-  }
-
-  if (report_hardware_status_)
-    cpu_temp_info_ = info;
-}
-
 bool DeviceStatusCollector::GetActivityTimes(
-    em::DeviceStatusReportRequest* request) {
+    em::DeviceStatusReportRequest* status) {
   DictionaryPrefUpdate update(local_state_, prefs::kDeviceActivityTimes);
   base::DictionaryValue* activity_times = update.Get();
 
   bool anything_reported = false;
   for (base::DictionaryValue::Iterator it(*activity_times); !it.IsAtEnd();
        it.Advance()) {
     int64_t start_timestamp;
     int activity_milliseconds;
     if (base::StringToInt64(it.key(), &start_timestamp) &&
         it.value().GetAsInteger(&activity_milliseconds)) {
       // This is correct even when there are leap seconds, because when a leap
       // second occurs, two consecutive seconds have the same timestamp.
       int64_t end_timestamp = start_timestamp + Time::kMillisecondsPerDay;
 
-      em::ActiveTimePeriod* active_period = request->add_active_period();
+      em::ActiveTimePeriod* active_period = status->add_active_period();
       em::TimePeriod* period = active_period->mutable_time_period();
       period->set_start_timestamp(start_timestamp);
       period->set_end_timestamp(end_timestamp);
       active_period->set_active_duration(activity_milliseconds);
       if (start_timestamp >= last_reported_day_) {
         last_reported_day_ = start_timestamp;
         duration_for_last_reported_day_ = activity_milliseconds;
       }
       anything_reported = true;
     } else {
       NOTREACHED();
     }
   }
   return anything_reported;
 }
 
 bool DeviceStatusCollector::GetVersionInfo(
-    em::DeviceStatusReportRequest* request) {
-  request->set_browser_version(version_info::GetVersionNumber());
-  request->set_os_version(os_version_);
-  request->set_firmware_version(firmware_version_);
+    em::DeviceStatusReportRequest* status) {
+  status->set_browser_version(version_info::GetVersionNumber());
+  status->set_os_version(os_version_);
+  status->set_firmware_version(firmware_version_);
   return true;
 }
 
-bool DeviceStatusCollector::GetBootMode(
-    em::DeviceStatusReportRequest* request) {
+bool DeviceStatusCollector::GetBootMode(em::DeviceStatusReportRequest* status) {
   std::string dev_switch_mode;
   bool anything_reported = false;
   if (statistics_provider_->GetMachineStatistic(
           chromeos::system::kDevSwitchBootKey, &dev_switch_mode)) {
     if (dev_switch_mode == chromeos::system::kDevSwitchBootValueDev)
-      request->set_boot_mode("Dev");
+      status->set_boot_mode("Dev");

[Andrew T Wilson (Slow), 2016/09/26 16:33:42]

This is all fine, although note that your patch is significantly larger because
you decided to rename this variable. I might've split these into two patches
(one for a mechanical renaming, one for logic changes).

[ljusten (tachyonic), 2016/09/27 16:53:55]

Acknowledged.

     else if (dev_switch_mode == chromeos::system::kDevSwitchBootValueVerified)
-      request->set_boot_mode("Verified");
+      status->set_boot_mode("Verified");
     anything_reported = true;
   }
   return anything_reported;
 }
 
-bool DeviceStatusCollector::GetLocation(
-    em::DeviceStatusReportRequest* request) {
-  em::DeviceLocation* location = request->mutable_device_location();
-  if (!position_.Validate()) {
-    location->set_error_code(
-        em::DeviceLocation::ERROR_CODE_POSITION_UNAVAILABLE);
-    location->set_error_message(position_.error_message);
-  } else {
-    location->set_latitude(position_.latitude);
-    location->set_longitude(position_.longitude);
-    location->set_accuracy(position_.accuracy);
-    location->set_timestamp(
-        (position_.timestamp - Time::UnixEpoch()).InMilliseconds());
-    // Lowest point on land is at approximately -400 meters.
-    if (position_.altitude > -10000.)
-      location->set_altitude(position_.altitude);
-    if (position_.altitude_accuracy >= 0.)
-      location->set_altitude_accuracy(position_.altitude_accuracy);
-    if (position_.heading >= 0. && position_.heading <= 360)
-      location->set_heading(position_.heading);
-    if (position_.speed >= 0.)
-      location->set_speed(position_.speed);
-    location->set_error_code(em::DeviceLocation::ERROR_CODE_NONE);
-  }
-  return true;
-}
-
 bool DeviceStatusCollector::GetNetworkInterfaces(
-    em::DeviceStatusReportRequest* request) {
+    em::DeviceStatusReportRequest* status) {
   // Maps shill device type strings to proto enum constants.
   static const struct {
     const char* type_string;
     em::NetworkInterface::NetworkDeviceType type_constant;
   } kDeviceTypeMap[] = {
     { shill::kTypeEthernet,  em::NetworkInterface::TYPE_ETHERNET,  },
     { shill::kTypeWifi,      em::NetworkInterface::TYPE_WIFI,      },
     { shill::kTypeWimax,     em::NetworkInterface::TYPE_WIMAX,     },
     { shill::kTypeBluetooth, em::NetworkInterface::TYPE_BLUETOOTH, },
     { shill::kTypeCellular,  em::NetworkInterface::TYPE_CELLULAR,  },
@@ skipping 31 matching lines @@
     for (; type_idx < arraysize(kDeviceTypeMap); ++type_idx) {
       if ((*device)->type() == kDeviceTypeMap[type_idx].type_string)
         break;
     }
 
     // If the type isn't in |kDeviceTypeMap|, the interface is not relevant for
     // reporting. This filters out VPN devices.
     if (type_idx >= arraysize(kDeviceTypeMap))
       continue;
 
-    em::NetworkInterface* interface = request->add_network_interface();
+    em::NetworkInterface* interface = status->add_network_interface();
     interface->set_type(kDeviceTypeMap[type_idx].type_constant);
     if (!(*device)->mac_address().empty())
       interface->set_mac_address((*device)->mac_address());
     if (!(*device)->meid().empty())
       interface->set_meid((*device)->meid());
     if (!(*device)->imei().empty())
       interface->set_imei((*device)->imei());
     if (!(*device)->path().empty())
       interface->set_device_path((*device)->path());
     anything_reported = true;
@@ skipping 19 matching lines @@
         em::NetworkState::UNKNOWN;
     const std::string connection_state_string(state->connection_state());
     for (size_t i = 0; i < arraysize(kConnectionStateMap); ++i) {
       if (connection_state_string == kConnectionStateMap[i].state_string) {
         connection_state_enum = kConnectionStateMap[i].state_constant;
         break;
       }
     }
 
     // Copy fields from NetworkState into the status report.
-    em::NetworkState* proto_state = request->add_network_state();
+    em::NetworkState* proto_state = status->add_network_state();
     proto_state->set_connection_state(connection_state_enum);
     anything_reported = true;
 
     // Report signal strength for wifi connections.
     if (state->type() == shill::kTypeWifi) {
       // If shill has provided a signal strength, convert it to dBm and store it
       // in the status report. A signal_strength() of 0 connotes "no signal"
       // rather than "really weak signal", so we only report signal strength if
       // it is non-zero.
       if (state->signal_strength()) {
         proto_state->set_signal_strength(
             ConvertWifiSignalStrength(state->signal_strength()));
       }
     }
 
     if (!state->device_path().empty())
       proto_state->set_device_path(state->device_path());
 
     if (!state->ip_address().empty())
       proto_state->set_ip_address(state->ip_address());
 
     if (!state->gateway().empty())
       proto_state->set_gateway(state->gateway());
   }
   return anything_reported;
 }
 
-bool DeviceStatusCollector::GetUsers(em::DeviceStatusReportRequest* request) {
+bool DeviceStatusCollector::GetUsers(em::DeviceStatusReportRequest* status) {
   const user_manager::UserList& users =
       chromeos::ChromeUserManager::Get()->GetUsers();
 
   bool anything_reported = false;
   for (auto* user : users) {
     // Only users with gaia accounts (regular) are reported.
     if (!user->HasGaiaAccount())
       continue;
 
-    em::DeviceUser* device_user = request->add_user();
+    em::DeviceUser* device_user = status->add_user();
     if (chromeos::ChromeUserManager::Get()->ShouldReportUser(user->email())) {
       device_user->set_type(em::DeviceUser::USER_TYPE_MANAGED);
       device_user->set_email(user->email());
     } else {
       device_user->set_type(em::DeviceUser::USER_TYPE_UNMANAGED);
       // Do not report the email address of unmanaged users.
     }
     anything_reported = true;
   }
   return anything_reported;
 }
 
 bool DeviceStatusCollector::GetHardwareStatus(
-    em::DeviceStatusReportRequest* status) {
-  // Add volume info.
-  status->clear_volume_info();
-  for (const em::VolumeInfo& info : volume_info_) {
-    *status->add_volume_info() = info;
-  }
+    em::DeviceStatusReportRequest* status,
+    scoped_refptr<GetStatusState> state) {
+  // Sample disk volume info in a background thread.
+  state->SampleVolumeInfo(volume_info_fetcher_);
 
+  // Sample CPU temperature in a background thread.
+  state->SampleCPUTempInfo(cpu_temp_fetcher_);
+
+  // Add CPU utilization and free RAM. Note that these stats are sampled in
+  // regular intervals. Unlike CPU temp and volume info these are not one-time
+  // sampled values, hence the difference in logic.
   status->set_system_ram_total(base::SysInfo::AmountOfPhysicalMemory());
   status->clear_system_ram_free();
   status->clear_cpu_utilization_pct();
   for (const ResourceUsage& usage : resource_usage_) {
     status->add_cpu_utilization_pct(usage.cpu_usage_percent);
     status->add_system_ram_free(usage.bytes_of_ram_free);
   }
-
-  // Add CPU temp info.
-  status->clear_cpu_temp_info();
-  for (const em::CPUTempInfo& info : cpu_temp_info_) {
-    *status->add_cpu_temp_info() = info;
-  }
   return true;
 }
 
 bool DeviceStatusCollector::GetOsUpdateStatus(
     em::DeviceStatusReportRequest* status) {
   const base::Version platform_version(GetPlatformVersion());
   if (!platform_version.IsValid())
     return false;
 
   const std::string required_platform_version_string =
@@ skipping 41 matching lines @@
         em::OsUpdateStatus::OS_IMAGE_DOWNLOAD_NOT_STARTED);
   }
 
   return true;
 }
 
 bool DeviceStatusCollector::GetRunningKioskApp(
     em::DeviceStatusReportRequest* status) {
   // Must be on creation thread since some stats are written to in that thread
   // and accessing them from another thread would lead to race conditions.
-  CHECK(content::BrowserThread::CurrentlyOn(creation_thread_));
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   std::unique_ptr<const DeviceLocalAccount> account =
       GetAutoLaunchedKioskSessionInfo();
   // Only generate running kiosk app reports if we are in an auto-launched kiosk
   // session.
   if (!account)
     return false;
 
   em::AppStatus* running_kiosk_app = status->mutable_running_kiosk_app();
   running_kiosk_app->set_app_id(account->kiosk_app_id);
 
   const std::string app_version = GetAppVersion(account->kiosk_app_id);
   if (app_version.empty()) {
     DLOG(ERROR) << "Unable to get version for extension: "
                 << account->kiosk_app_id;
   } else {
     running_kiosk_app->set_extension_version(app_version);
   }
 
   chromeos::KioskAppManager::App app_info;
   if (chromeos::KioskAppManager::Get()->GetApp(account->kiosk_app_id,
                                                &app_info)) {
     running_kiosk_app->set_required_platform_version(
         app_info.required_platform_version);
   }
   return true;
 }
 
-void DeviceStatusCollector::GetDeviceStatusAsync(
-    const DeviceStatusCallback& response) {
+void DeviceStatusCollector::GetDeviceAndSessionStatusAsync(
+    const StatusCallback& response) {
   // Must be on creation thread since some stats are written to in that thread
   // and accessing them from another thread would lead to race conditions.
-  CHECK(content::BrowserThread::CurrentlyOn(creation_thread_));
+  DCHECK(thread_checker_.CalledOnValidThread());
 
-  std::unique_ptr<em::DeviceStatusReportRequest> status =
-      base::MakeUnique<em::DeviceStatusReportRequest>();
-  bool got_status = false;
+  // Some of the data we're collecting is gathered in background threads.
+  // This object keeps track of the state of each async request.
+  scoped_refptr<GetStatusState> state(
+      new GetStatusState(task_runner_, response));
+
+  // Gather device status (might queue some async queries)
+  GetDeviceStatus(state);
+
+  // Gather device status (might queue some async queries)
+  GetSessionStatus(state);
+
+  // If there are no outstanding async queries, e.g. from GetHardwareStatus(),
+  // the destructor of |state| calls |response|. If there are async queries, the
+  // queries hold references to |state|, so that |state| is only destroyed when
+  // the last async query has finished.
+}
+
+void DeviceStatusCollector::GetDeviceStatus(
+    scoped_refptr<GetStatusState> state) {
+  em::DeviceStatusReportRequest* status = state->device_status();
+  bool anything_reported = false;
 
   if (report_activity_times_)
-    got_status |= GetActivityTimes(status.get());
+    anything_reported |= GetActivityTimes(status);
 
   if (report_version_info_)
-    got_status |= GetVersionInfo(status.get());
+    anything_reported |= GetVersionInfo(status);
 
   if (report_boot_mode_)
-    got_status |= GetBootMode(status.get());
-
-  if (report_location_)
-    got_status |= GetLocation(status.get());
+    anything_reported |= GetBootMode(status);
 
   if (report_network_interfaces_)
-    got_status |= GetNetworkInterfaces(status.get());
+    anything_reported |= GetNetworkInterfaces(status);
 
   if (report_users_)
-    got_status |= GetUsers(status.get());
+    anything_reported |= GetUsers(status);
 
   if (report_hardware_status_)
-    got_status |= GetHardwareStatus(status.get());
+    anything_reported |= GetHardwareStatus(status, state);
 
   if (report_os_update_status_)
-    got_status |= GetOsUpdateStatus(status.get());
+    anything_reported |= GetOsUpdateStatus(status);
 
   if (report_running_kiosk_app_)
-    got_status |= GetRunningKioskApp(status.get());
+    anything_reported |= GetRunningKioskApp(status);
 
   // Wipe pointer if we didn't actually add any data.
-  if (!got_status)
-    status.reset();
-
-  content::BrowserThread::PostTask(creation_thread_, FROM_HERE,
-                                   base::Bind(response, base::Passed(&status)));
+  if (!anything_reported)
+    state->ResetDeviceStatus();
 }
 
-void DeviceStatusCollector::GetDeviceSessionStatusAsync(
-    const DeviceSessionStatusCallback& response) {
-  // Only generate session status reports if session status reporting is
-  // enabled.
-  if (!report_session_status_) {
-    content::BrowserThread::PostTask(creation_thread_, FROM_HERE,
-                                     base::Bind(response, nullptr));
-    return;
-  }
+void DeviceStatusCollector::GetSessionStatus(
+    scoped_refptr<GetStatusState> state) {
+  em::SessionStatusReportRequest* status = state->session_status();
+  bool anything_reported = false;
 
+  if (report_session_status_)
+    anything_reported |= GetAccountStatus(status);
+
+  // Wipe pointer if we didn't actually add any data.
+  if (!anything_reported)
+    state->ResetSessionStatus();
+}
+
+bool DeviceStatusCollector::GetAccountStatus(
+    em::SessionStatusReportRequest* status) {
   std::unique_ptr<const DeviceLocalAccount> account =
       GetAutoLaunchedKioskSessionInfo();
-  // Only generate session status reports if we are in an auto-launched kiosk
-  // session.
-  if (!account) {
-    content::BrowserThread::PostTask(creation_thread_, FROM_HERE,
-                                     base::Bind(response, nullptr));
-    return;
-  }
-
-  std::unique_ptr<em::SessionStatusReportRequest> status =
-      base::MakeUnique<em::SessionStatusReportRequest>();
+  if (!account)
+    return false;
 
   // Get the account ID associated with this user.
   status->set_device_local_account_id(account->account_id);
   em::AppStatus* app_status = status->add_installed_apps();
   app_status->set_app_id(account->kiosk_app_id);
 
   // Look up the app and get the version.
   const std::string app_version = GetAppVersion(account->kiosk_app_id);
   if (app_version.empty()) {
     DLOG(ERROR) << "Unable to get version for extension: "
                 << account->kiosk_app_id;
   } else {
     app_status->set_extension_version(app_version);
   }
 
-  content::BrowserThread::PostTask(creation_thread_, FROM_HERE,
-                                   base::Bind(response, base::Passed(&status)));
+  return true;
 }
 
 std::string DeviceStatusCollector::GetAppVersion(
     const std::string& kiosk_app_id) {
   Profile* const profile =
       chromeos::ProfileHelper::Get()->GetProfileByUser(
           user_manager::UserManager::Get()->GetActiveUser());
   const extensions::ExtensionRegistry* const registry =
       extensions::ExtensionRegistry::Get(profile);
   const extensions::Extension* const extension = registry->GetExtensionById(
       kiosk_app_id, extensions::ExtensionRegistry::EVERYTHING);
   if (!extension)
     return std::string();
   return extension->VersionString();
 }
 
 void DeviceStatusCollector::OnSubmittedSuccessfully() {
   TrimStoredActivityPeriods(last_reported_day_, duration_for_last_reported_day_,
                             std::numeric_limits<int64_t>::max());
 }
 
 void DeviceStatusCollector::OnOSVersion(const std::string& version) {
   os_version_ = version;
 }
 
 void DeviceStatusCollector::OnOSFirmware(const std::string& version) {
   firmware_version_ = version;
 }
 
-void DeviceStatusCollector::ScheduleGeolocationUpdateRequest() {
-  if (geolocation_update_timer_.IsRunning() || geolocation_update_in_progress_)
-    return;
-
-  if (position_.Validate()) {
-    TimeDelta elapsed = GetCurrentTime() - position_.timestamp;
-    TimeDelta interval =
-        TimeDelta::FromSeconds(kGeolocationPollIntervalSeconds);
-    if (elapsed <= interval) {
-      geolocation_update_timer_.Start(
-          FROM_HERE,
-          interval - elapsed,
-          this,
-          &DeviceStatusCollector::ScheduleGeolocationUpdateRequest);
-      return;
-    }
-  }
-
-  geolocation_update_in_progress_ = true;
-  if (location_update_requester_.is_null()) {
-    geolocation_subscription_ =
-        device::GeolocationProvider::GetInstance()->AddLocationUpdateCallback(
-            base::Bind(&DeviceStatusCollector::ReceiveGeolocationUpdate,
-                       weak_factory_.GetWeakPtr()),
-            true);
-  } else {
-    location_update_requester_.Run(base::Bind(
-        &DeviceStatusCollector::ReceiveGeolocationUpdate,
-        weak_factory_.GetWeakPtr()));
-  }
-}
-
-void DeviceStatusCollector::ReceiveGeolocationUpdate(
-    const device::Geoposition& position) {
-  geolocation_update_in_progress_ = false;
-
-  // Ignore update if device location reporting has since been disabled.
-  if (!report_location_)
-    return;
-
-  if (position.Validate()) {
-    position_ = position;
-    base::DictionaryValue location;
-    location.SetDouble(kLatitude, position.latitude);
-    location.SetDouble(kLongitude, position.longitude);
-    location.SetDouble(kAltitude, position.altitude);
-    location.SetDouble(kAccuracy, position.accuracy);
-    location.SetDouble(kAltitudeAccuracy, position.altitude_accuracy);
-    location.SetDouble(kHeading, position.heading);
-    location.SetDouble(kSpeed, position.speed);
-    location.SetString(kTimestamp,
-        base::Int64ToString(position.timestamp.ToInternalValue()));
-    local_state_->Set(prefs::kDeviceLocation, location);
-  }
-
-  ScheduleGeolocationUpdateRequest();
-}
-
-void DeviceStatusCollector::ReceiveVolumeInfo(
-    const std::vector<em::VolumeInfo>& info) {
-  if (report_hardware_status_)
-    volume_info_ = info;
-}
-
 }  // namespace policy