Added asynchronous notification of readiness to the StatisticsProvider, and

chrome/browser/metrics/metrics_service.cc

Index: chrome/browser/metrics/metrics_service.cc
diff --git a/chrome/browser/metrics/metrics_service.cc b/chrome/browser/metrics/metrics_service.cc
index c691f70268c9ebff8018cb199862dd7ed64c8ed1..9d02bb2bddb4f0d67f23f0cf6a523ee8f6fe15d7 100644
--- a/chrome/browser/metrics/metrics_service.cc
+++ b/chrome/browser/metrics/metrics_service.cc
@@ -89,11 +89,10 @@
 // initial log.
 //
 //    INIT_TASK_SCHEDULED,    // Waiting for deferred init tasks to complete.
-// Typically about 30 seconds after startup, a task is sent to a second thread
-// (the file thread) to perform deferred (lower priority and slower)
-// initialization steps such as getting the list of plugins.  That task will
-// (when complete) make an async callback (via a Task) to indicate the
-// completion.
+// Typically about 30 seconds after startup, a task is posted to perform
+// deferred (lower priority and slower) initialization steps such as getting the
+// list of plugins.  That task will (when complete) make an async callback (via
+// a Task) to indicate the completion.
 //
 //    INIT_TASK_DONE,         // Waiting for timer to send initial log.
 // The callback has arrived, and it is now possible for an initial log to be
@@ -199,7 +198,6 @@
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/cros/cros_library.h"
 #include "chrome/browser/chromeos/external_metrics.h"
-#include "chrome/browser/chromeos/system/statistics_provider.h"
 #endif
 
 using base::Time;
@@ -222,6 +220,10 @@ bool IsSingleThreaded() {
 // initialization work.
 const int kInitializationDelaySeconds = 30;
 
+// The timeout, in seconds, to resume initialization of the MetricsService if
+// the initialization of the StatisticsProvider hasn't completed yet.
+const int kStatisticsProviderTimeoutSeconds = 300;
+
 // This specifies the amount of time to wait for all renderers to send their
 // data.
 const int kMaxHistogramGatheringWaitDuration = 60000;  // 60 seconds.
@@ -243,6 +245,11 @@ const int kSaveStateIntervalMinutes = 5;
 // e.g., the server is down.
 const int kNoResponseCode = content::URLFetcher::RESPONSE_CODE_INVALID - 1;
 
+// Used to indicate that a report was generated before the hardware_class
+// property was available from the StatisticsProvider. This is used to identify
+// faulty reports from Chrome OS clients.
+const char kHardwareClassNotReady[] = "(not ready)";
+
 }
 
 // static
@@ -406,6 +413,9 @@ MetricsService::MetricsService()
 
 MetricsService::~MetricsService() {
   SetRecording(false);
+#if defined(OS_CHROMEOS)
+  chromeos::system::StatisticsProvider::GetInstance()->RemoveObserver(this);

[jar (doing other things), 2012/05/21 18:10:02]

Are we sure it was added before we call this remove?

[Joao da Silva, 2012/05/22 14:55:25]

Removing an observer that isn't registered is OK. Otherwise we'd have to track
whether we are currently observing...

+#endif
 }
 
 void MetricsService::Start() {
@@ -750,32 +760,66 @@ void MetricsService::InitializeMetricsState() {
   ScheduleNextStateSave();
 }
 
-// static
-void MetricsService::InitTaskGetHardwareClass(
-    base::WeakPtr<MetricsService> self,
-    base::MessageLoopProxy* target_loop) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+void MetricsService::InitTaskGetHardwareClass() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_EQ(INIT_TASK_SCHEDULED, state_);
 
-  std::string hardware_class;
 #if defined(OS_CHROMEOS)
-  chromeos::system::StatisticsProvider::GetInstance()->GetMachineStatistic(
-      "hardware_class", &hardware_class);
+  // Exactly one of these two callbacks will be invoked. |on_timeout_callback_|
+  // is posted to UI with a certain delay; |on_ready_callback_| is invoked
+  // once the StatisticsProvider() becomes ready. Execution of one of these
+  // callbacks cancels the execution of the other.
+  on_timeout_callback_.Reset(
+      base::Bind(&MetricsService::OnStatisticsProviderTimeout,
+                 base::Unretained(this)));
+  on_ready_callback_.Reset(
+      base::Bind(&MetricsService::InitTaskGetPluginInfo,
+                 base::Unretained(this)));
+
+  BrowserThread::PostDelayedTask(
+      BrowserThread::UI, FROM_HERE,
+      on_timeout_callback_.callback(),
+      base::TimeDelta::FromSeconds(kStatisticsProviderTimeoutSeconds));
+
+  // The hardware class can only be retrieved once the StatisticsProvider is
+  // ready. This usually happens early enough, but can take longer on some
+  // faulty hardware.
+  chromeos::system::StatisticsProvider::GetInstance()->AddObserver(this);

[jar (doing other things), 2012/05/21 18:10:02]

I *think* the semantics of AddObserver() are such that they sometimes
synchronously call the completion, and other times they actually add to the
list.  This leaves us unsure if we need to do a remove later.

+#else
+  InitTaskGetPluginInfo();
 #endif  // OS_CHROMEOS
+}
 
