Address races in UserImageManagerImpl

UserImageManagerImpl uses background threads and delayed tasks to load
images, save images and schedule downloads. There are races in this
implementation so that an earlier operation can complete after a later
operation, clobbering its result. Delayed tasks using base::Unretained()
may cause crashes on shutdown if the unretained object no longer exists.
Some of the races were partially worked around with global state variables
that do not account for the fact that operations for multiple users may be
going on in parallel.

This CL is an almost complete rewrite of the UserImageManagerImpl that
addresses the races and thread safety issues. Pending tasks are
encapsulated by a Job class with at most one Job running per user at a
time.

Migration of old user images is simplified by this CL: There used to be a
delay between user login and migration, postponing the I/O-heavy operation
until after login is complete. However, the implementation was wrong and
migration of PNG images was actually carried out immediately. Only for
users who had selected one of the default images was the migration
delayed, which is unnecessary as it does not involve any file I/O or
transcoding. Since there have been no complaints about the performance
impact of migration during login, this CL removes the delay altogether.

The CL also updates the UserImageManagerImpl browser test, re-enabling the
previously flaky NonJPEGImageFromFile.

BUG=152957,152959,257009
TEST=Updated browser tests; manual tests with chromeos=1 and VM

Review URL: https://codereview.chromium.org/69863006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@235645 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/login/fake_user_manager.cc

@@ -151,6 +151,10 @@ const User* FakeUserManager::FindUser(const std::string& email) const {
   return NULL;
 }
 
+User* FakeUserManager::FindUserAndModify(const std::string& email) {
+  return NULL;
+}
+
 const User* FakeUserManager::GetLoggedInUser() const {
   return NULL;
 }

chrome/browser/chromeos/login/fake_user_manager.h

