Make PendingWallpaper pass weak pointers with a default delay

After applying a default delay to PendingWallpaper, some tests
failed (crbug.com/779504) or have memory leak [1].

These tests indirectly called SetUserWallpaper, but they do not
care/know about this, so they do not actively wait until wallpaper
loading is done, but start the destruction directly.

This used to be okay when SetUserWallpaper had zero delay.

But when it has the default delay, the timer is still running when
the destruction starts, and the timer holds a reference to the shared
pointer of PendingWallpaper, which results in memory leak.

What's worse, if the WallpaperManager already destructs itself,
PendingWallpaper::ProcessRequest does not know it, and it crashes.

After changing the shared pointer to a weak pointer, WallpaperManager
will be able to completely manage the lifetime of PendingWallpaper.

[1] https://luci-milo.appspot.com/buildbot/chromium.memory/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20(1)/24405

Below are the description of the original CL:

1) All the PendingWallpaper requests are now subject to a delay
calculated from average loading time. However, in most cases the delay
is still zero, and this change only affects corner cases that multiple
requests coming together, which may result in wallpaper being stuck.

2) Add the account_id update in PendingWallpaper. Did not modify
the functionality of PendingWallpaper, except for some renaming.

TBR=oshima@chromium.org

Bug: 779504, 778451
Change-Id: Iae943f88bbe046849c3afab75f35a253934fe75b
Reviewed-on: https://chromium-review.googlesource.com/745022Reviewed-by: Wenzhao (Colin) Zang <wzang@chromium.org>
Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Commit-Queue: Wenzhao (Colin) Zang <wzang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512883}

chrome/browser/chromeos/BUILD.gn

