Address races in UserImageManagerImpl

chrome/browser/chromeos/login/user_image_manager_impl.cc

Index: chrome/browser/chromeos/login/user_image_manager_impl.cc
diff --git a/chrome/browser/chromeos/login/user_image_manager_impl.cc b/chrome/browser/chromeos/login/user_image_manager_impl.cc
index 06cc80c05423f499f5ed923033cff6168b79bd4e..36a26437877dbba153382b4fea3eda59f9b0bc90 100644
--- a/chrome/browser/chromeos/login/user_image_manager_impl.cc
+++ b/chrome/browser/chromeos/login/user_image_manager_impl.cc
@@ -17,10 +17,9 @@
 #include "base/prefs/scoped_user_pref_update.h"
 #include "base/rand_util.h"
 #include "base/sequenced_task_runner.h"
+#include "base/task_runner_util.h"
 #include "base/threading/sequenced_worker_pool.h"
-#include "base/threading/worker_pool.h"
 #include "base/time/time.h"
-#include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/login/default_user_images.h"
@@ -35,22 +34,19 @@
 #include "chromeos/chromeos_switches.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
-#include "content/public/common/url_constants.h"
 #include "ui/base/webui/web_ui_util.h"
 #include "ui/gfx/image/image_skia.h"
 
-using content::BrowserThread;
-
 namespace chromeos {
 
 namespace {
 
-// A dictionary that maps usernames to old user image data with images stored in
+// A dictionary that maps user_ids to old user image data with images stored in
 // PNG format. Deprecated.
 // TODO(ivankr): remove this const char after migration is gone.
 const char kUserImages[] = "UserImages";
 
-// A dictionary that maps usernames to user image data with images stored in
+// A dictionary that maps user_ids to user image data with images stored in
 // JPEG format.
 const char kUserImageProperties[] = "user_image_info";
 
@@ -59,9 +55,6 @@ const char kImagePathNodeName[] = "path";
 const char kImageIndexNodeName[] = "index";
 const char kImageURLNodeName[] = "url";
 
-// Delay betweeen user login and user image migration.
-const int kUserImageMigrationDelaySec = 50;

[bartfab (slow), 2013/11/13 23:12:09]

I removed this because it was not working correctly anyway:
* If the user had chosen a default image, the migration (a trivial operation)
would be delayed by 50 seconds.
* If the user had chosen a custom image, the migration (an expensive operation
involving re-encoding to JPEG) would be carried out immediately.

Since the more expensive of the two operations was done immediately and nobody
seems to have noticed or complained in the past, I removed the delay altogether.

-
 // Delay betweeen user login and attempt to update user's profile data.
 const int kProfileDataDownloadDelaySec = 10;
 
@@ -104,7 +97,7 @@ const char kProfileDownloadReasonScheduled[] = "Scheduled";
 // Time histogram suffix for a profile image download retry.
 const char kProfileDownloadReasonRetry[] = "Retry";
 
-// Add a histogram showing the time it takes to download a profile image.
+// Add a histogram showing the time it takes to download profile image.
 // Separate histograms are reported for each download |reason| and |result|.
 void AddProfileImageTimeHistogram(ProfileDownloadResult result,
                                   const std::string& download_reason,
@@ -143,18 +136,6 @@ void AddProfileImageTimeHistogram(ProfileDownloadResult result,
   DVLOG(1) << "Profile image download time: " << time_delta.InSecondsF();
 }
 
-// Deletes image file.
-void DeleteImageFile(const std::string& image_path) {
-  if (image_path.empty())
-    return;
-  base::FilePath fp(image_path);
-  BrowserThread::PostTask(
-      BrowserThread::FILE,
-      FROM_HERE,
-      base::Bind(base::IgnoreResult(&base::DeleteFile),
-                 fp, /* recursive= */  false));
-}
-
 // Converts |image_index| to UMA histogram value.
 int ImageIndexToHistogramIndex(int image_index) {
   switch (image_index) {
@@ -168,11 +149,31 @@ int ImageIndexToHistogramIndex(int image_index) {
   }
 }
 
-}  // namespace
+bool SaveImage(const UserImage& user_image, const base::FilePath& image_path) {
+  UserImage safe_image;
+  const UserImage::RawImage* encoded_image = NULL;
+  if (!user_image.is_safe_format()) {
+    safe_image = UserImage::CreateAndEncode(user_image.image());
+    encoded_image = &safe_image.raw_image();
+    UMA_HISTOGRAM_MEMORY_KB("UserImage.RecodedJpegSize", encoded_image->size());
+  } else if (user_image.has_raw_image()) {
+    encoded_image = &user_image.raw_image();
+  } else {
+    NOTREACHED() << "Raw image missing.";
+    return false;
+  }
 
-// static
-int UserImageManagerImpl::user_image_migration_delay_sec =
-    kUserImageMigrationDelaySec;
+  if (file_util::WriteFile(image_path,
+                           reinterpret_cast<const char*>(&(*encoded_image)[0]),
+                           encoded_image->size()) == -1) {
+    LOG(ERROR) << "Failed to save image to file.";
+    return false;
+  }
+
+  return true;
+}
+
+}  // namespace
 
 // static
 void UserImageManager::RegisterPrefs(PrefRegistrySimple* registry) {
@@ -180,22 +181,259 @@ void UserImageManager::RegisterPrefs(PrefRegistrySimple* registry) {
   registry->RegisterDictionaryPref(kUserImageProperties);
 }
 
+// Every image load or update is encapsulated by a Job. The Job is allowed to
+// perform tasks on background threads or in helper processes but:
+// * Changes to User objects and local state as well as any calls to the
+//   |parent_| must be performed on the thread that the Job is created on only.
+// * File writes and deletions must be performed via the |parent_|'s
+//   |background_task_runner_| only.
+//
+// Only one of the Load*() and Set*() methods may be called per Job.
+class UserImageManagerImpl::Job {
+ public:
+  // The |Job| will update the |user| object for |user_id|.
+  Job(UserImageManagerImpl* parent, const std::string& user_id);
+  ~Job();
+
+  // Loads the image at |image_path| or one of the default images, depending on
+  // |image_index|, and updates the |user| object for |user_id_| with the new
+  // image.
+  void LoadImage(base::FilePath image_path,
+                 const int image_index,
+                 const GURL& image_url);
+
+  // Sets the user image for |user_id_| in local state to the default image
+  // indicated by |default_image_index|. Also updates the |user| object for
+  // |user_id_| with the new image.
+  void SetToDefaultImage(int default_image_index);
+
+  // Saves the |user_image| to disk and sets the user image for |user_id_| in
+  // local state to that image. Also updates the |user| object for |user_id_|
+  // with the new image.
+  void SetToImage(int image_index,
+                  const UserImage& user_image);
+
+  // Loads the the image at |path|, transcodes it to JPEG format, saves the
+  // image to disk and sets the user image for |user_id_| in local state to that
+  // image. If  |resize| is true, the image is cropped and resized before
+  // transcoding. Also  updates the |user| object for |user_id_| with the new
+  // image.
+  void SetToPath(const base::FilePath& path,
+                 int image_index,
+                 const GURL& image_url,
+                 bool resize);
+
+ private:
+  // Called back after an image has been loaded from disk.
+  void OnLoadImageDone(bool save, const UserImage& user_image);
+
+  // Updates the |user| object for |user_id_| with |user_image_|.
+  void UpdateUser();
+
+  // Saves |user_image_| to disk in JPEG format. Local state will be updated
+  // when a callback indicates that the save has been successful.
+  void SaveImageAndUpdateLocalState();
+
+  // Called back after the |user_image_| has been saved to disk. If |success| is
+  // true sets the user image for |user_id_| in local state to that image.
+  void OnSaveImageDone(bool success);
+
+  // Updates the user image for |user_id_| in local state, setting it to
+  // one of the default images or the saved |user_image_|, depending on
+  // |image_index_|.
+  void UpdateLocalState();
+
+  // Notifies the |parent_| that the Job is done.
+  void NotifyJobDone();
+
+  UserImageManagerImpl* parent_;
+  const std::string user_id_;
+
+  // Whether one of the Load*() or Set*() methods has been run already.
+  bool run_;
+
+  int image_index_;
+  GURL image_url_;
+  base::FilePath image_path_;
+
+  UserImage user_image_;
+
+  base::WeakPtrFactory<Job> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(Job);
+};
+
+UserImageManagerImpl::Job::Job(UserImageManagerImpl* parent,
+                               const std::string& user_id)
+    : parent_(parent),
+      user_id_(user_id),
+      run_(false),
+      weak_factory_(this) {
+}
+
+UserImageManagerImpl::Job::~Job() {
+}
+
+void UserImageManagerImpl::Job::LoadImage(base::FilePath image_path,
+                                          const int image_index,
+                                          const GURL& image_url) {
+  DCHECK(!run_);
+  run_ = true;
+
+  image_index_ = image_index;
+  image_url_ = image_url;
+  image_path_ = image_path;
+
+  if (image_index_ >= 0 && image_index_ < kDefaultImagesCount) {
+    // Load one of the default images. This happens synchronously.
+    user_image_ = UserImage(GetDefaultImage(image_index_));
+    UpdateUser();
+    NotifyJobDone();
+  } else if (image_index_ == User::kExternalImageIndex ||
+             image_index_ == User::kProfileImageIndex) {
+    // Load the user image from a file referenced by |image_path|. This happens
+    // asynchronously. The JPEG image loader can be used here because
+    // LoadImage() is called only for users whose user image has previously
+    // been set by one of the Set*() methods, which transcode to JPEG format.
+    parent_->image_loader_->Start(image_path_.value(),

[Nikita (slow), 2013/11/14 15:17:13]

DCHECK that image_path is not empty.

[bartfab (slow), 2013/11/14 15:55:10]

There actually is a DCHECK for this in the code that calls LoadImage(), but I
added one here as well now as requested.

+                                  0,
+                                  base::Bind(&Job::OnLoadImageDone,
+                                             weak_factory_.GetWeakPtr(),
+                                             false));
+  } else {
+    NOTREACHED();
+    NotifyJobDone();
+  }
+}
+
+void UserImageManagerImpl::Job::SetToDefaultImage(int default_image_index) {
+  DCHECK(!run_);
+  run_ = true;
+
+  DCHECK_LE(0, default_image_index);
+  DCHECK_GT(kDefaultImagesCount, default_image_index);
+
+  image_index_ = default_image_index;
+  user_image_ = UserImage(GetDefaultImage(image_index_));
+
+  UpdateUser();
+  UpdateLocalState();
+  NotifyJobDone();
+}
+
+void UserImageManagerImpl::Job::SetToImage(int image_index,
+                                           const UserImage& user_image) {
+  DCHECK(!run_);
+  run_ = true;
+
+  DCHECK(image_index == User::kExternalImageIndex ||
+         image_index == User::kProfileImageIndex);
+
+  image_index_ = image_index;
+  user_image_ = user_image;
+
+  UpdateUser();
+  SaveImageAndUpdateLocalState();
+}
+
+void UserImageManagerImpl::Job::SetToPath(const base::FilePath& path,
+                                          int image_index,
+                                          const GURL& image_url,
+                                          bool resize) {
+  DCHECK(!run_);

[Nikita (slow), 2013/11/14 15:17:13]

DCHECK path.

[bartfab (slow), 2013/11/14 15:55:10]

Done.

+  run_ = true;
+
+  image_index_ = image_index;
+  image_url_ = image_url;
+
+  parent_->unsafe_image_loader_->Start(path.value(),
+                                       resize ? login::kMaxUserImageSize : 0,
+                                       base::Bind(&Job::OnLoadImageDone,
+                                                  weak_factory_.GetWeakPtr(),
+                                                  true));
+}
+
+void UserImageManagerImpl::Job::OnLoadImageDone(bool save,
+                                                const UserImage& user_image) {
+  user_image_ = user_image;
+  UpdateUser();
+  if (save)
+    SaveImageAndUpdateLocalState();
+  else
+    NotifyJobDone();
+}
+
+void UserImageManagerImpl::Job::UpdateUser() {
+  User* user = const_cast<User*>(UserManager::Get()->FindUser(user_id_));

[Nikita (slow), 2013/11/14 15:17:13]

How can const_cast be avoided?
Declare this class as a friend and use private method? 
Or add new public API to UserManager instead?

[bartfab (slow), 2013/11/14 15:55:10]

I actually inherited this case from the original implementation but I agree that
it is bad. I promoted UserManagerImpl::FindUserAndModify() to a public method
now so that it is possible to get a pointer to a mutable User without casting.

+  if (!user)
+    return;
+
+  if (!user_image_.image().isNull())
+    user->SetImage(user_image_, image_index_);
+  else
+    user->SetStubImage(image_index_, false);
+  user->SetImageURL(image_url_);
+
+  parent_->OnJobChangedUserImage(user);
+}
+
+void UserImageManagerImpl::Job::SaveImageAndUpdateLocalState() {
+  base::FilePath user_data_dir;
+  PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
+  image_path_ = user_data_dir.Append(user_id_ + kSafeImagePathExtension);
+
+  base::PostTaskAndReplyWithResult(
+      parent_->background_task_runner_,
+      FROM_HERE,
+      base::Bind(&SaveImage, user_image_, image_path_),
+      base::Bind(&Job::OnSaveImageDone, weak_factory_.GetWeakPtr()));
+}
+
+void UserImageManagerImpl::Job::OnSaveImageDone(bool success) {
+  if (success)
+    UpdateLocalState();
+  NotifyJobDone();
+}
+
+void UserImageManagerImpl::Job::UpdateLocalState() {
+  // Ignore if data stored or cached outside the user's cryptohome is to be
+  // treated as ephemeral.
+  if (UserManager::Get()->IsUserNonCryptohomeDataEphemeral(user_id_))
+    return;
+
+  scoped_ptr<base::DictionaryValue> entry(new base::DictionaryValue);
+  entry->Set(kImagePathNodeName, new base::StringValue(image_path_.value()));
+  entry->Set(kImageIndexNodeName, new base::FundamentalValue(image_index_));
+  if (!image_url_.is_empty())
+    entry->Set(kImageURLNodeName, new StringValue(image_url_.spec()));
+  DictionaryPrefUpdate update(g_browser_process->local_state(),
+                              kUserImageProperties);
+  update->SetWithoutPathExpansion(user_id_, entry.release());
+
+  UserManager::Get()->NotifyLocalStateChanged();
+}
+
+void UserImageManagerImpl::Job::NotifyJobDone() {
+  parent_->OnJobDone(user_id_);
+  // |parent_| drops ownership of |this| in OnJobDone(), requiring the Job to
+  // destroy itself.
+  delete this;

[Nikita (slow), 2013/11/14 15:17:13]

Any reasons why me might want to use DeleteSoon() here instead?

[bartfab (slow), 2013/11/14 15:55:10]

I was sure I had a good reason not to use DeleteSoon()... but thinking about it
again, I really cannot see why not to use it. Done.

+}
+
 UserImageManagerImpl::UserImageManagerImpl()
-    : last_image_set_async_(false),
-      downloaded_profile_image_data_url_(content::kAboutBlankURL),
-      downloading_profile_image_(false),
-      migrate_current_user_on_load_(false) {
-  base::SequencedWorkerPool* blocking_pool = BrowserThread::GetBlockingPool();
-  // Background task runner on which file I/O, image decoding and resizing are
-  // done.
-  scoped_refptr<base::SequencedTaskRunner> task_runner =
+    : downloading_profile_image_(false),
+      profile_image_requested_(false),
+      weak_factory_(this) {
+  base::SequencedWorkerPool* blocking_pool =
+      content::BrowserThread::GetBlockingPool();
+  background_task_runner_ =
       blocking_pool->GetSequencedTaskRunnerWithShutdownBehavior(
           blocking_pool->GetSequenceToken(),
           base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
   image_loader_ = new UserImageLoader(ImageDecoder::ROBUST_JPEG_CODEC,
-                                      task_runner);
+                                      background_task_runner_);
   unsafe_image_loader_ = new UserImageLoader(ImageDecoder::DEFAULT_CODEC,
-                                             task_runner);
+                                             background_task_runner_);
 }
 
 UserImageManagerImpl::~UserImageManagerImpl() {
@@ -212,215 +450,186 @@ void UserImageManagerImpl::LoadUserImages(const UserList& users) {
 
   for (UserList::const_iterator it = users.begin(); it != users.end(); ++it) {
     User* user = *it;
-    const base::DictionaryValue* image_properties = NULL;
-    bool needs_migration = false;  // |true| if user has image in old format.
-    bool safe_source = false;      // |true| if image loaded from safe source.
+    const std::string& user_id = user->email();
+    bool needs_migration = false;
 
+    // If entries are found in both |prefs_images_unsafe| and |prefs_images|,
+    // |prefs_images| is honored for now but |prefs_images_unsafe| will be
+    // migrated, overwriting the |prefs_images| entry, when the user logs in.
+    const base::DictionaryValue* image_properties = NULL;
     if (prefs_images_unsafe) {
       needs_migration = prefs_images_unsafe->GetDictionaryWithoutPathExpansion(
-          user->email(), &image_properties);
+          user_id, &image_properties);
+      if (needs_migration)
+        users_to_migrate_.insert(user_id);
     }
     if (prefs_images) {
-      safe_source = prefs_images->GetDictionaryWithoutPathExpansion(
-          user->email(), &image_properties);
+      prefs_images->GetDictionaryWithoutPathExpansion(user_id,

[Nikita (slow), 2013/11/14 15:17:13]

Can you please describe how migration implementation has been changed in this
CL?

[bartfab (slow), 2013/11/14 15:55:10]

Added to CL description.

+                                                      &image_properties);
     }
 
-    if (needs_migration)
-      users_to_migrate_.insert(user->email());
-
     if (!image_properties) {
-      SetInitialUserImage(user->email());
-    } else {
-      int image_index = User::kInvalidImageIndex;
-      image_properties->GetInteger(kImageIndexNodeName, &image_index);
-
-      if (image_index >= 0 && image_index < kDefaultImagesCount) {
-        user->SetImage(UserImage(GetDefaultImage(image_index)),
-                       image_index);
-      } else if (image_index == User::kExternalImageIndex ||
-                 image_index == User::kProfileImageIndex) {
-        std::string image_path;
-        image_properties->GetString(kImagePathNodeName, &image_path);
-        // Path may be empty for profile images (meaning that the image
-        // hasn't been downloaded for the first time yet, in which case a
-        // download will be scheduled for |kProfileDataDownloadDelayMs|
-        // after user logs in).
-        DCHECK(!image_path.empty() || image_index == User::kProfileImageIndex);
-        std::string image_url;
-        image_properties->GetString(kImageURLNodeName, &image_url);
-        GURL image_gurl(image_url);
-        // Until image has been loaded, use the stub image (gray avatar).
-        user->SetStubImage(image_index, true);
-        user->SetImageURL(image_gurl);
-        if (!image_path.empty()) {
-          if (needs_migration) {
-            // Non-JPG image will be migrated once user logs in.
-            // Stub image will be used for now. Continue with other users.
-            continue;
-          }
-          DCHECK(safe_source);
-          if (!safe_source)
-            continue;
-
-          // Load user image asynchronously - at this point we are able to use
-          // JPEG image loaded since image comes from safe pref source
-          // i.e. converted to JPEG.
-          image_loader_->Start(
-              image_path, 0  /* no resize */,
-              base::Bind(&UserImageManagerImpl::SetUserImage,
-                         base::Unretained(this),
-                         user->email(), image_index, image_gurl));
-        }
-      } else {
-        NOTREACHED();
-      }
+      SetInitialUserImage(user_id);
+      continue;
     }
+
+    int image_index = User::kInvalidImageIndex;
+    image_properties->GetInteger(kImageIndexNodeName, &image_index);
+    if (image_index >= 0 && image_index < kDefaultImagesCount) {
+      user->SetImage(UserImage(GetDefaultImage(image_index)),
+                     image_index);
+      continue;
+    }
+
+    if (image_index != User::kExternalImageIndex &&
+        image_index != User::kProfileImageIndex) {
+      NOTREACHED();
+      continue;
+    }
+
+    std::string image_url_string;
+    image_properties->GetString(kImageURLNodeName, &image_url_string);
+    GURL image_url(image_url_string);
+    std::string image_path;
+    image_properties->GetString(kImagePathNodeName, &image_path);
+
+    user->SetImageURL(image_url);
+    DCHECK(!image_path.empty() || image_index == User::kProfileImageIndex);
+    if (image_path.empty() || needs_migration) {
+      // Use a stub image (gray avatar) if either of the following is true:
+      // * The profile image is to be used but has not been downloaded yet. The
+      //   profile image will be downloaded after login.
+      // * The image needs migration. Migration will be performed after login.
+      user->SetStubImage(image_index, true);
+      continue;
+    }
+
+    linked_ptr<Job>& job = jobs_[user_id];
+    job.reset(new Job(this, user_id));
+    job->LoadImage(base::FilePath(image_path), image_index, image_url);
   }
 }
 
-void UserImageManagerImpl::UserLoggedIn(const std::string& email,
+void UserImageManagerImpl::UserLoggedIn(const std::string& user_id,
                                         bool user_is_new,
                                         bool user_is_local) {
   User* user = UserManager::Get()->GetLoggedInUser();
   if (user_is_new) {
     if (!user_is_local)
-      SetInitialUserImage(email);
+      SetInitialUserImage(user_id);
   } else {
-    if (!user_is_local) {
-      // If current user image is profile image, it needs to be refreshed.
-      bool download_profile_image =
-          user->image_index() == User::kProfileImageIndex;
-      if (download_profile_image)
-        InitDownloadedProfileImage();
-
-      // Download user's profile data (full name and optionally image) to see if
-      // it has changed.
-      BrowserThread::PostDelayedTask(
-          BrowserThread::UI,
-          FROM_HERE,
-          base::Bind(&UserImageManagerImpl::DownloadProfileData,
-                     base::Unretained(this),
-                     kProfileDownloadReasonLoggedIn,
-                     download_profile_image),
-          base::TimeDelta::FromSeconds(kProfileDataDownloadDelaySec));
-    }
-
     UMA_HISTOGRAM_ENUMERATION("UserImage.LoggedIn",
                               ImageIndexToHistogramIndex(user->image_index()),
                               kHistogramImagesCount);
 
-    if (users_to_migrate_.count(email)) {
+    if (users_to_migrate_.find(user_id) != users_to_migrate_.end()) {
       const DictionaryValue* prefs_images_unsafe =
           g_browser_process->local_state()->GetDictionary(kUserImages);
       const base::DictionaryValue* image_properties = NULL;
       if (prefs_images_unsafe->GetDictionaryWithoutPathExpansion(
-              user->email(), &image_properties)) {
+              user_id, &image_properties)) {
         std::string image_path;
         image_properties->GetString(kImagePathNodeName, &image_path);
+        linked_ptr<Job>& job = jobs_[user_id];
+        job.reset(new Job(this, user_id));
         if (!image_path.empty()) {
-          // User needs image format migration but
-          // first we need to load and decode that image.
-          LOG(INFO) << "Waiting for user image to load before migration";
-          migrate_current_user_on_load_ = true;
-              unsafe_image_loader_->Start(
-                  image_path, 0  /* no resize */,
-                  base::Bind(&UserImageManagerImpl::SetUserImage,
-                             base::Unretained(this),
-                             user->email(),
-                             user->image_index(),
-                             user->image_url()));
+          LOG(INFO) << "Loading old user image, then migrating it.";
+          job->SetToPath(base::FilePath(image_path),
+                         user->image_index(),
+                         user->image_url(),
+                         false);
         } else {
-          // Otherwise migrate user image properties right away.
-          BrowserThread::PostDelayedTask(
-              BrowserThread::UI,
-              FROM_HERE,
-              base::Bind(&UserImageManagerImpl::MigrateUserImage,
-                         base::Unretained(this)),
-              base::TimeDelta::FromSeconds(user_image_migration_delay_sec));
+          job->SetToDefaultImage(user->image_index());
         }
       }
     }
   }
 
-  if (!user_is_local) {
-    // Set up a repeating timer for refreshing the profile data.
-    profile_download_timer_.Start(
-        FROM_HERE, base::TimeDelta::FromSeconds(kProfileRefreshIntervalSec),
-        this, &UserImageManagerImpl::DownloadProfileDataScheduled);
+  // Reset the downloaded profile image as a new user logged in.
+  downloaded_profile_image_ = gfx::ImageSkia();
+  downloaded_profile_image_data_url_.clear();
+  profile_image_url_ = GURL();
+  profile_image_requested_ = false;
+
+  if (UserManager::Get()->IsLoggedInAsRegularUser()) {
+    TryToInitDownloadedProfileImage();
+
+    // Schedule an initial download of the profile data (full name and
+    // optionally image).
+    profile_download_one_shot_timer_.Start(
+        FROM_HERE,
+        base::TimeDelta::FromSeconds(kProfileDataDownloadDelaySec),
+        base::Bind(&UserImageManagerImpl::DownloadProfileData,
+                   base::Unretained(this),
+                   kProfileDownloadReasonLoggedIn));
+    // Schedule periodic refreshes of the profile data.
+    profile_download_periodic_timer_.Start(
+        FROM_HERE,
+        base::TimeDelta::FromSeconds(kProfileRefreshIntervalSec),
+        base::Bind(&UserImageManagerImpl::DownloadProfileData,
+                   base::Unretained(this),
+                   kProfileDownloadReasonScheduled));
+  } else {
+    profile_download_one_shot_timer_.Stop();
+    profile_download_periodic_timer_.Stop();
   }
-  CommandLine* command_line = CommandLine::ForCurrentProcess();
+
+  const CommandLine* command_line = CommandLine::ForCurrentProcess();
+  if (user_image_sync_observer_.get() &&
+      !command_line->HasSwitch(::switches::kMultiProfiles)) {
+    NOTREACHED() << "User logged in more than once.";
+  }
+
   if (user->CanSyncImage() &&
       !command_line->HasSwitch(chromeos::switches::kDisableUserImageSync)) {
-    if (user_image_sync_observer_.get() &&
-        !command_line->HasSwitch(::switches::kMultiProfiles))
-      NOTREACHED() << "User logged in second time.";
     user_image_sync_observer_.reset(new UserImageSyncObserver(user));
+  } else {
+    user_image_sync_observer_.reset();
   }
 }
 
-void UserImageManagerImpl::SaveUserDefaultImageIndex(
-    const std::string& username,
-    int image_index) {
-  DCHECK(image_index >= 0 && image_index < kDefaultImagesCount);
-  SetUserImage(username, image_index, GURL(),
-               UserImage(GetDefaultImage(image_index)));
-  SaveImageToLocalState(username, "", image_index, GURL(), false);
+void UserImageManagerImpl::SaveUserDefaultImageIndex(const std::string& user_id,
+                                                     int default_image_index) {
+  linked_ptr<Job>& job = jobs_[user_id];
+  job.reset(new Job(this, user_id));
+  job->SetToDefaultImage(default_image_index);
 }
 
-void UserImageManagerImpl::SaveUserImage(const std::string& username,
+void UserImageManagerImpl::SaveUserImage(const std::string& user_id,
                                          const UserImage& user_image) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  SaveUserImageInternal(username, User::kExternalImageIndex,
-                        GURL(), user_image);
+  linked_ptr<Job>& job = jobs_[user_id];
+  job.reset(new Job(this, user_id));
+  job->SetToImage(User::kExternalImageIndex, user_image);
 }
 
-void UserImageManagerImpl::SaveUserImageFromFile(const std::string& username,
+void UserImageManagerImpl::SaveUserImageFromFile(const std::string& user_id,
                                                  const base::FilePath& path) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  // Always use unsafe image loader because we resize the image when saving
-  // anyway.
-  unsafe_image_loader_->Start(
-      path.value(), login::kMaxUserImageSize,
-      base::Bind(&UserImageManagerImpl::SaveUserImage,
-                 base::Unretained(this), username));
+  linked_ptr<Job>& job = jobs_[user_id];
+  job.reset(new Job(this, user_id));
+  job->SetToPath(path, User::kExternalImageIndex, GURL(), true);
 }
 
 void UserImageManagerImpl::SaveUserImageFromProfileImage(
-    const std::string& username) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  if (!downloaded_profile_image_.isNull()) {
-    // Profile image has already been downloaded, so save it to file right now.
-    DCHECK(profile_image_url_.is_valid());
-    SaveUserImageInternal(
-        username,
-        User::kProfileImageIndex, profile_image_url_,
-        UserImage::CreateAndEncode(downloaded_profile_image_));
-  } else {
-    // No profile image - use the stub image (gray avatar).
-    SetUserImage(username, User::kProfileImageIndex, GURL(), UserImage());
-    SaveImageToLocalState(username, "", User::kProfileImageIndex,
-                          GURL(), false);
-  }
+    const std::string& user_id) {
+  // Use the profile image if it has been downloaded already. Otherwise, use a
+  // stub image (gray avatar).
+  linked_ptr<Job>& job = jobs_[user_id];
+  job.reset(new Job(this, user_id));
+  job->SetToImage(User::kProfileImageIndex,
+                  downloaded_profile_image_.isNull() ?
+                      UserImage() :
+                      UserImage::CreateAndEncode(downloaded_profile_image_));
 }
 
-void UserImageManagerImpl::DeleteUserImage(const std::string& username) {
-  // Delete from the old dictionary, if present.
-  DeleteOldUserImage(username);
-
-  PrefService* prefs = g_browser_process->local_state();
-  DictionaryPrefUpdate prefs_images_update(prefs, kUserImageProperties);
-  const base::DictionaryValue* image_properties;
-  if (prefs_images_update->GetDictionaryWithoutPathExpansion(
-          username, &image_properties)) {
-    std::string image_path;
-    image_properties->GetString(kImageURLNodeName, &image_path);
-    prefs_images_update->RemoveWithoutPathExpansion(username, NULL);
-    DeleteImageFile(image_path);
-  }
+void UserImageManagerImpl::DeleteUserImage(const std::string& user_id) {
+  jobs_.erase(user_id);
+  DeleteUserImageAndLocalStateEntry(user_id, kUserImages);
+  DeleteUserImageAndLocalStateEntry(user_id, kUserImageProperties);
 }
 
 void UserImageManagerImpl::DownloadProfileImage(const std::string& reason) {
-  DownloadProfileData(reason, true);
+  profile_image_requested_ = true;
+  DownloadProfileData(reason);
 }
 
 UserImageSyncObserver* UserImageManagerImpl::GetSyncObserver() const {
@@ -428,230 +637,73 @@ UserImageSyncObserver* UserImageManagerImpl::GetSyncObserver() const {
 }
 
 void UserImageManagerImpl::Shutdown() {
-  profile_image_downloader_.reset();
+  profile_downloader_.reset();
   user_image_sync_observer_.reset();
 }
 
 const gfx::ImageSkia& UserImageManagerImpl::DownloadedProfileImage() const {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   return downloaded_profile_image_;
 }
 
-base::FilePath UserImageManagerImpl::GetImagePathForUser(
-    const std::string& username) {
-  std::string filename = username + kSafeImagePathExtension;
-  base::FilePath user_data_dir;
-  PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
-  return user_data_dir.AppendASCII(filename);
-}
-
-void UserImageManagerImpl::SetInitialUserImage(const std::string& username) {
+void UserImageManagerImpl::SetInitialUserImage(const std::string& user_id) {
   // Choose a random default image.
-  int image_id =
-      base::RandInt(kFirstDefaultImageIndex, kDefaultImagesCount - 1);
-  SaveUserDefaultImageIndex(username, image_id);
-}
-
-void UserImageManagerImpl::SetUserImage(const std::string& username,
-                                        int image_index,
-                                        const GURL& image_url,
-                                        const UserImage& user_image) {
-  User* user = const_cast<User*>(UserManager::Get()->FindUser(username));
-  // User may have been removed by now.
-  if (user) {
-    bool image_changed = user->image_index() != User::kInvalidImageIndex;
-    bool is_current_user = user == UserManager::Get()->GetLoggedInUser();
-    if (!user_image.image().isNull())
-      user->SetImage(user_image, image_index);
-    else
-      user->SetStubImage(image_index, false);
-    user->SetImageURL(image_url);
-    // For the logged-in user with a profile picture, initialize
-    // |downloaded_profile_picture_|.
-    if (is_current_user && image_index == User::kProfileImageIndex) {
-      InitDownloadedProfileImage();
-    }
-    if (image_changed) {
-      // Unless this is first-time setting with |SetInitialUserImage|,
-      // send a notification about image change.
-      content::NotificationService::current()->Notify(
-          chrome::NOTIFICATION_LOGIN_USER_IMAGE_CHANGED,
-          content::Source<UserImageManager>(this),
-          content::Details<const User>(user));
-    }
-    if (is_current_user && migrate_current_user_on_load_)
-      MigrateUserImage();
-  }
-}
-
-void UserImageManagerImpl::SaveUserImageInternal(const std::string& username,
-                                                 int image_index,
-                                                 const GURL& image_url,
-                                                 const UserImage& user_image) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  SetUserImage(username, image_index, image_url, user_image);
-
-  // Ignore if data stored or cached outside the user's cryptohome is to be
-  // treated as ephemeral.
-  if (UserManager::Get()->IsUserNonCryptohomeDataEphemeral(username))
-    return;
-
-  base::FilePath image_path = GetImagePathForUser(username);
-  DVLOG(1) << "Saving user image to " << image_path.value();
-
-  last_image_set_async_ = true;
-
-  base::WorkerPool::PostTask(
-      FROM_HERE,
-      base::Bind(&UserImageManagerImpl::SaveImageToFile,
-                 base::Unretained(this),
-                 username, user_image, image_path, image_index, image_url),
-      /* is_slow= */ false);
-}
-
-void UserImageManagerImpl::SaveImageToFile(const std::string& username,
-                                           const UserImage& user_image,
-                                           const base::FilePath& image_path,
-                                           int image_index,
-                                           const GURL& image_url) {
-  if (!SaveBitmapToFile(user_image, image_path))
-    return;
-
-  BrowserThread::PostTask(
-      BrowserThread::UI,
-      FROM_HERE,
-      base::Bind(&UserImageManagerImpl::SaveImageToLocalState,
-                 base::Unretained(this),
-                 username, image_path.value(), image_index, image_url, true));
-}
-
-void UserImageManagerImpl::SaveImageToLocalState(const std::string& username,
-                                                 const std::string& image_path,
-                                                 int image_index,
-                                                 const GURL& image_url,
-                                                 bool is_async) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  // Ignore if data stored or cached outside the user's cryptohome is to be
-  // treated as ephemeral.
-  if (UserManager::Get()->IsUserNonCryptohomeDataEphemeral(username))
-    return;
-
-  // TODO(ivankr): use unique filenames for user images each time
-  // a new image is set so that only the last image update is saved
-  // to Local State and notified.
-  if (is_async && !last_image_set_async_) {
-    DVLOG(1) << "Ignoring saved image because it has changed";
-    return;
-  } else if (!is_async) {
-    // Reset the async image save flag if called directly from the UI thread.
-    last_image_set_async_ = false;
-  }
-
-  PrefService* local_state = g_browser_process->local_state();
-  DictionaryPrefUpdate images_update(local_state, kUserImageProperties);
-  base::DictionaryValue* image_properties = new base::DictionaryValue();
-  image_properties->Set(kImagePathNodeName, new StringValue(image_path));
-  image_properties->Set(kImageIndexNodeName,
-                        new base::FundamentalValue(image_index));
-  if (!image_url.is_empty()) {
-    image_properties->Set(kImageURLNodeName,
-                          new StringValue(image_url.spec()));
-  } else {
-    image_properties->Remove(kImageURLNodeName, NULL);
-  }
-  images_update->SetWithoutPathExpansion(username, image_properties);
-  DVLOG(1) << "Saving path to user image in Local State.";
-
-  if (users_to_migrate_.count(username)) {
-    DeleteOldUserImage(username);
-    users_to_migrate_.erase(username);
-  }
-
-  UserManager::Get()->NotifyLocalStateChanged();
-}
-
-bool UserImageManagerImpl::SaveBitmapToFile(const UserImage& user_image,
-                                            const base::FilePath& image_path) {
-  UserImage safe_image;
-  const UserImage::RawImage* encoded_image = NULL;
-  if (!user_image.is_safe_format()) {
-    safe_image = UserImage::CreateAndEncode(user_image.image());
-    encoded_image = &safe_image.raw_image();
-    UMA_HISTOGRAM_MEMORY_KB("UserImage.RecodedJpegSize", encoded_image->size());
-  } else if (user_image.has_raw_image()) {
-    encoded_image = &user_image.raw_image();
-  } else {
-    NOTREACHED() << "Raw image missing.";
-    return false;
-  }
-
-  if (file_util::WriteFile(image_path,
-                           reinterpret_cast<const char*>(&(*encoded_image)[0]),
-                           encoded_image->size()) == -1) {
-    LOG(ERROR) << "Failed to save image to file.";
-    return false;
-  }
-  return true;
-}
-
-void UserImageManagerImpl::InitDownloadedProfileImage() {
-  const User* logged_in_user = UserManager::Get()->GetLoggedInUser();
-  DCHECK_EQ(logged_in_user->image_index(), User::kProfileImageIndex);
-  if (downloaded_profile_image_.isNull() && !logged_in_user->image_is_stub()) {
-    VLOG(1) << "Profile image initialized";
-    downloaded_profile_image_ = logged_in_user->image();
+  SaveUserDefaultImageIndex(user_id,
+                            base::RandInt(kFirstDefaultImageIndex,
+                                          kDefaultImagesCount - 1));
+}
+
+void UserImageManagerImpl::TryToInitDownloadedProfileImage() {
+  const User* user = UserManager::Get()->GetLoggedInUser();
+  if (user->image_index() == User::kProfileImageIndex &&
+      downloaded_profile_image_.isNull() &&
+      !user->image_is_stub()) {
+    // Initialize the |downloaded_profile_image_| for the currently logged-in
+    // user if it has not been initialized already, the user image is the
+    // profile image and the user image has been loaded successfully.
+    VLOG(1) << "Profile image initialized from disk.";
+    downloaded_profile_image_ = user->image();
     downloaded_profile_image_data_url_ =
         webui::GetBitmapDataUrl(*downloaded_profile_image_.bitmap());
-    profile_image_url_ = logged_in_user->image_url();
+    profile_image_url_ = user->image_url();
   }
 }
 
-void UserImageManagerImpl::DownloadProfileData(const std::string& reason,
-                                               bool download_image) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+bool UserImageManagerImpl::NeedProfileImage() const {
+  return UserManager::Get()->IsLoggedInAsRegularUser() &&
+         (UserManager::Get()->GetLoggedInUser()->image_index() ==
+              User::kProfileImageIndex ||
+          profile_image_requested_);
+}
 
+void UserImageManagerImpl::DownloadProfileData(const std::string& reason) {
   // GAIA profiles exist for regular users only.
   if (!UserManager::Get()->IsLoggedInAsRegularUser())
     return;
 
-  // Mark profile picture as needed.
-  downloading_profile_image_ |= download_image;
-
-  // Another download is already in progress
-  if (profile_image_downloader_.get())
+  // If a download is already in progress, allow it to continue, with one
+  // exception: If the current download does not include the profile image but
+  // the image has since become necessary, start a new download that includes
+  // the profile image.
+  if (profile_downloader_ &&
+      (downloading_profile_image_ || !NeedProfileImage())) {
     return;
+  }
 
+  downloading_profile_image_ = NeedProfileImage();
   profile_image_download_reason_ = reason;
-  profile_image_load_start_time_ = base::Time::Now();
-  profile_image_downloader_.reset(new ProfileDownloader(this));
-  profile_image_downloader_->Start();
-}
-
-void UserImageManagerImpl::DownloadProfileDataScheduled() {
-  const User* logged_in_user = UserManager::Get()->GetLoggedInUser();
-  // If current user image is profile image, it needs to be refreshed.
-  bool download_profile_image =
-      logged_in_user->image_index() == User::kProfileImageIndex;
-  DownloadProfileData(kProfileDownloadReasonScheduled, download_profile_image);
-}
-
-void UserImageManagerImpl::DownloadProfileDataRetry(bool download_image) {
-  DownloadProfileData(kProfileDownloadReasonRetry, download_image);
+  profile_image_load_start_time_ = base::TimeTicks::Now();
+  profile_downloader_.reset(new ProfileDownloader(this));
+  profile_downloader_->Start();
 }
 
-// ProfileDownloaderDelegate override.
 bool UserImageManagerImpl::NeedsProfilePicture() const {
   return downloading_profile_image_;
 }
 
-// ProfileDownloaderDelegate override.
 int UserImageManagerImpl::GetDesiredImageSideLength() const {
   return GetCurrentUserImageSize();
 }
 
-// ProfileDownloaderDelegate override.
 std::string UserImageManagerImpl::GetCachedPictureURL() const {
   return profile_image_url_.spec();
 }
@@ -662,21 +714,19 @@ Profile* UserImageManagerImpl::GetBrowserProfile() {
 
 void UserImageManagerImpl::OnProfileDownloadSuccess(
     ProfileDownloader* downloader) {
-  // Make sure that |ProfileDownloader| gets deleted after return.
-  scoped_ptr<ProfileDownloader> profile_image_downloader(
-      profile_image_downloader_.release());
-  DCHECK_EQ(downloader, profile_image_downloader.get());
+  // Ensure that the |profile_downloader_| is deleted when this method returns.
+  scoped_ptr<ProfileDownloader> profile_downloader(
+      profile_downloader_.release());
+  DCHECK_EQ(downloader, profile_downloader.get());
 
-  UserManager* user_manager = UserManager::Get();
-  const User* user = user_manager->GetLoggedInUser();
+  const User* user = UserManager::Get()->GetLoggedInUser();
+  const std::string& user_id = user->email();
 
-  user_manager->UpdateUserAccountData(user->email(),
-                                      downloader->GetProfileFullName(),
-                                      downloader->GetProfileLocale());
+  UserManager::Get()->UpdateUserAccountData(user_id,
+                                            downloader->GetProfileFullName(),
+                                            downloader->GetProfileLocale());
 
-  bool requested_image = downloading_profile_image_;
-  downloading_profile_image_ = false;
-  if (!requested_image)
+  if (!downloading_profile_image_)
     return;
 
   ProfileDownloadResult result = kDownloadFailure;
@@ -695,11 +745,17 @@ void UserImageManagerImpl::OnProfileDownloadSuccess(
   }
 
   UMA_HISTOGRAM_ENUMERATION("UserImage.ProfileDownloadResult",
-      result, kDownloadResultsCount);
-
+                            result,
+                            kDownloadResultsCount);
   DCHECK(!profile_image_load_start_time_.is_null());
-  base::TimeDelta delta = base::Time::Now() - profile_image_load_start_time_;
-  AddProfileImageTimeHistogram(result, profile_image_download_reason_, delta);
+  AddProfileImageTimeHistogram(
+      result,
+      profile_image_download_reason_,
+      base::TimeTicks::Now() - profile_image_load_start_time_);
+
+  // Ignore the image if it is no longer needed.
+  if (!NeedProfileImage())
+    return;
 
   if (result == kDownloadDefault) {
     content::NotificationService::current()->Notify(
@@ -708,16 +764,19 @@ void UserImageManagerImpl::OnProfileDownloadSuccess(
         content::NotificationService::NoDetails());
   }
 
-  // Nothing to do if picture is cached or the default avatar.
+  // Nothing to do if the picture is cached or is the default avatar.
   if (result != kDownloadSuccess)
     return;
 
+  profile_image_requested_ = false;
+
   // Check if this image is not the same as already downloaded.
-  SkBitmap new_bitmap(downloader->GetProfilePicture());
-  std::string new_image_data_url = webui::GetBitmapDataUrl(new_bitmap);
+  const std::string new_image_data_url =
+      webui::GetBitmapDataUrl(SkBitmap(downloader->GetProfilePicture()));
   if (!downloaded_profile_image_data_url_.empty() &&
-      new_image_data_url == downloaded_profile_image_data_url_)
+      new_image_data_url == downloaded_profile_image_data_url_) {
     return;
+  }
 
   downloaded_profile_image_data_url_ = new_image_data_url;
   downloaded_profile_image_ = gfx::ImageSkia::CreateFrom1xBitmap(
@@ -725,12 +784,12 @@ void UserImageManagerImpl::OnProfileDownloadSuccess(
   profile_image_url_ = GURL(downloader->GetProfilePictureURL());
 
   if (user->image_index() == User::kProfileImageIndex) {
-    VLOG(1) << "Updating profile image for logged-in user";
+    VLOG(1) << "Updating profile image for logged-in user.";
     UMA_HISTOGRAM_ENUMERATION("UserImage.ProfileDownloadResult",
                               kDownloadSuccessChanged,
                               kDownloadResultsCount);
-    // This will persist |downloaded_profile_image_| to file.
-    SaveUserImageFromProfileImage(user->email());
+    // This will persist |downloaded_profile_image_| to disk.
+    SaveUserImageFromProfileImage(user_id);
   }
 
   content::NotificationService::current()->Notify(
@@ -742,78 +801,132 @@ void UserImageManagerImpl::OnProfileDownloadSuccess(
 void UserImageManagerImpl::OnProfileDownloadFailure(
     ProfileDownloader* downloader,
     ProfileDownloaderDelegate::FailureReason reason) {
-  DCHECK_EQ(downloader, profile_image_downloader_.get());
-  profile_image_downloader_.reset();
+  DCHECK_EQ(downloader, profile_downloader_.get());
+  profile_downloader_.reset();
 
-  UMA_HISTOGRAM_ENUMERATION("UserImage.ProfileDownloadResult",
-      kDownloadFailure, kDownloadResultsCount);
-
-  DCHECK(!profile_image_load_start_time_.is_null());
-  base::TimeDelta delta = base::Time::Now() - profile_image_load_start_time_;
-  AddProfileImageTimeHistogram(kDownloadFailure, profile_image_download_reason_,
-                               delta);
+  if (downloading_profile_image_) {
+    UMA_HISTOGRAM_ENUMERATION("UserImage.ProfileDownloadResult",
+                              kDownloadFailure,
+                              kDownloadResultsCount);
+    DCHECK(!profile_image_load_start_time_.is_null());
+    AddProfileImageTimeHistogram(
+        kDownloadFailure,
+        profile_image_download_reason_,
+        base::TimeTicks::Now() - profile_image_load_start_time_);
+  }
 
   UserManager* user_manager = UserManager::Get();
   const User* user = user_manager->GetLoggedInUser();
 
-  // Need note that at least one attempt finished.
   user_manager->UpdateUserAccountData(user->email(), string16(), "");
 
-  // Retry download after some time if a network error has occured.
   if (reason == ProfileDownloaderDelegate::NETWORK_ERROR) {
-    BrowserThread::PostDelayedTask(
-        BrowserThread::UI,
+    // Retry download after a delay if a network error occurred.
+    profile_download_one_shot_timer_.Start(
         FROM_HERE,
-        base::Bind(&UserImageManagerImpl::DownloadProfileDataRetry,
+        base::TimeDelta::FromSeconds(kProfileDataDownloadRetryIntervalSec),
+        base::Bind(&UserImageManagerImpl::DownloadProfileData,
                    base::Unretained(this),
-                   downloading_profile_image_),
-        base::TimeDelta::FromSeconds(kProfileDataDownloadRetryIntervalSec));
+                   kProfileDownloadReasonRetry));
   }
 
-  downloading_profile_image_ = false;
-
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_PROFILE_IMAGE_UPDATE_FAILED,
       content::Source<UserImageManager>(this),
       content::NotificationService::NoDetails());
 }
 
-void UserImageManagerImpl::MigrateUserImage() {
-  User* user = UserManager::Get()->GetLoggedInUser();
-  if (user->image_is_loading()) {
-    LOG(INFO) << "Waiting for user image to load before migration";
-    migrate_current_user_on_load_ = true;
-    return;
-  }
-  migrate_current_user_on_load_ = false;
-  if (user->has_raw_image() && user->image_is_safe_format()) {
-    // Nothing to migrate already, make sure we delete old image.
-    DeleteOldUserImage(user->email());
-    users_to_migrate_.erase(user->email());
+void UserImageManagerImpl::DeleteUserImageAndLocalStateEntry(
+    const std::string& user_id,
+    const char* prefs_dict_root) {
+  DictionaryPrefUpdate update(g_browser_process->local_state(),
+                              prefs_dict_root);
+  const base::DictionaryValue* image_properties;
+  if (!update->GetDictionaryWithoutPathExpansion(user_id, &image_properties))
     return;
+
+  std::string image_path;
+  image_properties->GetString(kImagePathNodeName, &image_path);
+  if (!image_path.empty()) {
+    background_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(base::IgnoreResult(&base::DeleteFile),
+                   base::FilePath(image_path),
+                   false));
   }
-  if (user->HasDefaultImage()) {
-    SaveUserDefaultImageIndex(user->email(), user->image_index());
+  update->RemoveWithoutPathExpansion(user_id, NULL);
+}
+
+void UserImageManagerImpl::OnJobChangedUserImage(const User* user) {
+  if (user == UserManager::Get()->GetLoggedInUser())
+    TryToInitDownloadedProfileImage();
+
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_LOGIN_USER_IMAGE_CHANGED,
+      content::Source<UserImageManagerImpl>(this),
+      content::Details<const User>(user));
+}
+
+void UserImageManagerImpl::OnJobDone(const std::string& user_id) {
+  std::map<std::string, linked_ptr<Job> >::iterator it =
+      jobs_.find(user_id);
+  if (it != jobs_.end()) {
+    // The Job for |user_id| is done and must be destroyed. Since control will
+    // return to the Job when this method finishes, it cannot be destroyed from
+    // here. Instead, ownership is dropped and the Job destroys itself.
+    it->second.release();
+    jobs_.erase(it);
   } else {
-    SaveUserImageInternal(user->email(), user->image_index(),
-                          user->image_url(), user->user_image());
+    NOTREACHED();
+  }
+
+  if (users_to_migrate_.find(user_id) == users_to_migrate_.end())
+    return;
+  // Migration completed for |user_id|.
+  users_to_migrate_.erase(user_id);
+
+  const DictionaryValue* prefs_images_unsafe =
+      g_browser_process->local_state()->GetDictionary(kUserImages);
+  const base::DictionaryValue* image_properties = NULL;
+  if (!prefs_images_unsafe->GetDictionaryWithoutPathExpansion(
+          user_id, &image_properties)) {
+    NOTREACHED();
+    return;
   }
+
+  int image_index = User::kInvalidImageIndex;
+  image_properties->GetInteger(kImageIndexNodeName, &image_index);
   UMA_HISTOGRAM_ENUMERATION("UserImage.Migration",
-                            ImageIndexToHistogramIndex(user->image_index()),
+                            ImageIndexToHistogramIndex(image_index),
                             kHistogramImagesCount);
-}
 
-void UserImageManagerImpl::DeleteOldUserImage(const std::string& username) {
-  PrefService* prefs = g_browser_process->local_state();
-  DictionaryPrefUpdate prefs_images_update(prefs, kUserImages);
-  const base::DictionaryValue* image_properties;
-  if (prefs_images_update->GetDictionaryWithoutPathExpansion(
-          username, &image_properties)) {
-    std::string image_path;
-    image_properties->GetString(kImagePathNodeName, &image_path);
-    prefs_images_update->RemoveWithoutPathExpansion(username, NULL);
-    DeleteImageFile(image_path);
+  std::string image_path;
+  image_properties->GetString(kImagePathNodeName, &image_path);
+  if (!image_path.empty()) {
+    // If an old image exists, delete it and remove |user_id| from the old prefs
+    // dictionary only after the deletion has completed. This ensures that no
+    // orphaned image is left behind if the browser crashes before the deletion
+    // has been performed: In that case, local state will be unchanged and the
+    // migration will be run again on the user's next login.
+    background_task_runner_->PostTaskAndReply(
+        FROM_HERE,
+        base::Bind(base::IgnoreResult(&base::DeleteFile),
+                   base::FilePath(image_path),
+                   false),
+        base::Bind(&UserImageManagerImpl::UpdateLocalStateAfterMigration,
+                   weak_factory_.GetWeakPtr(),
+                   user_id));
+  } else {
+    // If no old image exists, remove |user_id| from the old prefs dictionary.
+    UpdateLocalStateAfterMigration(user_id);
   }
 }
 
+void UserImageManagerImpl::UpdateLocalStateAfterMigration(
+    const std::string& user_id) {
+  DictionaryPrefUpdate update(g_browser_process->local_state(),
+                              kUserImages);
+  update->RemoveWithoutPathExpansion(user_id, NULL);
+}
+
 }  // namespace chromeos