SupervisedUsers : Fix transaction cleanup

chrome/browser/chromeos/login/user_manager_impl.cc

Index: chrome/browser/chromeos/login/user_manager_impl.cc
diff --git a/chrome/browser/chromeos/login/user_manager_impl.cc b/chrome/browser/chromeos/login/user_manager_impl.cc
index 3f772404ab0224eee4419e921441830a6dfa391c..a1f40e542b97d358d00dff69c13f52d9b0507b99 100644
--- a/chrome/browser/chromeos/login/user_manager_impl.cc
+++ b/chrome/browser/chromeos/login/user_manager_impl.cc
@@ -126,36 +126,6 @@ void OnRemoveUserComplete(const std::string& user_email,
   }
 }
 
-// This method is used to implement UserManager::RemoveUser.
-void RemoveUserInternal(const std::string& user_email,
-                        chromeos::RemoveUserDelegate* delegate) {
-  CrosSettings* cros_settings = CrosSettings::Get();
-
-  // Ensure the value of owner email has been fetched.
-  if (CrosSettingsProvider::TRUSTED != cros_settings->PrepareTrustedValues(
-          base::Bind(&RemoveUserInternal, user_email, delegate))) {
-    // Value of owner email is not fetched yet.  RemoveUserInternal will be
-    // called again after fetch completion.
-    return;
-  }
-  std::string owner;
-  cros_settings->GetString(kDeviceOwner, &owner);
-  if (user_email == owner) {
-    // Owner is not allowed to be removed from the device.
-    return;
-  }
-
-  if (delegate)
-    delegate->OnBeforeUserRemoved(user_email);
-
-  chromeos::UserManager::Get()->RemoveUserFromList(user_email);
-  cryptohome::AsyncMethodCaller::GetInstance()->AsyncRemove(
-      user_email, base::Bind(&OnRemoveUserComplete, user_email));
-
-  if (delegate)
-    delegate->OnUserRemoved(user_email);
-}
-
 // Helper function that copies users from |users_list| to |users_vector| and
 // |users_set|. Duplicates and users already present in |existing_users| are
 // skipped.
@@ -511,12 +481,56 @@ void UserManagerImpl::RemoveUser(const std::string& user_id,
   RemoveUserInternal(user_id, delegate);
 }
 
+// This method is used to implement UserManager::RemoveUser.

[Nikita (slow), 2013/11/13 01:37:19]

nit: drop comment.

+void UserManagerImpl::RemoveUserInternal(const std::string& user_email,
+                                         RemoveUserDelegate* delegate) {
+  CrosSettings* cros_settings = CrosSettings::Get();
+
+  // Ensure the value of owner email has been fetched.
+  if (CrosSettingsProvider::TRUSTED != cros_settings->PrepareTrustedValues(
+          base::Bind(&UserManagerImpl::RemoveUserInternal,
+                     base::Unretained(this),
+                     user_email, delegate))) {
+    // Value of owner email is not fetched yet.  RemoveUserInternal will be
+    // called again after fetch completion.
+    return;
+  }
+  std::string owner;
+  cros_settings->GetString(kDeviceOwner, &owner);
+  if (user_email == owner) {
+    // Owner is not allowed to be removed from the device.
+    return;
+  }
+  RemoveNonOwnerUserInternal(user_email, delegate);
+}
+
+void UserManagerImpl::RemoveNonOwnerUserInternal(const std::string& user_email,
+                                                 RemoveUserDelegate* delegate) {
+  if (delegate)
+    delegate->OnBeforeUserRemoved(user_email);
+  RemoveUserFromList(user_email);
+  cryptohome::AsyncMethodCaller::GetInstance()->AsyncRemove(
+      user_email, base::Bind(&OnRemoveUserComplete, user_email));
+
+  if (delegate)
+    delegate->OnUserRemoved(user_email);
+}
+
 void UserManagerImpl::RemoveUserFromList(const std::string& user_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  EnsureUsersLoaded();

[Nikita (slow), 2013/11/13 01:37:19]

Do you think whether removing this call might have some side effects?

   RemoveNonCryptohomeData(user_id);
-  User* user = RemoveRegularOrLocallyManagedUserFromList(user_id);
-  delete user;
+  if (users_loaded_) {
+    User* user = RemoveRegularOrLocallyManagedUserFromList(user_id);
+    delete user;
+  } else {
+    DCHECK(gaia::ExtractDomainName(user_id) ==
+        UserManager::kLocallyManagedUserDomain);
+    // Special case, removing partially-constructed supervised user during user
+    // list loading.
+    ListPrefUpdate users_update(g_browser_process->local_state(),
+                                kRegularUsers);
+    users_update->Remove(base::StringValue(user_id), NULL);
+  }
   // Make sure that new data is persisted to Local State.
   g_browser_process->local_state()->CommitPendingWrite();
 }
@@ -957,7 +971,7 @@ bool UserManagerImpl::IsUserNonCryptohomeDataEphemeral(
 
   // Data belonging to the owner, anyone found on the user list and obsolete
   // public accounts whose data has not been removed yet is not ephemeral.
-  if (user_id == owner_email_  || FindUserInList(user_id) ||
+  if (user_id == owner_email_  || UserExistsInList(user_id) ||
       user_id == g_browser_process->local_state()->
           GetString(kPublicAccountPendingDataRemoval)) {
     return false;
@@ -1033,9 +1047,9 @@ void UserManagerImpl::EnsureUsersLoaded() {
 
   if (users_loaded_)
     return;
-  users_loaded_ = true;
-
-  // Clean up user list first.
+  // Clean up user list first. All code down the path should be synchronous,
+  // so that local state after transaction rollback is in consistent state.
+  // This process also should not trigger EnsureUsersLoaded again.
   if (supervised_user_manager_->HasFailedUserCreationTransaction())
     supervised_user_manager_->RollbackUserCreationTransaction();
 
@@ -1087,6 +1101,7 @@ void UserManagerImpl::EnsureUsersLoaded() {
     users_.push_back(User::CreatePublicAccountUser(*it));
     UpdatePublicAccountDisplayName(*it);
   }
+  users_loaded_ = true;
 
   user_image_manager_->LoadUserImages(users_);
 }
@@ -1164,6 +1179,17 @@ const User* UserManagerImpl::FindUserInList(const std::string& user_id) const {
   return NULL;
 }
 
+const bool UserManagerImpl::UserExistsInList(const std::string& user_id) const {
+  PrefService* local_state = g_browser_process->local_state();
+  const ListValue* user_list = local_state->GetList(kRegularUsers);
+  for (size_t i = 0; i < user_list->GetSize(); ++i) {
+    std::string email;
+    if (user_list->GetString(i, &email) && (user_id == email))
+      return true;
+  }
+  return false;
+}
+
 User* UserManagerImpl::FindUserInListAndModify(const std::string& user_id) {
   UserList& users = GetUsersAndModify();
   for (UserList::iterator it = users.begin(); it != users.end(); ++it) {