@@ -1823,6 +1823,8 @@ source_set("unit_tests") {
     "login/users/affiliation_unittest.cc",
     "login/users/multi_profile_user_controller_unittest.cc",
     "login/users/user_manager_unittest.cc",
+    "login/users/wallpaper/wallpaper_manager_test_utils.cc",
+    "login/users/wallpaper/wallpaper_manager_test_utils.h",
     "login/users/wallpaper/wallpaper_manager_unittest.cc",
     "mobile/mobile_activator_unittest.cc",
     "mobile_config_unittest.cc",

chrome/browser/chromeos/arc/wallpaper/arc_wallpaper_service.cc

@@ -233,8 +233,8 @@ void ArcWallpaperService::SetDefaultWallpaper() {
   // ImageDecoder::ImageRequest.
   decode_request_.reset();
   const PrimaryAccount& account = GetPrimaryAccount();
-  chromeos::WallpaperManager::Get()->SetDefaultWallpaper(account.id,
-                                                         account.is_active);
+  chromeos::WallpaperManager::Get()->SetDefaultWallpaper(
+      account.id, account.is_active /* update_wallpaper */);
 }
 
 void ArcWallpaperService::GetWallpaper(GetWallpaperCallback callback) {

chrome/browser/chromeos/arc/wallpaper/arc_wallpaper_service_unittest.cc

@@ -16,6 +16,7 @@
 #include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
 #include "chrome/browser/chromeos/login/users/scoped_user_manager_enabler.h"
 #include "chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h"
+#include "chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_test_utils.h"
 #include "chrome/browser/image_decoder.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
@@ -131,6 +132,8 @@ class ArcWallpaperServiceTest : public ash::AshTestBase {
 TEST_F(ArcWallpaperServiceTest, SetDefaultWallpaper) {
   service_->SetDefaultWallpaper();
   RunAllPendingInMessageLoop();
+  // Wait until wallpaper loading is done.
+  chromeos::wallpaper_manager_test_utils::WaitAsyncWallpaperLoadFinished();
   ASSERT_EQ(1u, wallpaper_instance_->changed_ids().size());
   EXPECT_EQ(-1, wallpaper_instance_->changed_ids()[0]);
 }
@@ -141,7 +144,8 @@ TEST_F(ArcWallpaperServiceTest, SetAndGetWallpaper) {
   std::vector<uint8_t> bytes;
   service_->SetWallpaper(bytes, 10 /* ID */);
   content::RunAllTasksUntilIdle();
-
+  // Wait until wallpaper loading is done.
+  chromeos::wallpaper_manager_test_utils::WaitAsyncWallpaperLoadFinished();
   ASSERT_EQ(1u, wallpaper_instance_->changed_ids().size());
   EXPECT_EQ(10, wallpaper_instance_->changed_ids()[0]);
 
@@ -160,6 +164,8 @@ TEST_F(ArcWallpaperServiceTest, SetWallpaperFailure) {
   std::vector<uint8_t> bytes;
   service_->SetWallpaper(bytes, 10 /* ID */);
   content::RunAllTasksUntilIdle();
+  // Wait until wallpaper loading is done.
+  chromeos::wallpaper_manager_test_utils::WaitAsyncWallpaperLoadFinished();
 
   // For failure case, ArcWallpaperService reports that wallpaper is changed to
   // requested wallpaper (ID=10), then reports that the wallpaper is changed

chrome/browser/chromeos/customization/customization_wallpaper_downloader_browsertest.cc

@@ -247,7 +247,8 @@ class CustomizationWallpaperDownloaderBrowserTest
 IN_PROC_BROWSER_TEST_F(CustomizationWallpaperDownloaderBrowserTest,
                        OEMWallpaperIsPresent) {
   CreateCmdlineWallpapers();
-  WallpaperManager::Get()->SetDefaultWallpaperNow(EmptyAccountId());
+  WallpaperManager::Get()->SetDefaultWallpaper(EmptyAccountId(),
+                                               true /* update_wallpaper */);
   wallpaper_manager_test_utils::WaitAsyncWallpaperLoadFinished();
   EXPECT_TRUE(wallpaper_manager_test_utils::ImageIsNearColor(
       ash::Shell::Get()->wallpaper_controller()->GetWallpaper(),
@@ -276,7 +277,8 @@ IN_PROC_BROWSER_TEST_F(CustomizationWallpaperDownloaderBrowserTest,
 IN_PROC_BROWSER_TEST_F(CustomizationWallpaperDownloaderBrowserTest,
                        OEMWallpaperRetryFetch) {
   CreateCmdlineWallpapers();
-  WallpaperManager::Get()->SetDefaultWallpaperNow(EmptyAccountId());
+  WallpaperManager::Get()->SetDefaultWallpaper(EmptyAccountId(),
+                                               true /* update_wallpaper */);
   wallpaper_manager_test_utils::WaitAsyncWallpaperLoadFinished();
   EXPECT_TRUE(wallpaper_manager_test_utils::ImageIsNearColor(
       ash::Shell::Get()->wallpaper_controller()->GetWallpaper(),

chrome/browser/chromeos/extensions/wallpaper_private_api.cc

@@ -479,7 +479,8 @@ bool WallpaperPrivateResetWallpaperFunction::RunAsync() {
   if (wallpaper_manager->IsPolicyControlled(account_id))
     return false;
 
-  wallpaper_manager->SetDefaultWallpaper(account_id, true);
+  wallpaper_manager->SetDefaultWallpaper(account_id,
+                                         true /* update_wallpaper */);
 
   Profile* profile = Profile::FromBrowserContext(browser_context());
   // This API is only available to the component wallpaper picker. We do not

chrome/browser/chromeos/login/lock/views_screen_locker.cc

@@ -188,7 +188,7 @@ void ViewsScreenLocker::HandleOnFocusPod(const AccountId& account_id) {
     lock_screen_utils::SetUserInputMethod(account_id.GetUserEmail(),
                                           ime_state_.get());
     lock_screen_utils::SetKeyboardSettings(account_id);
-    WallpaperManager::Get()->SetUserWallpaperDelayed(account_id);
+    WallpaperManager::Get()->SetUserWallpaper(account_id);
 
     bool use_24hour_clock = false;
     if (user_manager::known_user::GetBooleanPref(

chrome/browser/chromeos/login/ui/user_adding_screen.cc

@@ -69,7 +69,7 @@ void UserAddingScreenImpl::Cancel() {
 
   // Reset wallpaper if cancel adding user from multiple user sign in page.
   if (user_manager::UserManager::Get()->IsUserLoggedIn()) {
-    WallpaperManager::Get()->SetUserWallpaperDelayed(
+    WallpaperManager::Get()->SetUserWallpaper(
         user_manager::UserManager::Get()->GetActiveUser()->GetAccountId());
   }
 }

chrome/browser/chromeos/login/ui/webui_login_display.cc

@@ -223,14 +223,14 @@ void WebUILoginDisplay::MigrateUserData(const std::string& old_password) {
 }
 
 void WebUILoginDisplay::LoadWallpaper(const AccountId& account_id) {
-  WallpaperManager::Get()->SetUserWallpaperDelayed(account_id);
+  WallpaperManager::Get()->SetUserWallpaper(account_id);
 }
 
 void WebUILoginDisplay::LoadSigninWallpaper() {
   if (!WallpaperManager::Get()->SetDeviceWallpaperIfApplicable(
           user_manager::SignInAccountId())) {
-    WallpaperManager::Get()->SetDefaultWallpaperDelayed(
-        user_manager::SignInAccountId());
+    WallpaperManager::Get()->SetDefaultWallpaper(
+        user_manager::SignInAccountId(), true /* update_wallpaper */);
   }
 }
 

chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc

@@ -770,7 +770,7 @@ void ChromeUserManagerImpl::GuestUserLoggedIn() {
       user_manager::User::USER_IMAGE_INVALID, false);
 
   // Initializes wallpaper after active_user_ is set.
-  WallpaperManager::Get()->SetUserWallpaperNow(user_manager::GuestAccountId());
+  WallpaperManager::Get()->SetUserWallpaper(user_manager::GuestAccountId());
 }
 
 void ChromeUserManagerImpl::RegularUserLoggedIn(const AccountId& account_id) {
@@ -785,7 +785,7 @@ void ChromeUserManagerImpl::RegularUserLoggedIn(const AccountId& account_id) {
   }
 
   if (IsCurrentUserNew())
-    WallpaperManager::Get()->SetUserWallpaperNow(account_id);
+    WallpaperManager::Get()->SetUserWallpaper(account_id);
 
   GetUserImageManager(account_id)->UserLoggedIn(IsCurrentUserNew(), false);
 
@@ -801,7 +801,7 @@ void ChromeUserManagerImpl::RegularUserLoggedInAsEphemeral(
   ChromeUserManager::RegularUserLoggedInAsEphemeral(account_id);
 
   GetUserImageManager(account_id)->UserLoggedIn(IsCurrentUserNew(), false);
-  WallpaperManager::Get()->SetUserWallpaperNow(account_id);
+  WallpaperManager::Get()->SetUserWallpaper(account_id);
 }
 
 void ChromeUserManagerImpl::SupervisedUserLoggedIn(
@@ -816,11 +816,11 @@ void ChromeUserManagerImpl::SupervisedUserLoggedIn(
     SetIsCurrentUserNew(true);
     active_user_ = user_manager::User::CreateSupervisedUser(account_id);
     // Leaving OAuth token status at the default state = unknown.
-    WallpaperManager::Get()->SetUserWallpaperNow(account_id);
+    WallpaperManager::Get()->SetUserWallpaper(account_id);
   } else {
     if (supervised_user_manager_->CheckForFirstRun(account_id.GetUserEmail())) {
       SetIsCurrentUserNew(true);
-      WallpaperManager::Get()->SetUserWallpaperNow(account_id);
+      WallpaperManager::Get()->SetUserWallpaper(account_id);
     } else {
       SetIsCurrentUserNew(false);
     }
@@ -860,7 +860,7 @@ void ChromeUserManagerImpl::PublicAccountUserLoggedIn(
   // always fetched/cleared inside a user session), in the case the user-policy
   // controlled wallpaper was cached/cleared by not updated in the login screen,
   // so we need to update the wallpaper after the public user logged in.
-  WallpaperManager::Get()->SetUserWallpaperNow(user->GetAccountId());
+  WallpaperManager::Get()->SetUserWallpaper(user->GetAccountId());
   WallpaperManager::Get()->EnsureLoggedInUserWallpaperLoaded();
 
   SetPublicAccountDelegates();
@@ -877,7 +877,7 @@ void ChromeUserManagerImpl::KioskAppLoggedIn(user_manager::User* user) {
       user_manager::User::USER_IMAGE_INVALID, false);
 
   const AccountId& kiosk_app_account_id = user->GetAccountId();
-  WallpaperManager::Get()->SetUserWallpaperNow(kiosk_app_account_id);
+  WallpaperManager::Get()->SetUserWallpaper(kiosk_app_account_id);
 
   // TODO(bartfab): Add KioskAppUsers to the users_ list and keep metadata like
   // the kiosk_app_id in these objects, removing the need to re-parse the
@@ -947,7 +947,7 @@ void ChromeUserManagerImpl::DemoAccountLoggedIn() {
           *ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
               IDR_LOGIN_DEFAULT_USER)),
       user_manager::User::USER_IMAGE_INVALID, false);
-  WallpaperManager::Get()->SetUserWallpaperNow(user_manager::DemoAccountId());
+  WallpaperManager::Get()->SetUserWallpaper(user_manager::DemoAccountId());
 
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
   command_line->AppendSwitch(::switches::kForceAppMode);

chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h

@@ -257,30 +257,21 @@ class WallpaperManager : public ash::mojom::WallpaperPicker,
                           const gfx::ImageSkia& image,
                           bool update_wallpaper);
 
-  // Updates wallpaper info for |account_id| to default. If |update_wallpaper|
-  // is false, don't change wallpaper but only update cache.
+  // Sets default wallpaper. |update_wallpaper| indicates whether to actually
+  // change the wallpaper, or only update cache.
   void SetDefaultWallpaper(const AccountId& account_id, bool update_wallpaper);
 
-  // Sets wallpaper to default wallpaper (asynchronously with zero delay).
-  void SetDefaultWallpaperNow(const AccountId& account_id);
+  // Sets |account_id|'s wallpaper.
+  void SetUserWallpaper(const AccountId& account_id);
 
-  // Sets wallpaper to default wallpaper (asynchronously with default delay).
-  void SetDefaultWallpaperDelayed(const AccountId& account_id);
-
-  // Sets |account_id|'s wallpaper (asynchronously with zero delay).
-  void SetUserWallpaperNow(const AccountId& account_id);
-
-  // Sets |account_id|'s wallpaper (asynchronously with default delay).
-  void SetUserWallpaperDelayed(const AccountId& account_id);
-
-  // Sets selected wallpaper information for |account_id| and saves it to Local
-  // State if |is_persistent| is true.
+  // Sets wallpaper info for |account_id| and saves it to local state if
+  // |is_persistent| is true.
   void SetUserWallpaperInfo(const AccountId& account_id,
                             const wallpaper::WallpaperInfo& info,
                             bool is_persistent);
 
-  // Sets wallpaper to |image| (asynchronously with zero delay). If
-  // |update_wallpaper| is false, skip change wallpaper but only update cache.
+  // Sets wallpaper to |image|. If |update_wallpaper| is false, skip change
+  // wallpaper but only update cache.
   void SetWallpaperFromImageSkia(const AccountId& account_id,
                                  const gfx::ImageSkia& image,
                                  wallpaper::WallpaperLayout layout,
@@ -292,7 +283,7 @@ class WallpaperManager : public ash::mojom::WallpaperPicker,
   void InitializeWallpaper();
 
   // Updates current wallpaper. It may switch the size of wallpaper based on the
-  // current display's resolution. (asynchronously with zero delay)
+  // current display's resolution.
   void UpdateWallpaper(bool clear_cache);
 
   // Returns if the image is in the pending list. |image_id| can be obtained
@@ -515,9 +506,6 @@ class WallpaperManager : public ash::mojom::WallpaperPicker,
                           MovableOnDestroyCallbackHolder on_finish,
                           std::unique_ptr<user_manager::UserImage> user_image);
 
-  // Creates new PendingWallpaper request (or updates currently pending).
-  void ScheduleSetUserWallpaper(const AccountId& account_id, bool delayed);
-
   // Sets wallpaper to default if |update_wallpaper| is true. Otherwise just
   // load defaut wallpaper to cache.
   void DoSetDefaultWallpaper(const AccountId& account_id,
@@ -538,11 +526,9 @@ class WallpaperManager : public ash::mojom::WallpaperPicker,
   // Notify all registered observers.
   void NotifyAnimationFinished();
 
-  // Calculate delay for next wallpaper load.
-  // It is usually average wallpaper load time.
-  // If last wallpaper load happened long ago, timeout should be reduced by
-  // the time passed after last wallpaper load. So usual user experience results
-  // in zero delay.
+  // Calculates delay for the next wallpaper load. In most cases it is zero. It
+  // starts with the average wallpaper load time, and is reduced by the time
+  // passed after the last wallpaper load.
   base::TimeDelta GetWallpaperLoadDelay() const;
 
   // This is called after we check that supplied default wallpaper files exist.
@@ -606,13 +592,13 @@ class WallpaperManager : public ash::mojom::WallpaperPicker,
                                      bool update_wallpaper,
                                      MovableOnDestroyCallbackHolder on_finish);
 
-  // Returns modifiable PendingWallpaper.
-  // Returns pending_inactive_ or creates new PendingWallpaper if necessary.
-  PendingWallpaper* GetPendingWallpaper(const AccountId& account_id,
-                                        bool delayed);
+  // Returns modifiable PendingWallpaper. (Either |pending_inactive_| or a new
+  // |PendingWallpaper| object.)
+  PendingWallpaper* GetPendingWallpaper();
 
   // This is called by PendingWallpaper when load is finished.
-  void RemovePendingWallpaperFromList(PendingWallpaper* pending);
+  void RemovePendingWallpaperFromList(
+      PendingWallpaper* finished_loading_request);
 
   // Set wallpaper to |user_image| controlled by policy.  (Takes a UserImage
   // because that's the callback interface provided by UserImageLoader.)
@@ -718,7 +704,7 @@ class WallpaperManager : public ash::mojom::WallpaperPicker,
   // Owns PendingWallpaper.
   // PendingWallpaper deletes itself from here on load complete.
   // All pending will be finally deleted on destroy.
-  typedef std::vector<scoped_refptr<PendingWallpaper>> PendingList;
+  typedef std::vector<PendingWallpaper*> PendingList;
   PendingList loading_;
 
   content::NotificationRegistrar registrar_;

chrome/browser/ui/ash/lock_screen_client.cc

@@ -92,7 +92,7 @@ void LockScreenClient::FocusLockScreenApps(bool reverse) {
 }
 
 void LockScreenClient::LoadWallpaper(const AccountId& account_id) {
-  chromeos::WallpaperManager::Get()->SetUserWallpaperDelayed(account_id);
+  chromeos::WallpaperManager::Get()->SetUserWallpaper(account_id);
 }
 
 void LockScreenClient::SignOutUser() {

chrome/browser/ui/ash/multi_user/user_switch_animator_chromeos.cc

@@ -189,7 +189,7 @@ void UserSwitchAnimatorChromeOS::TransitionWallpaper(
     wallpaper_delegate->SetAnimationDurationOverride(
         std::max(duration, kMinimalAnimationTimeMS));
     if (screen_cover_ != NEW_USER_COVERS_SCREEN) {
-      chromeos::WallpaperManager::Get()->SetUserWallpaperNow(new_account_id_);
+      chromeos::WallpaperManager::Get()->SetUserWallpaper(new_account_id_);
       wallpaper_user_id_for_test_ =
           (NO_USER_COVERS_SCREEN == screen_cover_ ? "->" : "") +
           new_account_id_.Serialize();
@@ -198,7 +198,7 @@ void UserSwitchAnimatorChromeOS::TransitionWallpaper(
     // Revert the wallpaper cross dissolve animation duration back to the
     // default.
     if (screen_cover_ == NEW_USER_COVERS_SCREEN)
-      chromeos::WallpaperManager::Get()->SetUserWallpaperNow(new_account_id_);
+      chromeos::WallpaperManager::Get()->SetUserWallpaper(new_account_id_);
 
     // Coming here the wallpaper user id is the final result. No matter how we
     // got here.

chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc

@@ -300,7 +300,7 @@ void SupervisedUserCreationScreenHandler::HandleManagerSelected(
     const AccountId& manager_id) {
   if (!delegate_)
     return;
-  WallpaperManager::Get()->SetUserWallpaperNow(manager_id);
+  WallpaperManager::Get()->SetUserWallpaper(manager_id);
 }
 
 void SupervisedUserCreationScreenHandler::HandleImportUserSelected(