[M87 merge] ambient: fix dcheck blocking error saving backup photos
Perform disk operations in SequencedTaskRunner
BUG=b:170346308
TEST=activate ambient mode, lock screen
(cherry picked from commit ddadb4220d53154ac339a1e34954dc129c5e0089)
Change-Id: I5faefd46e74e6ec8ae7061d56e7f6abd0c80326f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2460796
Commit-Queue: Jeffrey Young <[email protected]>
Reviewed-by: Xiaohui Chen <[email protected]>
Reviewed-by: Tao Wu <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#815959}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2464789
Cr-Commit-Position: refs/branch-heads/4280@{#301}
Cr-Branched-From: ea420fb963f9658c9969b6513c56b8f47efa1a2a-refs/heads/master@{#812852}
diff --git a/ash/ambient/ambient_photo_controller.cc b/ash/ambient/ambient_photo_controller.cc
index 797ba5a..7321caa 100644
--- a/ash/ambient/ambient_photo_controller.cc
+++ b/ash/ambient/ambient_photo_controller.cc
@@ -36,6 +36,7 @@
#include "base/task/thread_pool.h"
#include "base/task_runner_util.h"
#include "base/threading/sequenced_task_runner_handle.h"
+#include "base/unguessable_token.h"
#include "net/traffic_annotation/network_traffic_annotation.h"
#include "services/data_decoder/public/cpp/decode_image.h"
#include "services/network/public/cpp/resource_request.h"
@@ -170,7 +171,10 @@
class AmbientURLLoaderImpl : public AmbientURLLoader {
public:
- AmbientURLLoaderImpl() = default;
+ AmbientURLLoaderImpl()
+ : task_runner_(base::ThreadPool::CreateSequencedTaskRunner(
+ {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
+ base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})) {}
~AmbientURLLoaderImpl() override = default;
// AmbientURLLoader:
@@ -195,24 +199,21 @@
auto simple_loader = CreateSimpleURLLoader(url);
auto loader_factory = AmbientClient::Get()->GetURLLoaderFactory();
auto* loader_ptr = simple_loader.get();
+
+ // Create a temporary file path as target for download to guard against race
+ // conditions in reading.
+ base::FilePath temp_path = file_path.DirName().Append(
+ base::UnguessableToken::Create().ToString() + kPhotoFileExt);
+
// Download to temp file first to guarantee entire image is written without
// errors before attempting to read it.
-
- // Create a temp file.
- base::FilePath temp_file;
- if (!base::CreateTemporaryFileInDir(file_path.DirName(), &temp_file)) {
- LOG(ERROR) << "Cannot create a temporary file";
- std::move(callback).Run(base::FilePath());
- return;
- }
-
loader_ptr->DownloadToFile(
loader_factory.get(),
base::BindOnce(&AmbientURLLoaderImpl::OnUrlDownloadedToFile,
weak_factory_.GetWeakPtr(), std::move(callback),
std::move(simple_loader), std::move(loader_factory),
file_path),
- temp_file);
+ temp_path);
}
private:
@@ -254,15 +255,37 @@
LOG(ERROR) << "Downloading to file failed with error code: "
<< GetResponseCode(simple_loader.get())
<< " with network error" << simple_loader->NetError();
+
+ if (!temp_path.empty()) {
+ // Clean up temporary file.
+ task_runner_->PostTask(FROM_HERE, base::BindOnce(
+ [](const base::FilePath& path) {
+ base::DeleteFile(path);
+ },
+ temp_path));
+ }
std::move(callback).Run(base::FilePath());
return;
}
- if (!base::ReplaceFile(temp_path, desired_path, /*error=*/nullptr)) {
- LOG(ERROR) << "Unable to move downloaded file to ambient directory";
- std::move(callback).Run(base::FilePath());
- return;
- }
- std::move(callback).Run(std::move(desired_path));
+
+ // Swap the temporary file to the desired path, and then run the callback.
+ task_runner_->PostTaskAndReplyWithResult(
+ FROM_HERE,
+ base::BindOnce(
+ [](const base::FilePath& to_path, const base::FilePath& from_path) {
+ if (!base::ReplaceFile(from_path, to_path,
+ /*error=*/nullptr)) {
+ LOG(ERROR)
+ << "Unable to move downloaded file to ambient directory";
+ // Clean up the files.
+ base::DeleteFile(from_path);
+ base::DeleteFile(to_path);
+ return base::FilePath();
+ }
+ return to_path;
+ },
+ desired_path, temp_path),
+ std::move(callback));
}
int GetResponseCode(network::SimpleURLLoader* simple_loader) {
@@ -274,6 +297,7 @@
}
}
+ scoped_refptr<base::SequencedTaskRunner> task_runner_;
base::WeakPtrFactory<AmbientURLLoaderImpl> weak_factory_{this};
};
