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
+// immediately if the current user is the owner. Uses the
+// SignedSettingsTempStorage if there is no owner yet. Or

[rkc, 2011/08/31 11:18:11]

Nit: Incomplete comment?

[pastarmovj, 2011/08/31 11:50:58]

Ooops true.

+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
+    // 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.
+    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();
+      for (; i != migration_values_.end(); ++i) {
+        // This is needed to avoid loosing the value after the clear below.
+        const std::string value = i->second;
+        // Queue all values for storing.
+        BrowserThread::PostTask(
+            BrowserThread::UI, FROM_HERE,
+            base::Bind(
+                &SignedSettingsHelper::StartStorePropertyOp,
+                base::Unretained(SignedSettingsHelper::Get()),
+                i->first.c_str(), value,

[rkc, 2011/08/31 11:18:11]

Why the difference between the passing of these two parameters? They seem to be
identical in the function declaration and the map template.

[pastarmovj, 2011/08/31 11:50:58]

Underneath they are not the same because the key(first) is stored as const
string and the value second as a non-const string. And the compiler is clever
enough to extend the lifetime of the const object due to the const ref argument
but the value is not and the clear below deletes it.

For the sake of clarity I decided to go for unifying them.

+                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,
+                      const NotificationDetails& details) OVERRIDE {
+    if (type == chrome::NOTIFICATION_OWNERSHIP_CHECKED) {
+      if (!migration_values_.empty())
+        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