Delay the metrics policy migration call to make sure ownership has been taken.

chrome/browser/chromeos/user_cros_settings_provider.cc

Index: chrome/browser/chromeos/user_cros_settings_provider.cc
diff --git a/chrome/browser/chromeos/user_cros_settings_provider.cc b/chrome/browser/chromeos/user_cros_settings_provider.cc
index c50e429895399c73c5e8519a08951ff99de3f5b3..d8d50e33818f872a81f4dda3673abba6cf74b8a0 100644
--- a/chrome/browser/chromeos/user_cros_settings_provider.cc
+++ b/chrome/browser/chromeos/user_cros_settings_provider.cc
@@ -27,6 +27,7 @@
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
 #include "chrome/browser/ui/options/options_util.h"
+#include "chrome/common/chrome_notification_types.h"
 #include "chrome/installer/util/google_update_settings.h"
 #include "content/browser/browser_thread.h"
 
@@ -58,21 +59,84 @@ const char* kListSettings[] = {
   kAccountsPrefUsers
 };
 
-// Only write the property if the owner is the current logged on user.
-void StartStorePropertyOpIfOwner(const std::string& name,
-                                 const std::string& value,
-                                 SignedSettingsHelper::Callback* callback) {
-  if (OwnershipService::GetSharedInstance()->CurrentUserIsOwner()) {
-    BrowserThread::PostTask(BrowserThread::UI,
-                            FROM_HERE,
-                            base::Bind(
-                                &SignedSettingsHelper::StartStorePropertyOp,
-                                base::Unretained(SignedSettingsHelper::Get()),
-                                name,
-                                value,
-                                callback));
+// This class provides the means to migrate settings to the signed settings
+// store. It does one of three things - stored the settings in the policy blob

[Mattias Nissler (ping if slow), 2011/08/31 12:47:24]

s/stored/store/

[pastarmovj, 2011/08/31 14:29:36]

Done.

+// immediately if the current user is the owner. Uses the
+// SignedSettingsTempStorage if there is no owner yet, or waits for an
+// OWNERSHIP_CHECKED notification to delay the storing until the owner has
+// logged in.
+class MigrationHelper : public NotificationObserver {
+ public:
+  explicit MigrationHelper() : callback_(NULL) {
+    registrar_.Add(this, chrome::NOTIFICATION_OWNERSHIP_CHECKED,
+                   NotificationService::AllSources());
   }
-}
+
+  void set_callback(SignedSettingsHelper::Callback* callback) {
+    callback_ = callback;
+  }
+
+  void AddMigrationValue(const std::string& path, const std::string& value) {
+    // Migration value is not thread safe so take care if you call this function

[Mattias Nissler (ping if slow), 2011/08/31 12:47:24]

what is not thread safe? Isn't it the case that you want to call MigrateValues()
every time you call AddMigrationValue()? If that is true, let's just do the
thread switching in AddMigrationValue().

[pastarmovj, 2011/08/31 14:29:36]

This comment is obsolete now that we don't have to jump around different
threads.

On 2011/08/31 12:47:24, Mattias Nissler wrote:

+    // while MigrateValues is running. You can make sure this is not the case if
+    // You only call this function on the FILE thread.
+    migration_values_[path] = value;
+  }
+
+  void MigrateValues(void) {
+    // The check we do below should be done on the file thread so if we are not
+    // there jump on the right thread.

[Mattias Nissler (ping if slow), 2011/08/31 12:47:24]

s/jump on/jump to/

[pastarmovj, 2011/08/31 14:29:36]

Done.

+    if (!BrowserThread::CurrentlyOn(BrowserThread::FILE)) {
+      BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+                              base::Bind(&MigrationHelper::MigrateValues,
+                                         base::Unretained(this)));
+      return;
+    }
+
+    OwnershipService* service = OwnershipService::GetSharedInstance();
+    if (service->CurrentUserIsOwner() ||
+        service->GetStatus(true) != OwnershipService::OWNERSHIP_TAKEN) {
+      std::map<std::string, std::string>::const_iterator i =
+          migration_values_.begin();

[Mattias Nissler (ping if slow), 2011/08/31 12:47:24]

maybe move the i = migration_values_.begin() into the loop header if it fits?

[pastarmovj, 2011/08/31 14:29:36]

Done.

+      for (; i != migration_values_.end(); ++i) {
+        // This is needed to avoid loosing the value after the clear below.
+        // Temp objects passed by const ref will have the life time of the ref.
+        const std::string name = i->first;
+        const std::string value = i->second;
+        // Queue all values for storing.
+        BrowserThread::PostTask(

[Mattias Nissler (ping if slow), 2011/08/31 12:47:24]

It seems like you should only do the ownership checks on the file thread and
everything else on the UI thread. This also avoids accessing |migration_values_|
from different threads. Just post a task to the FILE thread to make the check,
then post a task back for the result. Bonus points if you make this into a
reusable component in the spirit of OwnershipStatusChecker.

[pastarmovj, 2011/08/31 14:29:36]

Tuned the OwnershipStatusChecker to supply all the info I needed.

On 2011/08/31 12:47:24, Mattias Nissler wrote:

+            BrowserThread::UI, FROM_HERE,
+            base::Bind(
+                &SignedSettingsHelper::StartStorePropertyOp,
+                base::Unretained(SignedSettingsHelper::Get()),
+                name, value,
+                callback_));
+      }
+      migration_values_.clear();
+    } else {
+      // Either we are not yet logged in or the user currently logged in is not
+      // the owner. So we should wait for user change. (Actually only the first
+      // case is interesting for us.)
+    }
+  }
+
+  // NotificationObserver overrides:
+  virtual void Observe(int type,
+                      const NotificationSource& source,

[Mattias Nissler (ping if slow), 2011/08/31 12:47:24]

indentation.

[pastarmovj, 2011/08/31 14:29:36]

Done.

+                      const NotificationDetails& details) OVERRIDE {
+    if (type == chrome::NOTIFICATION_OWNERSHIP_CHECKED) {
+      if (!migration_values_.empty())

[Mattias Nissler (ping if slow), 2011/08/31 12:47:24]

it's not safe to check from the UI thread, just don't.

[pastarmovj, 2011/08/31 14:29:36]

Done.

+        MigrateValues();
+    }
+  }
+
+ private:
+  NotificationRegistrar registrar_;
+  std::map<std::string, std::string> migration_values_;
+  SignedSettingsHelper::Callback* callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(MigrationHelper);
+};
 
 bool IsControlledBooleanSetting(const std::string& pref_path) {
   // TODO(nkostylev): Using std::find for 4 value array generates this warning
@@ -289,6 +353,7 @@ class UserCrosSettingsTrust : public SignedSettingsHelper::Callback {
   UserCrosSettingsTrust()
       : ownership_service_(OwnershipService::GetSharedInstance()),
         retries_left_(kNumRetriesLimit) {
+    migration_helper_.set_callback(this);
     // Start prefetching Boolean and String preferences.
     Reload();
   }
@@ -352,14 +417,10 @@ class UserCrosSettingsTrust : public SignedSettingsHelper::Callback {
         // Temporarily allow it until we fix http://crbug.com/62626
         base::ThreadRestrictions::ScopedAllowIO allow_io;
         stats_consent = GoogleUpdateSettings::GetCollectStatsConsent();
-        // Only store settings if the owner is logged on, otherwise the write
-        // will fail, triggering another read and we'll end up in an infinite
-        // loop. Owner check needs to be done on the FILE thread.
-        BrowserThread::PostTask(BrowserThread::FILE,
-                                FROM_HERE,
-                                base::Bind(&StartStorePropertyOpIfOwner, path,
-                                           stats_consent ? "true" : "false",
-                                           this));
+        // Make sure the values will get eventually written to the policy file.
+        migration_helper_.AddMigrationValue(
+            path, stats_consent ? "true" : "false");
+        migration_helper_.MigrateValues();
         UpdateCacheBool(path, stats_consent, USE_VALUE_SUPPLIED);
         LOG(WARNING) << "No metrics policy set will revert to checking "
                      << "consent file which is "
@@ -491,6 +552,7 @@ class UserCrosSettingsTrust : public SignedSettingsHelper::Callback {
   base::hash_map< std::string, std::vector< Task* > > callbacks_;
 
   OwnershipService* ownership_service_;
+  MigrationHelper migration_helper_;
 
   // In order to guard against occasional failure to fetch a property
   // we allow for some number of retries.