Added asynchronous notification of readiness to the StatisticsProvider, and

chrome/browser/chromeos/system/statistics_provider.cc

Index: chrome/browser/chromeos/system/statistics_provider.cc
diff --git a/chrome/browser/chromeos/system/statistics_provider.cc b/chrome/browser/chromeos/system/statistics_provider.cc
index f7b88ff1d3504bf5ea5f538d4e6a5ce5be6fbfdf..66a0cf3796e18cd3e7b2dae8e3d836819cfe02b9 100644
--- a/chrome/browser/chromeos/system/statistics_provider.cc
+++ b/chrome/browser/chromeos/system/statistics_provider.cc
@@ -5,11 +5,13 @@
 #include "chrome/browser/chromeos/system/statistics_provider.h"
 
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/chromeos/chromeos_version.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/memory/singleton.h"
+#include "base/message_loop.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/time.h"
 #include "base/chromeos/chromeos_version.h"
@@ -62,6 +64,15 @@ const char kVpdDelim[] = "\n";
 // Timeout that we should wait for statistics to get loaded
 const int kTimeoutSecs = 3;
 
+// Helper to invoke |callbacks| on the UI thread.
+void NotifyOnUI(std::vector<base::Closure>* callbacks) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  for (std::vector<base::Closure>::iterator it = callbacks->begin();
+       it != callbacks->end(); ++it) {
+    it->Run();
+  }
+}
+
 }  // namespace
 
 // The StatisticsProvider implementation used in production.
@@ -71,6 +82,8 @@ class StatisticsProviderImpl : public StatisticsProvider {
   virtual bool GetMachineStatistic(const std::string& name,
                                    std::string* result) OVERRIDE;
 
+  virtual void WhenReady(const base::Closure& callback) OVERRIDE;
+
   static StatisticsProviderImpl* GetInstance();
 
  private:
@@ -86,6 +99,7 @@ class StatisticsProviderImpl : public StatisticsProvider {
 
   NameValuePairsParser::NameValueMap machine_info_;
   base::WaitableEvent on_statistics_loaded_;
+  std::vector<base::Closure> callbacks_;
 
   DISALLOW_COPY_AND_ASSIGN(StatisticsProviderImpl);
 };
@@ -123,6 +137,16 @@ bool StatisticsProviderImpl::GetMachineStatistic(
   return false;

[jar (doing other things), 2012/04/24 18:37:40]

This API makes it impossible to tell whether there was a time-out, or if the
value was not present.  Hence you can't tell if it is "worth asking again."

[Joao da Silva, 2012/04/27 09:47:48]

This problem doesn't exist with the async notification; every user of this
interface should be refactored to use the async interface, and the WaitableEvent
should be removed.

[jar (doing other things), 2012/04/27 18:00:38]

I think what you're saying is that you plan to remove this API... and hence you
don't want to fix it.  Is that correct?

On 2012/04/27 09:47:48, Joao da Silva wrote:

[Joao da Silva, 2012/05/15 13:52:47]

This API is used in a couple of places that aren't straightforward to refactor
to wait for the asynchronous notification. For example,
StartupCustomizationDocument (chrome/browser/chromeos/customization_document.cc)
call this from the ctor, and I don't see an easy way to make it wait for an
async notification.

So for now I'd like to fix the problem at hand (empty hardware_class in UMA
reports), and introduce an API that clients who care should use :-)

 }
 
+void StatisticsProviderImpl::WhenReady(const base::Closure& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (on_statistics_loaded_.IsSignaled()) {
+    // Don't assume the caller is re-entrant.
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback);
+  } else {
+    callbacks_.push_back(callback);
+  }
+}
+
 // manual_reset needs to be true, as we want to keep the signaled state.
 StatisticsProviderImpl::StatisticsProviderImpl()
     : on_statistics_loaded_(true  /* manual_reset */,
@@ -172,6 +196,14 @@ void StatisticsProviderImpl::LoadMachineStatistics() {
   on_statistics_loaded_.Signal();
   VLOG(1) << "Finished loading statistics";
 
+  // Any callbacks registed before Signal() are in |callbacks_|. Any registed
+  // after Signal() will be immediately posted. NotifyOnUI() posts the callbacks
+  // in |callbacks_| from the UI loop.
+  std::vector<base::Closure>* callbacks = new std::vector<base::Closure>();
+  callbacks->swap(callbacks_);

[jar (doing other things), 2012/04/24 18:37:40]

What thread are you running on?  Are you ensured to be on the UI thread, where
callbacks is accessed?

[Joao da Silva, 2012/04/27 09:47:48]

This happens in the blocking pool, but the callbacks are passed to UI before
being invoked. The |callbacks_| vector isn't modified from UI anymore after
signaling the WaitableEvent.

[jar (doing other things), 2012/04/27 18:00:38]

It appears as though the line:

callbacks_.push_back(callback);

can be executed asynchronously on the UI thread.  Keep in mind that the test
that preceeds it (by several lines) may be executed befroe you Signal() on line
196 (since we are on different threads).

It is not clear to me what will happen if the swap() is being executed at the
same time as the push is being attempted.  It is plausible that you'd be safe,
but it is also quite likely that we'd corrupt memory.

What am I missing?

On 2012/04/27 09:47:48, Joao da Silva wrote:

[Joao da Silva, 2012/05/15 13:52:47]

You're totally right. I tried to make this a static method to execute on the
blocking pool and pass it a WeakPtr created from UI so that it could signal
completion to UI, and pass the parsed data along the callback. However that'd
mean that whoever is blocking in GetMachineStatistic() won't see the data before
this task runs in UI too, so I gave up that approach.

The current solution is to post back to UI a task just to invoke the callbacks,
but keep the WaitableEvent to unblock whoever might be waiting at
GetMachineStatistic() on UI. WDYT?

(I'd really really like to get the whole thing right, but it's not easy at all
to modify callers of GetMachineStatistic() that expect it to be a synchronous
call...)

+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                          base::Bind(NotifyOnUI, base::Owned(callbacks)));
+
 #if defined(GOOGLE_CHROME_BUILD)
   // TODO(kochi): This is for providing a channel information to
   // chrome::VersionInfo::GetChannel()/GetVersionStringModifier(),
@@ -210,6 +242,11 @@ class StatisticsProviderStubImpl : public StatisticsProvider {
     return false;
   }
 
+  virtual void WhenReady(const base::Closure& callback) OVERRIDE {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback);
+  }
+
   static StatisticsProviderStubImpl* GetInstance() {
     return Singleton<StatisticsProviderStubImpl,
         DefaultSingletonTraits<StatisticsProviderStubImpl> >::get();