Move statistics_provider to chromeos/system

chromeos/system/statistics_provider.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/system/statistics_provider.h"
+#include "chromeos/system/statistics_provider.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
+#include "base/location.h"
 #include "base/logging.h"
 #include "base/memory/singleton.h"
 #include "base/path_service.h"
+#include "base/synchronization/cancellation_flag.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/sys_info.h"
+#include "base/task_runner.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
 #include "chromeos/app_mode/kiosk_oem_manifest_parser.h"
 #include "chromeos/chromeos_constants.h"
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/system/name_value_pairs_parser.h"
-#include "content/public/browser/browser_thread.h"
-
-using content::BrowserThread;
 
 namespace chromeos {
 namespace system {
+
 namespace {
 
 // Path to the tool used to get system info, and delimiters for the output
 // format of the tool.
 const char* kCrosSystemTool[] = { "/usr/bin/crossystem" };
 const char kCrosSystemEq[] = "=";
 const char kCrosSystemDelim[] = "\n";
 const char kCrosSystemCommentDelim[] = "#";
 const char kCrosSystemUnknownValue[] = "(error)";
 
 const char kHardwareClassCrosSystemKey[] = "hwid";
-const char kHardwareClassKey[] = "hardware_class";
 const char kUnknownHardwareClass[] = "unknown";
 
 // File to get machine hardware info from, and key/value delimiters of
 // the file.
 // /tmp/machine-info is generated by platform/init/chromeos_startup.
 const char kMachineHardwareInfoFile[] = "/tmp/machine-info";
 const char kMachineHardwareInfoEq[] = "=";
 const char kMachineHardwareInfoDelim[] = " \n";
 
 // File to get ECHO coupon info from, and key/value delimiters of
@@ skipping 11 matching lines @@
 const int kTimeoutSecs = 3;
 
 // The location of OEM manifest file used to trigger OOBE flow for kiosk mode.
 const CommandLine::CharType kOemManifestFilePath[] =
     FILE_PATH_LITERAL("/usr/share/oem/oobe/manifest.json");
 
 }  // namespace
 
 // Key values for GetMachineStatistic()/GetMachineFlag() calls.
 const char kDevSwitchBootMode[] = "devsw_boot";
-const char kHardwareClass[] = "hardware_class";
+const char kHardwareClassKey[] = "hardware_class";
 const char kOffersCouponCodeKey[] = "ubind_attribute";
 const char kOffersGroupCodeKey[] = "gbind_attribute";
-const char kOemCanExitEnterpriseEnrollmentKey[] =
-    "oem_can_exit_enrollment";
-const char kOemDeviceRequisitionKey[] =
-    "oem_device_requisition";
-const char kOemIsEnterpriseManagedKey[] =
-    "oem_enterprise_managed";
-const char kOemKeyboardDrivenOobeKey[] =
-    "oem_keyboard_driven_oobe";
+const char kOemCanExitEnterpriseEnrollmentKey[] = "oem_can_exit_enrollment";
+const char kOemDeviceRequisitionKey[] = "oem_device_requisition";
+const char kOemIsEnterpriseManagedKey[] = "oem_enterprise_managed";
+const char kOemKeyboardDrivenOobeKey[] = "oem_keyboard_driven_oobe";
 
 // The StatisticsProvider implementation used in production.
 class StatisticsProviderImpl : public StatisticsProvider {
  public:
   // StatisticsProvider implementation:
-  virtual void Init() OVERRIDE;
-  virtual void StartLoadingMachineStatistics() OVERRIDE;
+  virtual void StartLoadingMachineStatistics(
+      const scoped_refptr<base::TaskRunner>& file_task_runner,
+      bool load_oem_manifest) OVERRIDE;
   virtual bool GetMachineStatistic(const std::string& name,
                                    std::string* result) OVERRIDE;
-  virtual bool GetMachineFlag(const std::string& name,
-                              bool* result) OVERRIDE;
-  virtual void LoadOemManifest() OVERRIDE;
+  virtual bool GetMachineFlag(const std::string& name, bool* result) OVERRIDE;
+  virtual void Shutdown() OVERRIDE;
 
   static StatisticsProviderImpl* GetInstance();
 
  protected:
-  StatisticsProviderImpl();
-  void LoadOemManifestFromFile(const base::FilePath& file);
-
- private:
   typedef std::map<std::string, bool> MachineFlags;
   friend struct DefaultSingletonTraits<StatisticsProviderImpl>;
 
-  // Loads the machine statistcs by examining the system.
-  void LoadMachineStatistics();
+  StatisticsProviderImpl();
+  virtual ~StatisticsProviderImpl();
 
-  bool initialized_;
+  // Waits up to |kTimeoutSecs| for statistics to be loaded. Returns true if
+  // they were loaded successfully.
+  bool WaitForStatisticsLoaded();
+
+  // Loads the machine statistics off of disk. Runs on the file thread.
+  void LoadMachineStatistics(bool load_oem_manifest);
+
+  // Loads the OEM statistics off of disk. Runs on the file thread.
+  void LoadOemManifestFromFile(const base::FilePath& file);
+
   bool load_statistics_started_;
   NameValuePairsParser::NameValueMap machine_info_;
   MachineFlags machine_flags_;
+  base::CancellationFlag cancellation_flag_;
+  // |on_statistics_loaded_| protects |machine_info_| and |machine_flags_|.
   base::WaitableEvent on_statistics_loaded_;
 
+ private:
   DISALLOW_COPY_AND_ASSIGN(StatisticsProviderImpl);
 };
 
-void StatisticsProviderImpl::Init() {
-  DCHECK(!initialized_);
-  initialized_ = true;
+bool StatisticsProviderImpl::WaitForStatisticsLoaded() {
+  CHECK(load_statistics_started_);
+  if (on_statistics_loaded_.IsSignaled())
+    return true;
+
+  // Block if the statistics are not loaded yet. Normally this shouldn't
+  // happen excpet during OOBE.
+  base::Time start_time = base::Time::Now();
+  base::ThreadRestrictions::ScopedAllowWait allow_wait;
+  on_statistics_loaded_.TimedWait(base::TimeDelta::FromSeconds(kTimeoutSecs));
+
+  base::TimeDelta dtime = base::Time::Now() - start_time;

[satorux1, 2013/10/07 05:01:54]

nit: dtime -> elapsed?

[stevenjb, 2013/10/07 18:07:30]

We use 'dtime' (delta time) a lot...

+  if (on_statistics_loaded_.IsSignaled()) {
+    LOG(ERROR) << "Statistics loaded after waiting "
+               << dtime.InMilliseconds() << "ms. ";
+    return true;
+  }
+
+  LOG(ERROR) << "Statistics not loaded after waiting "
+             << dtime.InMilliseconds() << "ms. ";
+  return false;
 }
 
 bool StatisticsProviderImpl::GetMachineStatistic(
     const std::string& name, std::string* result) {
-  CHECK(initialized_);
-  CHECK(load_statistics_started_)
-      << "GetMachineStatistic called before load started: " << name;
-
-  VLOG(1) << "Statistic is requested for " << name;
-  // Block if the statistics are not loaded yet. Per LOG(WARNING) below,
-  // the statistics are loaded before requested as of now. For regular
-  // sessions (i.e. not OOBE), statistics are first requested when the
-  // user is logging in so we have plenty of time to load the data
-  // beforehand.
-  //
-  // If you see the warning appeared for regular sessions, it probably
-  // means that there is new client code that uses the statistics in the
-  // very early stage of the browser startup. The statistic name should be
-  // helpful to identify the caller.
-  if (!on_statistics_loaded_.IsSignaled()) {
-    // http://crbug.com/125385
-    base::Time start_time = base::Time::Now();
-    base::ThreadRestrictions::ScopedAllowWait allow_wait;
-    on_statistics_loaded_.TimedWait(base::TimeDelta::FromSeconds(kTimeoutSecs));
-    base::TimeDelta dtime = base::Time::Now() - start_time;
-    if (!on_statistics_loaded_.IsSignaled()) {
-      LOG(ERROR) << "Statistics weren't loaded after waiting "
-                 << dtime.InMilliseconds() << "ms. "
-                 << "Requested statistic: " << name;
-      return false;
-    } else {
-      LOG(ERROR) << "Statistic loaded after waiting "
-                 << dtime.InMilliseconds() << "ms. "
-                 << "Requested statistic: " << name;
-    }
+  VLOG(1) << "Machine Statistic requested: " << name;
+  if (!WaitForStatisticsLoaded()) {
+    LOG(ERROR) << "GetMachineStatistic called before load started: " << name;

[satorux1, 2013/10/07 05:01:54]

maybe don't need this error? WaitForStatisticsLoaded() emits an error message.

[stevenjb, 2013/10/07 18:07:30]

This error and the one below distinguish between GetMachineStatistic and
GetMachineFlag, and specify the name of the statistic. The other error shows how
long we waited. Not perfect, but keeps the functions simpler I think.

+    return false;
   }
 
   NameValuePairsParser::NameValueMap::iterator iter = machine_info_.find(name);
-  if (iter != machine_info_.end()) {
-    *result = iter->second;
-    return true;
+  if (iter == machine_info_.end()) {
+    if (base::SysInfo::IsRunningOnChromeOS())
+      LOG(WARNING) << "Requested statistic not found: " << name;
+    return false;
   }
-  return false;
+  *result = iter->second;
+  return true;
 }
 
-bool StatisticsProviderImpl::GetMachineFlag(
-    const std::string& name, bool* result) {
-  MachineFlags::const_iterator iter = machine_flags_.find(name);
-  if (iter != machine_flags_.end()) {
-    *result = iter->second;
-    return true;
+bool StatisticsProviderImpl::GetMachineFlag(const std::string& name,
+                                            bool* result) {
+  VLOG(1) << "Machine Flag requested: " << name;
+  if (!WaitForStatisticsLoaded()) {
+    LOG(ERROR) << "GetMachineFlag called before load started: " << name;

[satorux1, 2013/10/07 05:01:54]

ditto.

[stevenjb, 2013/10/07 18:07:30]

See above.

+    return false;
   }
 
-  return false;
+  MachineFlags::const_iterator iter = machine_flags_.find(name);
+  if (iter == machine_flags_.end()) {
+    if (base::SysInfo::IsRunningOnChromeOS())
+      LOG(WARNING) << "Requested machine flag not found: " << name;
+    return false;
+  }
+  *result = iter->second;
+  return true;
 }
 
-void StatisticsProviderImpl::LoadOemManifest() {
-  LoadOemManifestFromFile(base::FilePath(kOemManifestFilePath));
+void StatisticsProviderImpl::Shutdown() {
+  cancellation_flag_.Set();  // Cancel any pending loads
 }
 
-// manual_reset needs to be true, as we want to keep the signaled state.
 StatisticsProviderImpl::StatisticsProviderImpl()
-    : initialized_(false),
-      load_statistics_started_(false),
+    : load_statistics_started_(false),
       on_statistics_loaded_(true  /* manual_reset */,
                             false /* initially_signaled */) {
 }
 
-void StatisticsProviderImpl::StartLoadingMachineStatistics() {
-  DCHECK(initialized_);
-  DCHECK(!load_statistics_started_);
+StatisticsProviderImpl::~StatisticsProviderImpl() {
+}
+
+void StatisticsProviderImpl::StartLoadingMachineStatistics(
+    const scoped_refptr<base::TaskRunner>& file_task_runner,
+    bool load_oem_manifest) {
+  CHECK(!load_statistics_started_);
   load_statistics_started_ = true;
 
-  VLOG(1) << "Started loading statistics";
-  BrowserThread::PostBlockingPoolTask(
+  VLOG(1) << "Started loading statistics. Load OEM Manifest: "
+          << load_oem_manifest;
+
+  file_task_runner->PostTask(
       FROM_HERE,
       base::Bind(&StatisticsProviderImpl::LoadMachineStatistics,
-                 base::Unretained(this)));
+                 base::Unretained(this),
+                 load_oem_manifest));
 }
 
-void StatisticsProviderImpl::LoadMachineStatistics() {
-  NameValuePairsParser parser(&machine_info_);
+void StatisticsProviderImpl::LoadMachineStatistics(bool load_oem_manifest) {

[satorux1, 2013/10/07 05:01:54]

add:

  DCHECK(file_task_runner_.RunsTasksOnCurrentThread())

[stevenjb, 2013/10/07 18:07:30]

Done.

[stevenjb, 2013/10/07 18:43:10]

Actually, can't add a DCHECK because we no longer store file_task_runner_.
However, this is a private method that is only called from one place, explicitly
from the task runner, so we don't really need a DCHECK. Added a comment instead.

+  if (cancellation_flag_.IsSet())

[satorux1, 2013/10/07 05:01:54]

This looks a bit scary. This will be a use-after-free if this function is called
after this object is deleted.

StatisticsProviderImpl is a singleton which will be deleted by AtExitManager. I
guess it's guaranteed that FILE thread is terminated before AtExitManager
deletes singletons, but the assumption sounds subtle (though it existed before
this patch). We could make the class a leaky singleton so we don't have to worry
about graceful shutdown, but that's also not desirable.

I think the root cause of the complication comes from the fact that the members
are modified on another thread. I wonder if we can get rid of the complication
as follows:

void LoadData(...) {
  // ... load data ...                                                          
                                                                
  waitable_event->Signal();
}

void StatisticsProviderImpl::StartLoadingMachineStatistics() {
  // The following three objects will be leaked if OnLoaded() is not called but
it's ok.                                                         
  MachineInfo* machine_info = new MachineInfo;
  MachineFlags* machine_flags = new MachineFlags;
  on_loaded_event_ = new WaitableEvent;  // this one needs to be a pointer
member

  on_loaded_callback_ = base::Bind(&StatisticsProviderImpl::OnLoaded,
                                   base::Owned(machine_info),
                                   base::Owned(machine_flags),
                                   base::Owned(on_loaded_event_);
  // Not a PostTaskAndReply as the callback will be run manually.
  file_task_runner->PostTask(base::Bind(&LoadData,  // free function            
                                                                
                                        machine_info,
                                        machine_flags,
                                        on_loaded_callback_));
}

void StatisticsProviderImpl::OnLoaded(MachineInfo* machine_info,
                                      MachineFlags* machine_flags,
                                      WaitableEvent* /* on_loaded_event */) {
  // copy the results.                                                          
                                                                
  machine_info_.swap(*machine_info);
  machine_flags_.swap(* machine_flags);
  loaded_ = true;
}

void StatisticsProviderImpl::WaitForStatisticsLoaded() {
  if (loaded_)
    return true;

  // ...                                                                        
                                                                
  on_loaded_event_->TimedWait(...);
  // ...                                                                        
                                                                
  if (on_loaded_event_->IsSignaled()) {
    // calls OnLoaded(), which copies data to members.
    on_loaded_callback_.Run();
    return true;
  }
  return false;
}


Hmm, this approach wasn't as simple as I wanted :P but I think it's still
(slightly) simpler than the current approach. What do you think?

[stevenjb, 2013/10/07 18:07:30]

I discussed the best way to do this with willchan@ and this is what we came up
with. I think your second paragraph best describes why it is OK to do it this
way; threads will always be stopped before Singleton instances are destroyed, so
there is no risk of use-after-free. I believe we rely on this pattern a fair
bit.

I'm not convinced the above is really any simpler, and it's not safer, just
commented better :) In fact, I would argue that the extra PostTask makes it
harder to follow. I can add a comment at the top of the function mentioning that
it will never get called after the Singleton is destroyed.

[satorux1, 2013/10/08 01:47:31]

That makes sense. Thank you for adding the comments.

+    return;
 
-  // Parse all of the key/value pairs from the crossystem tool.
-  if (!parser.ParseNameValuePairsFromTool(
-          arraysize(kCrosSystemTool), kCrosSystemTool, kCrosSystemEq,
-          kCrosSystemDelim, kCrosSystemCommentDelim)) {
-    LOG(WARNING) << "There were errors parsing the output of "
-                 << kCrosSystemTool << ".";
+  if (base::SysInfo::IsRunningOnChromeOS()) {
+    // Parse all of the key/value pairs from the crossystem tool.
+    NameValuePairsParser parser(&machine_info_);
+    if (!parser.ParseNameValuePairsFromTool(arraysize(kCrosSystemTool),
+                                            kCrosSystemTool,
+                                            kCrosSystemEq,
+                                            kCrosSystemDelim,
+                                            kCrosSystemCommentDelim)) {
+      LOG(ERROR) << "Errors parsing output from: " << kCrosSystemTool;
+    }
+
+    parser.GetNameValuePairsFromFile(base::FilePath(kMachineHardwareInfoFile),
+                                     kMachineHardwareInfoEq,
+                                     kMachineHardwareInfoDelim);
+    parser.GetNameValuePairsFromFile(base::FilePath(kEchoCouponFile),
+                                     kEchoCouponEq,
+                                     kEchoCouponDelim);
+    parser.GetNameValuePairsFromFile(base::FilePath(kVpdFile),
+                                     kVpdEq,
+                                     kVpdDelim);
   }
 
   // Ensure that the hardware class key is present with the expected
   // key name, and if it couldn't be retrieved, that the value is "unknown".
   std::string hardware_class = machine_info_[kHardwareClassCrosSystemKey];
   if (hardware_class.empty() || hardware_class == kCrosSystemUnknownValue)
     machine_info_[kHardwareClassKey] = kUnknownHardwareClass;
   else
     machine_info_[kHardwareClassKey] = hardware_class;
 
-  parser.GetNameValuePairsFromFile(base::FilePath(kMachineHardwareInfoFile),
-                                   kMachineHardwareInfoEq,
-                                   kMachineHardwareInfoDelim);
-  parser.GetNameValuePairsFromFile(base::FilePath(kEchoCouponFile),
-                                   kEchoCouponEq,
-                                   kEchoCouponDelim);
-  parser.GetNameValuePairsFromFile(base::FilePath(kVpdFile), kVpdEq, kVpdDelim);
+  if (load_oem_manifest) {
+    // If kAppOemManifestFile switch is specified, load OEM Manifest file.
+    CommandLine* command_line = CommandLine::ForCurrentProcess();
+    if (command_line->HasSwitch(switches::kAppOemManifestFile)) {
+      LoadOemManifestFromFile(
+          command_line->GetSwitchValuePath(switches::kAppOemManifestFile));
+    } else if (base::SysInfo::IsRunningOnChromeOS()) {
+      LoadOemManifestFromFile(base::FilePath(kOemManifestFilePath));
+    }
+  }
 
   // Finished loading the statistics.
   on_statistics_loaded_.Signal();
-  VLOG(1) << "Finished loading statistics";
+  VLOG(1) << "Finished loading statistics.";
 }
 
 void StatisticsProviderImpl::LoadOemManifestFromFile(
     const base::FilePath& file) {

[satorux1, 2013/10/07 05:01:54]

add: 

DCHECK(file_task_runner_.RunsTasksOnCurrentThread())

[stevenjb, 2013/10/07 18:07:30]

Done.

-  KioskOemManifestParser::Manifest oem_manifest;
-  if (!KioskOemManifestParser::Load(file, &oem_manifest))
+  if (cancellation_flag_.IsSet())
     return;
 
+  KioskOemManifestParser::Manifest oem_manifest;
+  if (!KioskOemManifestParser::Load(file, &oem_manifest)) {
+    LOG(WARNING) << "Unable to load OEM Manifest file: " << file.value();
+    return;
+  }
   machine_info_[kOemDeviceRequisitionKey] =
       oem_manifest.device_requisition;
   machine_flags_[kOemIsEnterpriseManagedKey] =
       oem_manifest.enterprise_managed;
   machine_flags_[kOemCanExitEnterpriseEnrollmentKey] =
       oem_manifest.can_exit_enrollment;
   machine_flags_[kOemKeyboardDrivenOobeKey] =
       oem_manifest.keyboard_driven_oobe;
+
+  VLOG(1) << "Loaded OEM Manifest statistics from " << file.value();
 }
 
 StatisticsProviderImpl* StatisticsProviderImpl::GetInstance() {
   return Singleton<StatisticsProviderImpl,
                    DefaultSingletonTraits<StatisticsProviderImpl> >::get();
 }
 
-// The stub StatisticsProvider implementation used on Linux desktop.
-class StatisticsProviderStubImpl : public StatisticsProviderImpl {
- public:
-  // StatisticsProvider implementation:
-  virtual void Init() OVERRIDE {}
+static StatisticsProvider* g_test_statistics_provider = NULL;
 
-  virtual void StartLoadingMachineStatistics() OVERRIDE {}
+// static
+StatisticsProvider* StatisticsProvider::GetInstance() {
+  if (g_test_statistics_provider)
+    return g_test_statistics_provider;
+  return StatisticsProviderImpl::GetInstance();
+}
 
-  virtual bool GetMachineStatistic(const std::string& name,
-                                   std::string* result) OVERRIDE {
-    if (name == "CHROMEOS_RELEASE_BOARD") {
-      // Note: syncer::GetSessionNameSynchronously() also uses the mechanism
-      // below to determine the CrOs release board. However, it cannot include
-      // statistics_provider.h and use this method because of the mutual
-      // dependency that creates between sync.gyp:sync and chrome.gyp:browser.
-      // TODO(rsimha): Update syncer::GetSessionNameSynchronously() if this code
-      // is ever moved into base/. See http://crbug.com/126732.
-      const CommandLine* command_line = CommandLine::ForCurrentProcess();
-      if (command_line->HasSwitch(chromeos::switches::kChromeOSReleaseBoard)) {
-        *result = command_line->
-            GetSwitchValueASCII(chromeos::switches::kChromeOSReleaseBoard);
-        return true;
-      }
-    }
-    return false;
-  }
-
-  virtual void LoadOemManifest() OVERRIDE {
-    CommandLine* command_line = CommandLine::ForCurrentProcess();
-    if (!command_line->HasSwitch(switches::kAppOemManifestFile))
-      return;
-
-    LoadOemManifestFromFile(
-        command_line->GetSwitchValuePath(switches::kAppOemManifestFile));
-  }
-
-  static StatisticsProviderStubImpl* GetInstance() {
-    return Singleton<StatisticsProviderStubImpl,
-        DefaultSingletonTraits<StatisticsProviderStubImpl> >::get();
-  }
-
- private:
-  friend struct DefaultSingletonTraits<StatisticsProviderStubImpl>;
-
-  StatisticsProviderStubImpl() {
-  }
-
-  DISALLOW_COPY_AND_ASSIGN(StatisticsProviderStubImpl);
-};
-
-StatisticsProvider* StatisticsProvider::GetInstance() {
-  if (base::SysInfo::IsRunningOnChromeOS()) {
-    return StatisticsProviderImpl::GetInstance();
-  } else {
-    return StatisticsProviderStubImpl::GetInstance();
-  }
+// static
+void StatisticsProvider::SetTestProvider(StatisticsProvider* test_provider) {
+  g_test_statistics_provider = test_provider;
 }
 
 }  // namespace system
 }  // namespace chromeos