0

wallpaper: Save to drivefs task runner should have MayBlock trait.

This caused a DCHECK failure when setting a custom wallpaper.
Although the drivefs write should happen on a task runner with the
MayBlock trait the PrefService usage has to happeon on the task
runner it was created on.

Test: WallpaperControllerTests (added another too).
Test: By hand, setting custom wallpaper.
Bug: b/211055742
Change-Id: I87b70ecf196924f05b95d036fb184211980d38e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3346405
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Commit-Queue: Qasid Sadiq <qasid@google.com>
Cr-Commit-Position: refs/heads/main@{#955824}
This commit is contained in:
Qasid Sadiq
2022-01-05 19:24:16 +00:00
committed by Chromium LUCI CQ
parent 432d4c2fa6
commit 15a73b1b7b
8 changed files with 89 additions and 45 deletions

@ -45,9 +45,10 @@ class ASH_PUBLIC_EXPORT WallpaperControllerClient {
const std::string& collection_id,
DailyWallpaperUrlFetchedCallback callback) = 0;
// Returns true if image was successfully saved.
virtual bool SaveWallpaperToDriveFs(const AccountId& account_id,
const base::FilePath& origin) = 0;
virtual void SaveWallpaperToDriveFs(
const AccountId& account_id,
const base::FilePath& origin,
base::OnceCallback<void(bool)> wallpaper_saved_callback) = 0;
virtual base::FilePath GetWallpaperPathFromDriveFs(
const AccountId& account_id) = 0;

@ -91,11 +91,12 @@ void TestWallpaperControllerClient::FetchImagesForCollection(
}
}
bool TestWallpaperControllerClient::SaveWallpaperToDriveFs(
void TestWallpaperControllerClient::SaveWallpaperToDriveFs(
const AccountId& account_id,
const base::FilePath& origin) {
const base::FilePath& origin,
base::OnceCallback<void(bool)> wallpaper_saved_callback) {
save_wallpaper_to_drive_fs_account_id_ = account_id;
return true;
std::move(wallpaper_saved_callback).Run(true);
}
base::FilePath TestWallpaperControllerClient::GetWallpaperPathFromDriveFs(

@ -68,8 +68,10 @@ class TestWallpaperControllerClient : public WallpaperControllerClient {
void FetchImagesForCollection(
const std::string& collection_id,
FetchImagesForCollectionCallback callback) override;
bool SaveWallpaperToDriveFs(const AccountId& account_id,
const base::FilePath& origin) override;
void SaveWallpaperToDriveFs(
const AccountId& account_id,
const base::FilePath& origin,
base::OnceCallback<void(bool)> wallpaper_saved_callback) override;
base::FilePath GetWallpaperPathFromDriveFs(
const AccountId& account_id) override;
void GetFilesId(const AccountId& account_id,

@ -1140,8 +1140,9 @@ void WallpaperControllerImpl::SetCustomWallpaper(
SaveAndSetWallpaperWithCompletion(
account_id, file_name, WallpaperType::kCustomized, layout,
/*show_wallpaper=*/is_active_user, image,
base::BindOnce(&WallpaperControllerImpl::SaveWallpaperToDriveFs,
weak_factory_.GetWeakPtr(), account_id));
base::BindOnce(
&WallpaperControllerImpl::SaveWallpaperToDriveFsAndSyncInfo,
weak_factory_.GetWeakPtr(), account_id));
}
}
@ -1765,7 +1766,7 @@ void WallpaperControllerImpl::OnActiveUserPrefServiceChanged(
if (local_info.type == WallpaperType::kCustomized) {
base::FilePath source = GetCustomWallpaperDir(kOriginalWallpaperSubDir)
.Append(local_info.location);
SaveWallpaperToDriveFs(account_id, source);
SaveWallpaperToDriveFsAndSyncInfo(account_id, source);
} else {
SetSyncedWallpaperInfo(account_id, local_info);
wallpaper_controller_client_->MigrateCollectionIdFromChromeApp(
@ -2746,7 +2747,7 @@ void WallpaperControllerImpl::SyncLocalAndRemotePrefs(
// which may not be available at the time of setting the local_info.
base::FilePath source = GetCustomWallpaperDir(kOriginalWallpaperSubDir)
.Append(local_info.location);
SaveWallpaperToDriveFs(account_id, source);
SaveWallpaperToDriveFsAndSyncInfo(account_id, source);
}
}
@ -2857,7 +2858,7 @@ base::TimeDelta WallpaperControllerImpl::GetTimeToNextDailyRefreshUpdate()
base::Time::Now().ToDeltaSinceWindowsEpoch() + base::Days(1);
}
void WallpaperControllerImpl::SaveWallpaperToDriveFs(
void WallpaperControllerImpl::SaveWallpaperToDriveFsAndSyncInfo(
const AccountId& account_id,
const base::FilePath& origin_path) {
if (!features::IsWallpaperWebUIEnabled())
@ -2866,10 +2867,17 @@ void WallpaperControllerImpl::SaveWallpaperToDriveFs(
return;
if (!wallpaper_controller_client_->IsWallpaperSyncEnabled(account_id))
return;
if (!wallpaper_controller_client_->SaveWallpaperToDriveFs(account_id,
origin_path)) {
wallpaper_controller_client_->SaveWallpaperToDriveFs(
account_id, origin_path,
base::BindOnce(&WallpaperControllerImpl::WallpaperSavedToDriveFS,
weak_factory_.GetWeakPtr(), account_id));
}
void WallpaperControllerImpl::WallpaperSavedToDriveFS(
const AccountId& account_id,
bool success) {
if (!success)
return;
}
WallpaperInfo local_info;
CHECK(GetLocalWallpaperInfo(account_id, &local_info));
SetSyncedWallpaperInfo(account_id, local_info);

@ -665,8 +665,10 @@ class ASH_EXPORT WallpaperControllerImpl
// wallpaper set.
base::TimeDelta GetTimeToNextDailyRefreshUpdate() const;
void SaveWallpaperToDriveFs(const AccountId& account_id,
const base::FilePath& origin_path);
void SaveWallpaperToDriveFsAndSyncInfo(const AccountId& account_id,
const base::FilePath& origin_path);
void WallpaperSavedToDriveFS(const AccountId& account_id, bool success);
void HandleCustomWallpaperInfoSyncedIn(const AccountId& account_id,
WallpaperInfo info);

@ -3358,6 +3358,16 @@ TEST_F(WallpaperControllerWallpaperWebUiTest, SetWallpaperInfoCustom) {
}
TEST_F(WallpaperControllerWallpaperWebUiTest, MigrateWallpaperInfo) {
WallpaperInfo expected_info = InfoWithType(WallpaperType::kOnline);
PutWallpaperInfoInPrefs(account_id_1, expected_info, GetLocalPrefService(),
prefs::kUserWallpaperInfo);
SimulateUserLogin(account_id_1);
AssertWallpaperInfoInPrefs(GetProfilePrefService(account_id_1),
prefs::kSyncableWallpaperInfo, account_id_1,
expected_info);
}
TEST_F(WallpaperControllerWallpaperWebUiTest, MigrateWallpaperInfoCustomized) {
WallpaperInfo expected_info = InfoWithType(WallpaperType::kCustomized);
PutWallpaperInfoInPrefs(account_id_1, expected_info, GetLocalPrefService(),
prefs::kUserWallpaperInfo);

@ -22,6 +22,7 @@
#include "base/path_service.h"
#include "base/strings/string_number_conversions.h"
#include "base/task/sequenced_task_runner.h"
#include "base/task/thread_pool.h"
#include "base/threading/sequenced_task_runner_handle.h"
#include "base/unguessable_token.h"
#include "base/values.h"
@ -211,6 +212,34 @@ base::FilePath GetDriveFsWallpaperDir(Profile* profile) {
.Append(kDriveFsWallpaperDirName);
}
bool SaveWallpaperToDriveFsIOTaskRunner(
const base::FilePath& origin,
const base::FilePath& destination_directory) {
if (destination_directory.empty())
return false;
if (!base::DirectoryExists(destination_directory) &&
!base::CreateDirectory(destination_directory)) {
return false;
}
std::string temp_file_name =
base::UnguessableToken::Create().ToString().append(
kDriveFsTempWallpaperFileName);
base::FilePath temp_destination =
destination_directory.Append(temp_file_name);
if (!base::CopyFile(origin, temp_destination)) {
base::DeleteFile(temp_destination);
return false;
}
base::FilePath destination =
destination_directory.Append(kDriveFsWallpaperFileName);
bool success = base::ReplaceFile(temp_destination, destination, nullptr);
base::DeleteFile(temp_destination);
return success;
}
} // namespace
WallpaperControllerClientImpl::WallpaperControllerClientImpl() {
@ -229,6 +258,10 @@ WallpaperControllerClientImpl::WallpaperControllerClientImpl() {
// SessionManager might not exist in unit tests.
if (session_manager)
session_observation_.Observe(session_manager);
io_task_runner_ = base::ThreadPool::CreateSequencedTaskRunner(
{base::MayBlock(), base::TaskPriority::BEST_EFFORT,
base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN});
}
WallpaperControllerClientImpl::~WallpaperControllerClientImpl() {
@ -480,34 +513,17 @@ bool WallpaperControllerClientImpl::ShouldShowWallpaperSetting() {
return wallpaper_controller_->ShouldShowWallpaperSetting();
}
bool WallpaperControllerClientImpl::SaveWallpaperToDriveFs(
void WallpaperControllerClientImpl::SaveWallpaperToDriveFs(
const AccountId& account_id,
const base::FilePath& origin) {
const base::FilePath& origin,
base::OnceCallback<void(bool)> wallpaper_saved_callback) {
Profile* profile = ProfileHelper::Get()->GetProfileByAccountId(account_id);
base::FilePath destination_directory = GetDriveFsWallpaperDir(profile);
if (destination_directory.empty())
return false;
if (!base::DirectoryExists(destination_directory) &&
!base::CreateDirectory(destination_directory)) {
return false;
}
std::string temp_file_name =
base::UnguessableToken::Create().ToString().append(
kDriveFsTempWallpaperFileName);
base::FilePath temp_destination =
destination_directory.Append(temp_file_name);
if (!base::CopyFile(origin, temp_destination)) {
base::DeleteFile(temp_destination);
return false;
}
base::FilePath destination =
destination_directory.Append(kDriveFsWallpaperFileName);
bool success = base::ReplaceFile(temp_destination, destination, nullptr);
base::DeleteFile(temp_destination);
return success;
io_task_runner_->PostTaskAndReplyWithResult(
FROM_HERE,
base::BindOnce(&SaveWallpaperToDriveFsIOTaskRunner, origin,
destination_directory),
std::move(wallpaper_saved_callback));
}
base::FilePath WallpaperControllerClientImpl::GetWallpaperPathFromDriveFs(

@ -72,8 +72,10 @@ class WallpaperControllerClientImpl
void FetchImagesForCollection(
const std::string& collection_id,
FetchImagesForCollectionCallback callback) override;
bool SaveWallpaperToDriveFs(const AccountId& account_id,
const base::FilePath& origin) override;
void SaveWallpaperToDriveFs(
const AccountId& account_id,
const base::FilePath& origin,
base::OnceCallback<void(bool)> wallpaper_saved_callback) override;
base::FilePath GetWallpaperPathFromDriveFs(
const AccountId& account_id) override;
void GetFilesId(const AccountId& account_id,
@ -208,6 +210,8 @@ class WallpaperControllerClientImpl
session_manager::SessionManagerObserver>
session_observation_{this};
scoped_refptr<base::SequencedTaskRunner> io_task_runner_;
base::WeakPtrFactory<WallpaperControllerClientImpl> weak_factory_{this};
base::WeakPtrFactory<WallpaperControllerClientImpl> storage_weak_factory_{
this};