Make user_manager::UserImage non-copyable

chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc

 // Copyright 2014 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/login/users/avatar/user_image_manager_impl.h"
 
 #include <stddef.h>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 130 matching lines @@
     case user_manager::User::USER_IMAGE_EXTERNAL:
       // TODO(ivankr): Distinguish this from selected from file.
       return default_user_image::kHistogramImageFromCamera;
     case user_manager::User::USER_IMAGE_PROFILE:
       return default_user_image::kHistogramImageFromProfile;
     default:
       return image_index;
   }
 }
 
-bool SaveImage(const user_manager::UserImage& user_image,
+bool SaveImage(scoped_ptr<user_manager::UserImage::Bytes> image_bytes,
                const base::FilePath& image_path) {
-  // This should always be true, because of the following reasons:
-  //
-  // 1) Profile image from Google account -> UserImage is created with
-  //    CreateAndEncode() that generates safe bytes representation.
-  // 2) Profile image from user-specified image -> The bytes representation
-  //    is regenerated after the original image is decoded and cropped.
-  // 3) Profile image from policy (via OnExternalDataFetched()) -> JPEG is
-  //    only allowed and ROBUST_JPEG_CODEC is used.
-  //
-  // However, check the value just in case because an unsafe image should
-  // never be saved.
-  if (!user_image.is_safe_format()) {
-    LOG(ERROR) << "User image is not in safe format";
-    return false;
-  }
-
-  const user_manager::UserImage::Bytes& image_bytes = user_image.image_bytes();
-  if (image_bytes.empty() ||
+  if (image_bytes->empty() ||
       base::WriteFile(image_path,
-                      reinterpret_cast<const char*>(image_bytes.data()),
-                      image_bytes.size()) == -1) {
+                      reinterpret_cast<const char*>(image_bytes->data()),
+                      image_bytes->size()) == -1) {
     LOG(ERROR) << "Failed to save image to file.";
     return false;
   }
 
   return true;
 }
 
 }  // namespace
 
 const char UserImageManagerImpl::kUserImageProperties[] = "user_image_info";
@@ skipping 27 matching lines @@
                  const int image_index,
                  const GURL& image_url);
 
   // Sets the user image in local state to the default image indicated
   // by |default_image_index|. Also updates the user object with the
   // new image.
   void SetToDefaultImage(int default_image_index);
 
   // Saves the |user_image| to disk and sets the user image in local
   // state to that image. Also updates the user with the new image.
-  void SetToImage(int image_index, const user_manager::UserImage& user_image);
+  void SetToImage(int image_index,
+                  scoped_ptr<user_manager::UserImage> user_image);
 
   // Decodes the JPEG image |data|, crops and resizes the image, saves
   // it to disk and sets the user image in local state to that image.
   // Also updates the user object with the new image.
   void SetToImageData(scoped_ptr<std::string> data);
 
   // Loads the image at |path|, transcodes it to JPEG format, saves
   // the image to disk and sets the user image in local state to that
   // image.  If |resize| is true, the image is cropped and resized
   // before transcoding.  Also updates the user object 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 user_manager::UserImage& user_image);
+  void OnLoadImageDone(bool save,
+                       scoped_ptr<user_manager::UserImage> user_image);
 
   // Updates the user object with |user_image|.
-  void UpdateUser(const user_manager::UserImage& user_image);
+  void UpdateUser(scoped_ptr<user_manager::UserImage> user_image);
 
-  // Saves |user_image_| to disk in JPEG format. Local state will be updated
-  // when a callback indicates that the image has been saved.
-  void SaveImageAndUpdateLocalState(const user_manager::UserImage& user_image);
+  // Saves |image_bytes| to disk in JPEG format if
+  // |image_is_safe_format|. Local state will be updated when a callback
+  // indicates that the image has been saved.
+  void SaveImageAndUpdateLocalState(
+      bool image_is_safe_format,
+      scoped_ptr<user_manager::UserImage::Bytes> image_bytes);
 
   // Called back after the user image has been saved to
   // disk. Updates the user image information in local state. The
   // information is only updated if |success| is true (indicating that
   // the image was saved successfully) or the user image is the
   // profile image (indicating that even if the image could not be
   // saved because it is not available right now, it will be
   // downloaded eventually).
   void OnSaveImageDone(bool success);
 