@@ -65,6 +65,7 @@ class FakeUserManager : public UserManager {
   virtual void RemoveUserFromList(const std::string& email) OVERRIDE {}
   virtual bool IsKnownUser(const std::string& email) const OVERRIDE;
   virtual const User* FindUser(const std::string& email) const OVERRIDE;
+  virtual User* FindUserAndModify(const std::string& email) OVERRIDE;
   virtual const User* GetLoggedInUser() const OVERRIDE;
   virtual User* GetLoggedInUser() OVERRIDE;
   virtual const User* GetPrimaryUser() const OVERRIDE;

chrome/browser/chromeos/login/mock_user_manager.h

@@ -38,6 +38,7 @@ class MockUserManager : public UserManager {
   MOCK_METHOD1(RemoveUserFromList, void(const std::string&));
   MOCK_CONST_METHOD1(IsKnownUser, bool(const std::string&));
   MOCK_CONST_METHOD1(FindUser, const User*(const std::string&));
+  MOCK_METHOD1(FindUserAndModify, User*(const std::string&));
   MOCK_METHOD2(SaveUserOAuthStatus, void(const std::string&,
                                          User::OAuthTokenStatus));
   MOCK_METHOD2(SaveUserDisplayName, void(const std::string&,

chrome/browser/chromeos/login/user_image_loader.cc

@@ -66,7 +66,9 @@ void UserImageLoader::ReadAndDecodeImage(const std::string& filepath,
   DCHECK(background_task_runner_->RunsTasksOnCurrentThread());
 
   scoped_ptr<std::string> data(new std::string);
-  base::ReadFileToString(base::FilePath(filepath), data.get());
+  const bool success =
+      base::ReadFileToString(base::FilePath(filepath), data.get());
+  DCHECK(success);
 
   DecodeImage(data.Pass(), image_info);
 }

chrome/browser/chromeos/login/user_image_manager.h

@@ -35,35 +35,35 @@ class UserImageManager {
   // Loads user image data from Local State.
   virtual void LoadUserImages(const UserList& users) = 0;
 
-  // Indicates that a user with the given |email| has just logged in.
-  virtual void UserLoggedIn(const std::string& email,
+  // Indicates that a user with the given |user_id| has just logged in.
+  virtual void UserLoggedIn(const std::string& user_id,
                             bool user_is_new,
                             bool user_is_local) = 0;
 
   // Sets user image to the default image with index |image_index|, sends
   // LOGIN_USER_IMAGE_CHANGED notification and updates Local State.
-  virtual void SaveUserDefaultImageIndex(const std::string& username,
+  virtual void SaveUserDefaultImageIndex(const std::string& user_id,
                                          int image_index) = 0;
 
   // Saves image to file, sends LOGIN_USER_IMAGE_CHANGED notification and
   // updates Local State.
-  virtual void SaveUserImage(const std::string& username,
+  virtual void SaveUserImage(const std::string& user_id,
                              const UserImage& user_image) = 0;
 
   // Tries to load user image from disk; if successful, sets it for the user,
   // sends LOGIN_USER_IMAGE_CHANGED notification and updates Local State.
-  virtual void SaveUserImageFromFile(const std::string& username,
+  virtual void SaveUserImageFromFile(const std::string& user_id,
                                      const base::FilePath& path) = 0;
 
-  // Sets profile image as user image for |username|, sends
+  // Sets profile image as user image for |user_id|, sends
   // LOGIN_USER_IMAGE_CHANGED notification and updates Local State. If the user
   // is not logged-in or the last |DownloadProfileImage| call has failed, a
   // default grey avatar will be used until the user logs in and profile image
   // is downloaded successfully.
-  virtual void SaveUserImageFromProfileImage(const std::string& username) = 0;
+  virtual void SaveUserImageFromProfileImage(const std::string& user_id) = 0;
 
   // Deletes user image and the corresponding image file.
-  virtual void DeleteUserImage(const std::string& username) = 0;
+  virtual void DeleteUserImage(const std::string& user_id) = 0;
 
   // Starts downloading the profile image for the logged-in user.
   // If user's image index is |kProfileImageIndex|, newly downloaded image

chrome/browser/chromeos/login/user_image_manager_browsertest.cc

@@ -5,24 +5,25 @@
 #include <string>
 
 #include "base/command_line.h"
+#include "base/compiler_specific.h"
 #include "base/file_util.h"
 #include "base/memory/ref_counted_memory.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/path_service.h"
+#include "base/prefs/pref_change_registrar.h"
 #include "base/prefs/pref_service.h"
 #include "base/prefs/scoped_user_pref_update.h"
+#include "base/run_loop.h"
 #include "base/values.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/login/default_user_images.h"
 #include "chrome/browser/chromeos/login/mock_user_manager.h"
-#include "chrome/browser/chromeos/login/user_image_manager_impl.h"
 #include "chrome/browser/chromeos/login/user_manager.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chromeos/chromeos_switches.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/test/test_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -34,20 +35,14 @@ namespace chromeos {
 const char kTestUser1[] = "test-user@example.com";
 const char kTestUser2[] = "test-user2@example.com";
 
-class UserImageManagerTest : public InProcessBrowserTest,
-                             public content::NotificationObserver,
-                             public UserManager::Observer {
+class UserImageManagerTest : public InProcessBrowserTest {
  protected:
   UserImageManagerTest() {
   }
 
   // InProcessBrowserTest overrides:
   virtual void SetUpOnMainThread() OVERRIDE {
-    UserManager::Get()->AddObserver(this);
-    user_image_manager_ = UserManager::Get()->GetUserImageManager();
     local_state_ = g_browser_process->local_state();
-    // No migration delay for testing.
-    UserImageManagerImpl::user_image_migration_delay_sec = 0;
   }
 
   virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
@@ -55,21 +50,6 @@ class UserImageManagerTest : public InProcessBrowserTest,
     command_line->AppendSwitchASCII(switches::kLoginProfile, "user");
   }
 
-  // content::NotificationObserver overrides:
-  virtual void Observe(int type,
-                       const content::NotificationSource& source,
-                       const content::NotificationDetails& details) OVERRIDE {
-    DCHECK(type == chrome::NOTIFICATION_LOGIN_USER_IMAGE_CHANGED);
-    registrar_.Remove(this, chrome::NOTIFICATION_LOGIN_USER_IMAGE_CHANGED,
-                      content::NotificationService::AllSources());
-    base::MessageLoopForUI::current()->Quit();
-  }
-
-  // UserManager::Observer overrides:
-  virtual void LocalStateChanged(UserManager* user_manager) OVERRIDE {
-    base::MessageLoopForUI::current()->Quit();
-  }
-
   // Adds given user to Local State, if not there.
   void AddUser(const std::string& username) {
     ListPrefUpdate users_pref(local_state_, "LoggedInUsers");
@@ -81,12 +61,6 @@ class UserImageManagerTest : public InProcessBrowserTest,
     UserManager::Get()->UserLoggedIn(username, username, false);
   }
 
-  // Subscribes for image change notification.
-  void ExpectImageChange() {
-    registrar_.Add(this, chrome::NOTIFICATION_LOGIN_USER_IMAGE_CHANGED,
-                   content::NotificationService::AllSources());
-  }
-
   // Stores old (pre-migration) user image info.
   void SetOldUserImageInfo(const std::string& username,
                            int image_index,
@@ -173,9 +147,7 @@ class UserImageManagerTest : public InProcessBrowserTest,
     return user_data_dir.Append(username).AddExtension(extension);
   }
 
-  UserImageManager* user_image_manager_;
   PrefService* local_state_;
-  content::NotificationRegistrar registrar_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(UserImageManagerTest);
@@ -192,8 +164,6 @@ IN_PROC_BROWSER_TEST_F(UserImageManagerTest, DefaultUserImagePreserved) {
   // Old info preserved.
   ExpectOldUserImageInfo(kTestUser1, kFirstDefaultImageIndex, base::FilePath());
   LogIn(kTestUser1);
-  // Wait for migration.
-  content::RunMessageLoop();
   // Image info is migrated now.
   ExpectNewUserImageInfo(kTestUser1, kFirstDefaultImageIndex, base::FilePath());
 }
@@ -213,8 +183,6 @@ IN_PROC_BROWSER_TEST_F(UserImageManagerTest, OtherUsersUnaffected) {
   ExpectOldUserImageInfo(kTestUser2, kFirstDefaultImageIndex + 1,
                          base::FilePath());
   LogIn(kTestUser1);
-  // Wait for migration.
-  content::RunMessageLoop();
   // Image info is migrated for the first user and unaffected for the rest.
   ExpectNewUserImageInfo(kTestUser1, kFirstDefaultImageIndex, base::FilePath());
   ExpectOldUserImageInfo(kTestUser2, kFirstDefaultImageIndex + 1,
@@ -235,9 +203,16 @@ IN_PROC_BROWSER_TEST_F(UserImageManagerTest, PRE_NonJPEGImageFromFile) {
                          GetUserImagePath(kTestUser1, "png"));
   const User* user = UserManager::Get()->FindUser(kTestUser1);
   EXPECT_TRUE(user->image_is_stub());
+
+  base::RunLoop run_loop;
+  PrefChangeRegistrar pref_change_registrar_;
+  pref_change_registrar_.Init(local_state_);
+  pref_change_registrar_.Add("UserImages", run_loop.QuitClosure());
   LogIn(kTestUser1);
+
   // Wait for migration.
-  content::RunMessageLoop();
+  run_loop.Run();
+
   // Image info is migrated and the image is converted to JPG.
   ExpectNewUserImageInfo(kTestUser1, User::kExternalImageIndex,
                          GetUserImagePath(kTestUser1, "jpg"));
@@ -250,15 +225,17 @@ IN_PROC_BROWSER_TEST_F(UserImageManagerTest, PRE_NonJPEGImageFromFile) {
   EXPECT_EQ(saved_image.height(), user->image().height());
 }
 
-// http://crbug.com/257009.
-IN_PROC_BROWSER_TEST_F(UserImageManagerTest, DISABLED_NonJPEGImageFromFile) {
-  ExpectImageChange();
+IN_PROC_BROWSER_TEST_F(UserImageManagerTest, NonJPEGImageFromFile) {
   UserManager::Get()->GetUsers();  // Load users.
-  // Wait for image load.
-  content::RunMessageLoop();
-  // Now the migrated image is used.
   const User* user = UserManager::Get()->FindUser(kTestUser1);
   ASSERT_TRUE(user);
+  // Wait for image load.
+  if (user->image_index() == User::kInvalidImageIndex) {
+    content::WindowedNotificationObserver(
+        chrome::NOTIFICATION_LOGIN_USER_IMAGE_CHANGED,
+        content::NotificationService::AllSources()).Wait();
+  }
+  // Now the migrated image is used.
   EXPECT_TRUE(user->image_is_safe_format());
   // Check image dimensions. Images can't be compared since JPEG is lossy.
   const gfx::ImageSkia& saved_image = GetDefaultImage(kFirstDefaultImageIndex);

chrome/browser/chromeos/login/user_manager.h

@@ -174,6 +174,11 @@ class UserManager {
   // list or currently logged in as ephemeral. Returns |NULL| otherwise.
   virtual const User* FindUser(const std::string& user_id) const = 0;
 
+  // Returns the user with the given user id if found in the persistent
+  // list or currently logged in as ephemeral. Returns |NULL| otherwise.
+  // Same as FindUser but returns non-const pointer to User object.
+  virtual User* FindUserAndModify(const std::string& user_id) = 0;
+
   // Returns the logged-in user.
   // TODO(nkostylev): Deprecate this call, move clients to GetActiveUser().
   // http://crbug.com/230852

chrome/browser/chromeos/login/user_manager_impl.cc

@@ -545,6 +545,13 @@ const User* UserManagerImpl::FindUser(const std::string& user_id) const {
   return FindUserInList(user_id);
 }
 
+User* UserManagerImpl::FindUserAndModify(const std::string& user_id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (active_user_ && active_user_->email() == user_id)
+    return active_user_;
+  return FindUserInListAndModify(user_id);
+}
+
 const User* UserManagerImpl::GetLoggedInUser() const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   return active_user_;
@@ -1168,13 +1175,6 @@ UserList& UserManagerImpl::GetUsersAndModify() {
   return users_;
 }
 
-User* UserManagerImpl::FindUserAndModify(const std::string& user_id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  if (active_user_ && active_user_->email() == user_id)
-    return active_user_;
-  return FindUserInListAndModify(user_id);
-}
-
 const User* UserManagerImpl::FindUserInList(const std::string& user_id) const {
   const UserList& users = GetUsers();
   for (UserList::const_iterator it = users.begin(); it != users.end(); ++it) {

chrome/browser/chromeos/login/user_manager_impl.h

@@ -75,6 +75,7 @@ class UserManagerImpl
   virtual void RemoveUserFromList(const std::string& user_id) OVERRIDE;
   virtual bool IsKnownUser(const std::string& user_id) const OVERRIDE;
   virtual const User* FindUser(const std::string& user_id) const OVERRIDE;
+  virtual User* FindUserAndModify(const std::string& user_id) OVERRIDE;
   virtual const User* GetLoggedInUser() const OVERRIDE;
   virtual User* GetLoggedInUser() OVERRIDE;
   virtual const User* GetActiveUser() const OVERRIDE;
@@ -178,11 +179,6 @@ class UserManagerImpl
   // Same as GetUsers but used if you need to modify User from that list.
   UserList& GetUsersAndModify();
 
-  // Returns the user with the given email address if found in the persistent
-  // list or currently logged in as ephemeral. Returns |NULL| otherwise.
-  // Same as FindUser but returns non-const pointer to User object.
-  User* FindUserAndModify(const std::string& user_id);
-
   // Returns the user with the given email address if found in the persistent
   // list. Returns |NULL| otherwise.
   const User* FindUserInList(const std::string& user_id) const;

chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc

@@ -12,13 +12,16 @@
 #include "ash/test/display_manager_test_api.h"
 #include "base/command_line.h"
 #include "base/file_util.h"
+#include "base/files/file_path.h"
 #include "base/message_loop/message_loop.h"
+#include "base/path_service.h"
 #include "base/prefs/scoped_user_pref_update.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/login/user.h"
 #include "chrome/browser/chromeos/login/user_manager.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/testing_browser_process.h"
@@ -110,8 +113,7 @@ class WallpaperManagerBrowserTest : public InProcessBrowserTest,
   }
 
   // Saves bitmap |resource_id| to disk.
-  void SaveUserWallpaperData(const std::string& username,
-                             const base::FilePath& wallpaper_path,
+  void SaveUserWallpaperData(const base::FilePath& wallpaper_path,
                              int resource_id) {
     scoped_refptr<base::RefCountedStaticMemory> image_data(
         ResourceBundle::GetSharedInstance().LoadDataResourceBytesForScale(
@@ -154,11 +156,9 @@ IN_PROC_BROWSER_TEST_F(WallpaperManagerBrowserTest,
 
   // Saves the small/large resolution wallpapers to small/large custom
   // wallpaper paths.
-  SaveUserWallpaperData(kTestUser1,
-                        small_wallpaper_path,
+  SaveUserWallpaperData(small_wallpaper_path,
                         kSmallWallpaperResourceId);
-  SaveUserWallpaperData(kTestUser1,
-                        large_wallpaper_path,
+  SaveUserWallpaperData(large_wallpaper_path,
                         kLargeWallpaperResourceId);
 
   std::string relative_path = base::FilePath(kTestUser1Hash).Append(id).value();
@@ -239,8 +239,7 @@ IN_PROC_BROWSER_TEST_F(WallpaperManagerBrowserTest,
       kSmallWallpaperSubDir,
       kTestUser1Hash,
       id);
-  SaveUserWallpaperData(kTestUser1,
-                        small_wallpaper_path,
+  SaveUserWallpaperData(small_wallpaper_path,
                         kSmallWallpaperResourceId);
 
   std::string relative_path = base::FilePath(kTestUser1Hash).Append(id).value();
@@ -282,6 +281,10 @@ IN_PROC_BROWSER_TEST_F(WallpaperManagerBrowserTest,
       User::DEFAULT,
       base::Time::Now().LocalMidnight()
   };
+  base::FilePath user_data_dir;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_USER_DATA, &user_data_dir));
+  SaveUserWallpaperData(user_data_dir.Append("123"),
+                        kLargeWallpaperResourceId);
   WallpaperManager::Get()->SetUserWallpaperInfo(kTestUser1, info, true);
 }
 
@@ -357,6 +360,10 @@ IN_PROC_BROWSER_TEST_F(WallpaperManagerBrowserTestNoAnimation,
       User::DEFAULT,
       base::Time::Now().LocalMidnight()
   };
+  base::FilePath user_data_dir;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_USER_DATA, &user_data_dir));
+  SaveUserWallpaperData(user_data_dir.Append("123"),
+                        kLargeWallpaperResourceId);
   WallpaperManager::Get()->SetUserWallpaperInfo(kTestUser1, info, true);
 }