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

chrome/browser/chromeos/user_cros_settings_provider.cc

 // Copyright (c) 2011 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/user_cros_settings_provider.h"
 
 #include <map>
 #include <set>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/hash_tables.h"
 #include "base/logging.h"
 #include "base/memory/singleton.h"
 #include "base/string_util.h"
 #include "base/task.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/cros/cros_library.h"
 #include "chrome/browser/chromeos/cros/login_library.h"
 #include "chrome/browser/chromeos/cros/network_library.h"
 #include "chrome/browser/chromeos/cros_settings.h"
 #include "chrome/browser/chromeos/cros_settings_names.h"
 #include "chrome/browser/chromeos/login/ownership_service.h"
 #include "chrome/browser/chromeos/login/user_manager.h"
 #include "chrome/browser/policy/browser_policy_connector.h"
 #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"
 
 namespace chromeos {
 
 namespace {
 
 const char kTrueIncantation[] = "true";
 const char kFalseIncantation[] = "false";
 const char kTrustedSuffix[] = "/trusted";
@@ skipping 11 matching lines @@
 
 const char* kStringSettings[] = {
   kDeviceOwner,
   kReleaseChannel
 };
 
 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
   // in chroot stl_algo.h:231: error: array subscript is above array bounds.
   // GCC 4.4.3
   return (pref_path == kAccountsPrefAllowNewUser) ||
          (pref_path == kAccountsPrefAllowGuest) ||
          (pref_path == kAccountsPrefShowUserNamesOnSignIn) ||
          (pref_path == kSignedDataRoamingEnabled) ||
          (pref_path == kStatsReportingPref);
@@ skipping 196 matching lines @@
     }
   }
 
  private:
   // upper bound for number of retries to fetch a signed setting.
   static const int kNumRetriesLimit = 9;
 
   UserCrosSettingsTrust()
       : ownership_service_(OwnershipService::GetSharedInstance()),
         retries_left_(kNumRetriesLimit) {
+    migration_helper_.set_callback(this);
     // Start prefetching Boolean and String preferences.
     Reload();
   }
 
   virtual ~UserCrosSettingsTrust() {
     if (BrowserThread::CurrentlyOn(BrowserThread::UI) &&
         CrosLibrary::Get()->EnsureLoaded()) {
       // Cancels all pending callbacks from us.
       SignedSettingsHelper::Get()->CancelCallback(this);
     }
@@ skipping 43 matching lines @@
       }
     } else if (path == kStatsReportingPref) {
       bool stats_consent = (use_value == USE_VALUE_SUPPLIED) ? value : false;
       // TODO(pastarmovj): Remove this once migration is not needed anymore.
       // If the value is not set we should try to migrate legacy consent file.
       if (use_value == USE_VALUE_DEFAULT) {
         // Loading consent file state causes us to do blocking IO on UI thread.
         // 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 "
                      << (stats_consent ? "on." : "off.");
       }
       // TODO(pastarmovj): Remove this once we don't need to regenerate the
       // consent file for the GUID anymore.
       VLOG(1) << "Metrics policy is being set to : " << stats_consent
               << "(reason : " << use_value << ")";
       OptionsUtil::ResolveMetricsReportingEnabled(stats_consent);
@@ skipping 111 matching lines @@
 
     // Reload the whitelist on settings op failure.
     if (code != SignedSettings::SUCCESS)
       CrosSettings::Get()->FireObservers(kAccountsPrefUsers);
   }
 
   // Pending callbacks that need to be invoked after settings verification.
   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.
   int retries_left_;
 
   friend class SignedSettingsHelper;
   friend struct DefaultSingletonTraits<UserCrosSettingsTrust>;
 
   DISALLOW_COPY_AND_ASSIGN(UserCrosSettingsTrust);
 };
@@ skipping 194 matching lines @@
   if (cached_whitelist_update->Remove(email_value, NULL))
     prefs->ScheduleSavePersistentPrefs();
 }
 
 // static
 void UserCrosSettingsProvider::UpdateCachedOwner(const std::string& email) {
   UpdateCacheString(kDeviceOwner, email, USE_VALUE_SUPPLIED);
 }
 
 }  // namespace chromeos