@@ skipping 36 matching lines @@
   DCHECK(!run_);
   run_ = true;
 
   image_index_ = image_index;
   image_url_ = image_url;
   image_path_ = image_path;
 
   if (image_index_ >= 0 &&
       image_index_ < default_user_image::kDefaultImagesCount) {
     // Load one of the default images. This happens synchronously.
-    const user_manager::UserImage user_image(
-        default_user_image::GetDefaultImage(image_index_));
-    UpdateUser(user_image);
+    scoped_ptr<user_manager::UserImage> user_image(new user_manager::UserImage(
+        default_user_image::GetDefaultImage(image_index_)));
+    UpdateUser(std::move(user_image));
     NotifyJobDone();
   } else if (image_index_ == user_manager::User::USER_IMAGE_EXTERNAL ||
              image_index_ == user_manager::User::USER_IMAGE_PROFILE) {
     // Load the user image from a file referenced by |image_path|. This happens
     // asynchronously. ROBUST_JPEG_CODEC 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.
     DCHECK(!image_path_.empty());
     user_image_loader::StartWithFilePath(
         parent_->background_task_runner_, image_path_,
         ImageDecoder::ROBUST_JPEG_CODEC,
         0,  // Do not crop.
         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(default_user_image::kDefaultImagesCount, default_image_index);
 
   image_index_ = default_image_index;
-  const user_manager::UserImage user_image(
-      default_user_image::GetDefaultImage(image_index_));
+  scoped_ptr<user_manager::UserImage> user_image(new user_manager::UserImage(
+      default_user_image::GetDefaultImage(image_index_)));
 
-  UpdateUser(user_image);
+  UpdateUser(std::move(user_image));
   UpdateLocalState();
   NotifyJobDone();
 }
 
 void UserImageManagerImpl::Job::SetToImage(
     int image_index,
-    const user_manager::UserImage& user_image) {
+    scoped_ptr<user_manager::UserImage> user_image) {
   DCHECK(!run_);
   run_ = true;
 
   DCHECK(image_index == user_manager::User::USER_IMAGE_EXTERNAL ||
          image_index == user_manager::User::USER_IMAGE_PROFILE);
 
   image_index_ = image_index;
 
-  UpdateUser(user_image);
-  SaveImageAndUpdateLocalState(user_image);
+  // Copy the image bytes, before the user image is passed to
+  // UpdateUser(). This is needed to safely save the data bytes to the disk
+  // in the blocking pool. Copying is not desirable but the user image is
+  // JPEG data of 512x512 pixels (usually <100KB), thus it's OK.

[hashimoto, 2016/03/15 08:39:26]

I don't want to call it OK to copy 100KB of data on the UI thread.

Probably it's better to change image_byte's type to base::RefCountedBytes to
eliminate the need of copy.

[satorux1, 2016/03/16 02:01:30]

Good point. Let me revisit this in a separate patch in favor of doing less
things in a patch. Updated the comment with a TODO.

+  const bool image_is_safe_format = user_image->is_safe_format();
+  scoped_ptr<user_manager::UserImage::Bytes> copied_bytes;
+  if (image_is_safe_format) {
+    copied_bytes.reset(
+        new user_manager::UserImage::Bytes(user_image->image_bytes()));
+  }
+
+  UpdateUser(std::move(user_image));
+
+  SaveImageAndUpdateLocalState(image_is_safe_format, std::move(copied_bytes));
 }
 
 void UserImageManagerImpl::Job::SetToImageData(scoped_ptr<std::string> data) {
   DCHECK(!run_);
   run_ = true;
 
   image_index_ = user_manager::User::USER_IMAGE_EXTERNAL;
 
   // This method uses ROBUST_JPEG_CODEC, not DEFAULT_CODEC:
   // * This is necessary because the method is used to update the user image
@@ skipping 23 matching lines @@
 
   DCHECK(!path.empty());
   user_image_loader::StartWithFilePath(
       parent_->background_task_runner_, path, ImageDecoder::DEFAULT_CODEC,
       resize ? login::kMaxUserImageSize : 0,
       base::Bind(&Job::OnLoadImageDone, weak_factory_.GetWeakPtr(), true));
 }
 
 void UserImageManagerImpl::Job::OnLoadImageDone(
     bool save,
-    const user_manager::UserImage& user_image) {
-  UpdateUser(user_image);
-  if (save)
-    SaveImageAndUpdateLocalState(user_image);
-  else
+    scoped_ptr<user_manager::UserImage> user_image) {
+  if (save) {
+    // See the comment in SetToImageData() for why the copy is created.

[hashimoto, 2016/03/15 08:39:26]

This comment is not maintainable, as it's super easy to modify/rename/delete
SetToImageData() without revising this comment.
Please repeat the same thing here, or share the same code between these 2
places.

[hashimoto, 2016/03/17 06:06:51]

ping?

[satorux1, 2016/03/17 06:56:22]

oops. i missed this comment. introduced a function to share the code.

+    const bool image_is_safe_format = user_image->is_safe_format();
+    scoped_ptr<user_manager::UserImage::Bytes> copied_bytes;
+    if (image_is_safe_format) {
+      copied_bytes.reset(
+          new user_manager::UserImage::Bytes(user_image->image_bytes()));
+    }
+
+    UpdateUser(std::move(user_image));
+
+    SaveImageAndUpdateLocalState(image_is_safe_format, std::move(copied_bytes));
+  } else {
+    UpdateUser(std::move(user_image));
     NotifyJobDone();
+  }
 }
 
 void UserImageManagerImpl::Job::UpdateUser(
-    const user_manager::UserImage& user_image) {
+    scoped_ptr<user_manager::UserImage> user_image) {
   user_manager::User* user = parent_->GetUserAndModify();
   if (!user)
     return;
 
-  if (!user_image.image().isNull()) {
-    user->SetImage(user_image, image_index_);
+  if (!user_image->image().isNull()) {
+    user->SetImage(std::move(user_image), image_index_);
   } else {
     user->SetStubImage(
-        user_manager::UserImage(
+        make_scoped_ptr(new user_manager::UserImage(
             *ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
-                IDR_PROFILE_PICTURE_LOADING)),
-        image_index_,
-        false);
+                IDR_PROFILE_PICTURE_LOADING))),
+        image_index_, false);
   }
   user->SetImageURL(image_url_);
 
   parent_->OnJobChangedUserImage();
 }
 
 void UserImageManagerImpl::Job::SaveImageAndUpdateLocalState(
-    const user_manager::UserImage& user_image) {
+    bool image_is_safe_format,
+    scoped_ptr<user_manager::UserImage::Bytes> image_bytes) {
   base::FilePath user_data_dir;
   PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
   image_path_ = user_data_dir.Append(user_id() + kSafeImagePathExtension);
 
+  // This should always be true, because of the following reasons:
+  //
+  // 1) Profile image from Google account -> UserImage is created with
+  //    CreateAndEncode() that generates safe bytes representation.
+  // 2) Profile image from user-specified image -> The bytes representation
+  //    is regenerated after the original image is decoded and cropped.
+  // 3) Profile image from policy (via OnExternalDataFetched()) -> JPEG is
+  //    only allowed and ROBUST_JPEG_CODEC is used.
+  //
+  // However, check the value just in case because an unsafe image should
+  // never be saved.
+  if (!image_is_safe_format) {
+    LOG(ERROR) << "User image is not in safe format";
+    OnSaveImageDone(false);
+    return;
+  }
+
   base::PostTaskAndReplyWithResult(
       parent_->background_task_runner_.get(), FROM_HERE,
-      base::Bind(&SaveImage, user_image, image_path_),
+      base::Bind(&SaveImage,
+                 base::Passed(std::move(image_bytes)), image_path_),
       base::Bind(&Job::OnSaveImageDone, weak_factory_.GetWeakPtr()));
 }
 
 void UserImageManagerImpl::Job::OnSaveImageDone(bool success) {
   if (success || image_index_ == user_manager::User::USER_IMAGE_PROFILE)
     UpdateLocalState();
   NotifyJobDone();
 }
 
 void UserImageManagerImpl::Job::UpdateLocalState() {
@@ skipping 57 matching lines @@
 
   if (!image_properties) {
     SetInitialUserImage();
     return;
   }
 
   int image_index = user_manager::User::USER_IMAGE_INVALID;
   image_properties->GetInteger(kImageIndexNodeName, &image_index);
   if (image_index >= 0 &&
       image_index < default_user_image::kDefaultImagesCount) {
-    user->SetImage(user_manager::UserImage(
-                       default_user_image::GetDefaultImage(image_index)),
+    user->SetImage(make_scoped_ptr(new user_manager::UserImage(
+                       default_user_image::GetDefaultImage(image_index))),
                    image_index);
     return;
   }
 
   if (image_index != user_manager::User::USER_IMAGE_EXTERNAL &&
       image_index != user_manager::User::USER_IMAGE_PROFILE) {
     NOTREACHED();
     return;
   }
 
   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);
-  user->SetStubImage(user_manager::UserImage(
+  user->SetStubImage(make_scoped_ptr(new user_manager::UserImage(
                          *ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
-                             IDR_PROFILE_PICTURE_LOADING)),
-                     image_index,
-                     true);
+                             IDR_PROFILE_PICTURE_LOADING))),
+                     image_index, true);
   DCHECK(!image_path.empty() ||
          image_index == user_manager::User::USER_IMAGE_PROFILE);
   if (image_path.empty()) {
     // Return if the profile image is to be used but has not been downloaded
     // yet. The profile image will be downloaded after login.
     return;
   }
 
   job_.reset(new Job(this));
   job_->LoadImage(base::FilePath(image_path), image_index, image_url);