-  target_loop->PostTask(FROM_HERE,
-      base::Bind(&MetricsService::OnInitTaskGotHardwareClass,
-          self, hardware_class));
+#if defined(OS_CHROMEOS)
+
+void MetricsService::OnMachineStatisticsReady() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  chromeos::system::StatisticsProvider::GetInstance()->GetMachineStatistic(
+      "hardware_class", &hardware_class_);
+  on_timeout_callback_.Cancel();
+  if (!on_ready_callback_.IsCancelled())

[jar (doing other things), 2012/05/21 18:10:02]

This cancellable callback looks more complex than need be.

This code is only called on the UI thread, and the Run() method on the next
line, if it runs at all, will call InitTaskGetPluginInfo() (just like we do in
the OnStatisticsProviderTimeout().

Why don't we just have boolean like:
get_plugin_info_was_called_

What is the complexity I'm missing?

[Joao da Silva, 2012/05/22 14:55:25]

That's another way to do it, I just wanted to avoid adding that state. I'll
refactor this like that.

[jar (doing other things), 2012/05/22 17:07:58]

Note: You could implement the "was_called" bool inside of the method, so that
extraneous calls would be ignored. That might simplify your concerns about state
(held in the instance), but it might make it harder to test etc.

On 2012/05/22 14:55:25, Joao da Silva wrote:

+    on_ready_callback_.callback().Run();
 }
 
-void MetricsService::OnInitTaskGotHardwareClass(
-    const std::string& hardware_class) {
+void MetricsService::OnStatisticsProviderTimeout() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  // Use an invalid hardware_class temporarily until the StatisticsProvider is
+  // ready.
+  hardware_class_ = kHardwareClassNotReady;
+  on_ready_callback_.Cancel();
+  InitTaskGetPluginInfo();
+}
+
+#endif  // OS_CHROMEOS
+
+void MetricsService::InitTaskGetPluginInfo() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK_EQ(INIT_TASK_SCHEDULED, state_);
-  hardware_class_ = hardware_class;
 
   // Start the next part of the init task: loading plugin information.
   PluginService::GetInstance()->GetPlugins(
       base::Bind(&MetricsService::OnInitTaskGotPluginInfo,
-          self_ptr_factory_.GetWeakPtr()));
+                 self_ptr_factory_.GetWeakPtr()));
 }
 
 void MetricsService::OnInitTaskGotPluginInfo(
@@ -892,16 +936,12 @@ void MetricsService::StartRecording() {
     // We only need to schedule that run once.
     state_ = INIT_TASK_SCHEDULED;
 
-    // Schedules a task on the file thread for execution of slower
-    // initialization steps (such as plugin list generation) necessary
-    // for sending the initial log.  This avoids blocking the main UI
-    // thread.
-    BrowserThread::PostDelayedTask(
-        BrowserThread::FILE,
+    // Schedules a delayed task for execution of slower initialization steps
+    // (such as plugin list generation) necessary for sending the initial log.
+    MessageLoop::current()->PostDelayedTask(

[jar (doing other things), 2012/05/21 18:10:02]

Why are we now willing to do this on the current (UI??) thread?

[Joao da Silva, 2012/05/22 14:55:25]

Because this isn't a blocking operation anymore, it just sets up the timeout
task and registers an observer.

[jar (doing other things), 2012/05/22 17:07:58]

Since it is non-blocking, there should be even less concern about running on the
UI thread.

I think registering an observer is supposed to be done on the "observer's
thread."  All notifications are done synchronously on that thread as well.

If you want cross-thread notification, or registration, you probably need to use
ObserverListThreadSafe... but that might mess things up and get the callback to
the registration thread :-/.

My guess is that you want all registration and notification to happen on the UI
thread... so I think you should go ahead and post to the UI thread still (and
not bother with ObserverListThreadSafe).


On 2012/05/22 14:55:25, Joao da Silva wrote:

[Joao da Silva, 2012/05/22 19:10:09]

I may be misunderstanding, but that is the case. The observer is added in the UI
thread, and the notification also comes in the UI thread.
Alright, I'll leave it as is then.

         FROM_HERE,
         base::Bind(&MetricsService::InitTaskGetHardwareClass,
-            self_ptr_factory_.GetWeakPtr(),
-            MessageLoop::current()->message_loop_proxy()),
+                   self_ptr_factory_.GetWeakPtr()),
         base::TimeDelta::FromSeconds(kInitializationDelaySeconds));
   }
 }