@@ skipping 48 matching lines @@
 }
 
 void UserImageManagerImpl::SaveUserDefaultImageIndex(int default_image_index) {
   if (IsUserImageManaged())
     return;
   job_.reset(new Job(this));
   job_->SetToDefaultImage(default_image_index);
 }
 
 void UserImageManagerImpl::SaveUserImage(
-    const user_manager::UserImage& user_image) {
+    scoped_ptr<user_manager::UserImage> user_image) {
   if (IsUserImageManaged())
     return;
   job_.reset(new Job(this));
-  job_->SetToImage(user_manager::User::USER_IMAGE_EXTERNAL, user_image);
+  job_->SetToImage(user_manager::User::USER_IMAGE_EXTERNAL,
+                   std::move(user_image));
 }
 
 void UserImageManagerImpl::SaveUserImageFromFile(const base::FilePath& path) {
   if (IsUserImageManaged())
     return;
   job_.reset(new Job(this));
   job_->SetToPath(path, user_manager::User::USER_IMAGE_EXTERNAL, GURL(), true);
 }
 
 void UserImageManagerImpl::SaveUserImageFromProfileImage() {
   if (IsUserImageManaged())
     return;
   // Use the profile image if it has been downloaded already. Otherwise, use a
   // stub image (gray avatar).
   job_.reset(new Job(this));
   job_->SetToImage(user_manager::User::USER_IMAGE_PROFILE,
                    downloaded_profile_image_.isNull()
-                       ? user_manager::UserImage()
+                       ? make_scoped_ptr(new user_manager::UserImage)
                        : user_manager::UserImage::CreateAndEncode(
                              downloaded_profile_image_));
   // If no profile image has been downloaded yet, ensure that a download is
   // started.
   if (downloaded_profile_image_.isNull())
     DownloadProfileData(kProfileDownloadReasonProfileImageChosen);
 }
 
 void UserImageManagerImpl::DeleteUserImage() {
   job_.reset();
@@ skipping 292 matching lines @@
 }
 
 bool UserImageManagerImpl::IsUserLoggedInAndHasGaiaAccount() const {
   const user_manager::User* user = GetUser();
   if (!user)
     return false;
   return user->is_logged_in() && user->HasGaiaAccount();
 }
 
 }  // namespace chromeos