[104]Fix Use-After-Free in Persister
Fixed the use of weak factory pointers trying to access destroyed
objects on different threads. The fix involves using SequenceBound
on the object to ensure that the tasks the object are running and
the object itself are running on the same thread.
(cherry picked from commit a8c9672ab894bf904cef2ff5c18dd3e077456b2b)
Bug: 1335316
Change-Id: I3efa2c900aa6b0cf7bfcd7f92799c4fb4a3d20d8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3715835
Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Commit-Queue: Adam Psarouthakis <psarouthakis@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1018734}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3738780
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Xinghui Lu <xinghuilu@chromium.org>
Auto-Submit: Adam Psarouthakis <psarouthakis@google.com>
Cr-Commit-Position: refs/branch-heads/5112@{#788}
Cr-Branched-From: b13d3fe7b3c47a56354ef54b221008afa754412e-refs/heads/main@{#1012729}
diff --git a/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_persister.cc b/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_persister.cc
index 1580263..fff65bf 100644
--- a/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_persister.cc
+++ b/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_persister.cc
@@ -28,6 +28,7 @@
// The initial index for the `read_index_` and the `write_index_`.
constexpr int kInitialWriteIndex = 0;
constexpr int kInitialReadIndex = -1;
+constexpr char kPersistedFileNamePrefix[] = "CRXTelemetry_";
void RecordWriteResult(bool success) {
base::UmaHistogramBoolean("SafeBrowsing.ExtensionPersister.WriteResult",
@@ -61,63 +62,13 @@
ExtensionTelemetryPersister::~ExtensionTelemetryPersister() = default;
-ExtensionTelemetryPersister::ExtensionTelemetryPersister(int max_num_files,
- Profile* profile)
- : profile_(profile), max_num_files_(max_num_files) {
- // Shutdown behavior is CONTINUE_ON_SHUTDOWN to ensure tasks
- // are run on the threads they were called on.
- task_runner_ = base::ThreadPool::CreateSequencedTaskRunner(
- {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
- base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN});
-}
+ExtensionTelemetryPersister::ExtensionTelemetryPersister(
+ int max_num_files,
+ base::FilePath profile_path)
+ : dir_path_(profile_path), max_num_files_(max_num_files) {}
void ExtensionTelemetryPersister::PersisterInit() {
- base::ThreadPool::PostTask(
- FROM_HERE, base::MayBlock(),
- base::BindOnce(&ExtensionTelemetryPersister::InitHelper,
- weak_factory_.GetWeakPtr()));
-}
-
-void ExtensionTelemetryPersister::WriteReport(const std::string write_string) {
- if (initialization_complete_ && !is_shut_down_) {
- task_runner_->PostTask(
- FROM_HERE,
- base::BindOnce(&ExtensionTelemetryPersister::SaveFile,
- weak_factory_.GetWeakPtr(), std::move(write_string)));
- }
-}
-
-void ExtensionTelemetryPersister::WriteReportDuringShutdown(
- const std::string write_string) {
- if (initialization_complete_ && !is_shut_down_) {
- is_shut_down_ = true;
- task_runner_->PostTask(
- FROM_HERE,
- base::BindOnce(&ExtensionTelemetryPersister::SaveFileDuringShutdown,
- std::move(write_string), dir_path_, write_index_));
- }
-}
-
-void ExtensionTelemetryPersister::ReadReport(
- base::OnceCallback<void(std::string, bool)> callback) {
- if (initialization_complete_ && read_index_ > kInitialReadIndex &&
- !is_shut_down_) {
- task_runner_->PostTask(
- FROM_HERE,
- base::BindOnce(&ExtensionTelemetryPersister::LoadFile,
- weak_factory_.GetWeakPtr(), std::move(callback)));
- } else {
- std::move(callback).Run("", false);
- }
-}
-
-void ExtensionTelemetryPersister::ClearPersistedFiles() {
- task_runner_->PostTask(
- FROM_HERE, base::BindOnce(&ExtensionTelemetryPersister::DeleteAllFiles,
- weak_factory_.GetWeakPtr()));
-}
-
-void ExtensionTelemetryPersister::InitHelper() {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
// TODO(https://crbug.com/1325864): Remove old directory clean up code after
// launch.
base::FilePath old_dir;
@@ -129,13 +80,13 @@
}
write_index_ = kInitialWriteIndex;
read_index_ = kInitialReadIndex;
- dir_path_ = profile_->GetPath();
dir_path_ = dir_path_.AppendASCII("CRXTelemetry");
if (!base::DirectoryExists(dir_path_))
base::CreateDirectory(dir_path_);
- while (base::PathExists(dir_path_.AppendASCII(
- ("CRXTelemetry_" + base::NumberToString(read_index_ + 1)))) &&
- (read_index_ < max_num_files_ - 1)) {
+ while (
+ base::PathExists(dir_path_.AppendASCII((
+ kPersistedFileNamePrefix + base::NumberToString(read_index_ + 1)))) &&
+ (read_index_ < max_num_files_ - 1)) {
read_index_++;
}
write_index_ = (read_index_ + 1) % max_num_files_;
@@ -143,47 +94,38 @@
RecordNumberOfFilesInCacheOnStartup(read_index_ + 1);
}
-// Static
-void ExtensionTelemetryPersister::SaveFileDuringShutdown(
- std::string write_string,
- base::FilePath dir_path,
- int write_index) {
- // If the persister directory does not exist the persister
- // will not write.
- if (!base::DirectoryExists(dir_path))
- return;
- base::FilePath path = dir_path.AppendASCII(
- ("CRXTelemetry_" + base::NumberToString(write_index)));
- bool success = base::WriteFile(path, write_string);
- RecordWriteResult(success);
+void ExtensionTelemetryPersister::WriteReport(const std::string write_string) {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+ if (initialization_complete_) {
+ if (!base::DirectoryExists(dir_path_)) {
+ base::CreateDirectory(dir_path_);
+ write_index_ = kInitialWriteIndex;
+ read_index_ = kInitialReadIndex;
+ }
+ base::FilePath path = dir_path_.AppendASCII(
+ (kPersistedFileNamePrefix + base::NumberToString(write_index_)));
+ bool success = base::WriteFile(path, write_string);
+ if (success) {
+ write_index_++;
+ if (write_index_ - 1 > read_index_)
+ read_index_ = write_index_ - 1;
+ if (write_index_ >= max_num_files_)
+ write_index_ = 0;
+ }
+ RecordWriteResult(success);
+ }
}
-void ExtensionTelemetryPersister::SaveFile(std::string write_string) {
- if (!base::DirectoryExists(dir_path_)) {
- base::CreateDirectory(dir_path_);
- write_index_ = kInitialWriteIndex;
- read_index_ = kInitialReadIndex;
+std::string ExtensionTelemetryPersister::ReadReport() {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+ if (!initialization_complete_ || read_index_ <= kInitialReadIndex) {
+ return std::string();
}
- base::FilePath path = dir_path_.AppendASCII(
- ("CRXTelemetry_" + base::NumberToString(write_index_)));
- bool success = base::WriteFile(path, write_string);
- if (success) {
- write_index_++;
- if (write_index_ - 1 > read_index_)
- read_index_ = write_index_ - 1;
- if (write_index_ >= max_num_files_)
- write_index_ = 0;
- }
- RecordWriteResult(success);
-}
-
-void ExtensionTelemetryPersister::LoadFile(
- base::OnceCallback<void(std::string, bool)> callback) {
bool read_success = false;
- std::string persisted_report = "";
+ std::string persisted_report;
base::File::Info info;
base::FilePath path = dir_path_.AppendASCII(
- ("CRXTelemetry_" + base::NumberToString(read_index_)));
+ (kPersistedFileNamePrefix + base::NumberToString(read_index_)));
// Check file to see if it's older than `kMaxFileAge`,
// if so, delete it and look for another file.
while (base::PathExists(path) && base::DirectoryExists(dir_path_)) {
@@ -205,20 +147,21 @@
DeleteFile(path);
RecordAgedFileFound(true);
path = dir_path_.AppendASCII(
- ("CRXTelemetry_" + base::NumberToString(read_index_)));
+ (kPersistedFileNamePrefix + base::NumberToString(read_index_)));
}
RecordReadResult(read_success);
- std::move(callback).Run(std::move(persisted_report), std::move(read_success));
+ return persisted_report;
}
-bool ExtensionTelemetryPersister::DeleteFile(const base::FilePath path) {
- return (base::PathExists(path) && base::DeleteFile(path));
-}
-
-void ExtensionTelemetryPersister::DeleteAllFiles() {
+void ExtensionTelemetryPersister::ClearPersistedFiles() {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
write_index_ = kInitialWriteIndex;
read_index_ = kInitialReadIndex;
base::DeletePathRecursively(dir_path_);
}
+bool ExtensionTelemetryPersister::DeleteFile(const base::FilePath path) {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+ return (base::PathExists(path) && base::DeleteFile(path));
+}
} // namespace safe_browsing
diff --git a/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_persister.h b/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_persister.h
index 6c90727..f8a00511 100644
--- a/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_persister.h
+++ b/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_persister.h
@@ -9,8 +9,7 @@
#include "base/files/file_util.h"
#include "base/memory/weak_ptr.h"
-
-class Profile;
+#include "base/sequence_checker.h"
namespace base {
class FilePath;
@@ -19,10 +18,16 @@
// The `ExtensionTelemetryPersister` stores data collected by the Extension
// Telemetry Service to disk. It creates files until `max_num_files_` are on
-// disk and then starts overwriting previously made files.
+// disk and then starts overwriting previously made files. The Persister runs
+// read and write functions that are time intensive. To avoid blocking
+// important threads, the persister should be created using SequenceBound.
class ExtensionTelemetryPersister {
public:
- explicit ExtensionTelemetryPersister(int max_num_files, Profile* profile);
+ // The `profile_path` is used to construct where the persister saves it's
+ // files. The persister creates a directory under profile_path/CRX_Telemetry
+ // and saves files there.
+ explicit ExtensionTelemetryPersister(int max_num_files,
+ base::FilePath profile_path);
ExtensionTelemetryPersister(const ExtensionTelemetryPersister&) = delete;
ExtensionTelemetryPersister& operator=(const ExtensionTelemetryPersister&) =
@@ -38,57 +43,26 @@
// exposed to the caller and is determined by `write_index_`.
void WriteReport(const std::string write_string);
- // Caller should use this method exactly once to write a telemetry report to
- // disk during Chrome/Profile shutdown. The persister object should not be
- // used after calling this method and should be destroyed.
- void WriteReportDuringShutdown(const std::string write_string);
-
// Reads a telemetry report from a file on disk. The file is deleted
// regardless of if the read was successful or not. The filename
- // is not exposed to the caller. The callback passes back the result
- // of the read operation and the contents of the report if the read succeeded.
- // The callback is expected to be bound to the thread it needs to run on.
- void ReadReport(base::OnceCallback<void(std::string, bool)> callback);
+ // is not exposed to the caller. Returns a string representation
+ // of the read file, or an empty string if the read failed.
+ std::string ReadReport();
- // Deletes the CRXTelemetry folder by calling DeleteAllFiles.
+ // Deletes the CRXTelemetry directory.
void ClearPersistedFiles();
private:
- // A helper function of PersisterInit allowing it post actions to the
- // thread pool.
- void InitHelper();
-
- // Writes data to the file represented by `write_index_`.
- void SaveFile(std::string write_string);
-
- // Writes data during a profile or Chrome shutdown. Persister
- // tasks run on the threadpool but it's destructor runs on the
- // main UI thread. This function is static to prevent threading
- // errors when the persister's destructor and posted task execute
- // at the same time but on different threads.
- static void SaveFileDuringShutdown(std::string write_string,
- base::FilePath dir_path,
- int write_index);
-
- // Reads data from the file represented by `read_index_`. When a
- // file is read it is also deleted.
- void LoadFile(base::OnceCallback<void(std::string, bool)> callback);
-
// Deletes the file that the `path` variable points to.
// Returns true if the file is deleted, false otherwise.
bool DeleteFile(const base::FilePath path);
// Deletes the CRXTelemetry directory.
- void DeleteAllFiles();
-
friend class ExtensionTelemetryPersisterTest;
- // Stores the directory path of the user's profile.
+ // Stores the directory path where the telemetry files are persisted.
base::FilePath dir_path_;
- // The profile with which this instance of the persister is associated.
- const raw_ptr<Profile> profile_;
-
// The maximum number of files that are stored on disk.
int max_num_files_;
@@ -105,16 +79,7 @@
// persister is done initializing.
bool initialization_complete_ = false;
- // Ensures once the persister has run it's shutdown write function
- // the persister will not post any other tasks.
- bool is_shut_down_ = false;
-
- // Task runner for read and write operations. Shutdown behavior
- // is to complete tasks that have started. There is a chance that if Chrome
- // is performing a lot of tasks during shutdown, persister tasks might
- // not run. This can most likely occur when Chrome is open and closed
- // very quickly. scoped_refptr<base::SequencedTaskRunner> task_runner_;
- scoped_refptr<base::SequencedTaskRunner> task_runner_;
+ SEQUENCE_CHECKER(sequence_checker_);
base::WeakPtrFactory<ExtensionTelemetryPersister> weak_factory_{this};
};
diff --git a/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_persister_unittest.cc b/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_persister_unittest.cc
index 3779055..ccb3dc71 100644
--- a/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_persister_unittest.cc
+++ b/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_persister_unittest.cc
@@ -4,12 +4,12 @@
#include "chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_persister.h"
#include <sstream>
-
-#include "base/callback.h"
#include "base/files/file_util.h"
#include "base/logging.h"
#include "base/path_service.h"
#include "base/task/bind_post_task.h"
+#include "base/task/thread_pool.h"
+#include "base/threading/sequence_bound.h"
#include "base/threading/sequenced_task_runner_handle.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/common/chrome_paths.h"
@@ -17,178 +17,169 @@
#include "content/public/browser/browser_task_traits.h"
#include "content/public/test/browser_task_environment.h"
#include "testing/gtest/include/gtest/gtest.h"
+
namespace safe_browsing {
class ExtensionTelemetryPersisterTest : public ::testing::Test {
public:
ExtensionTelemetryPersisterTest();
- void SetUp() override { ASSERT_NE(persister_, nullptr); }
+ void SetUp() override { ASSERT_EQ(persister_.is_null(), false); }
- std::unique_ptr<safe_browsing::ExtensionTelemetryPersister> persister_;
+ void CallbackHelper(std::string read) { read_string_ = read; }
- void CallbackHelper(std::string read, bool success) {
- read_string_ = read;
- success_ = success;
+ void TearDown() override {
+ persister_.SynchronouslyResetForTest();
+ testing::Test::TearDown();
}
+ base::SequenceBound<safe_browsing::ExtensionTelemetryPersister> persister_;
int kMaxNumFilesPersisted = 10;
content::BrowserTaskEnvironment task_environment_;
TestingProfile profile_;
- bool success_;
std::string read_string_;
base::WeakPtrFactory<ExtensionTelemetryPersisterTest> weak_factory_{this};
};
ExtensionTelemetryPersisterTest::ExtensionTelemetryPersisterTest()
: task_environment_(base::test::TaskEnvironment::TimeSource::MOCK_TIME) {
- persister_ = std::make_unique<ExtensionTelemetryPersister>(
- kMaxNumFilesPersisted, &profile_);
- persister_->PersisterInit();
+ persister_ = base::SequenceBound<ExtensionTelemetryPersister>(
+ base::ThreadPool::CreateSequencedTaskRunner(
+ {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
+ base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN}),
+ kMaxNumFilesPersisted, profile_.GetPath());
+ persister_.AsyncCall(&ExtensionTelemetryPersister::PersisterInit);
task_environment_.RunUntilIdle();
}
TEST_F(ExtensionTelemetryPersisterTest, WriteReadCheck) {
std::string written_string = "Test String 1";
- persister_->WriteReport(written_string);
- task_environment_.RunUntilIdle();
- auto callback = base::BindPostTask(
- base::SequencedTaskRunnerHandle::Get(),
+ persister_.AsyncCall(&ExtensionTelemetryPersister::WriteReport)
+ .WithArgs(written_string);
+ auto callback =
base::BindOnce(&ExtensionTelemetryPersisterTest::CallbackHelper,
- weak_factory_.GetWeakPtr()));
- // Read report and check its contents.
- persister_->ReadReport(std::move(callback));
+ weak_factory_.GetWeakPtr());
+ persister_.AsyncCall(&ExtensionTelemetryPersister::ReadReport)
+ .Then(std::move(callback));
task_environment_.RunUntilIdle();
- EXPECT_EQ(success_, true);
EXPECT_EQ(written_string, read_string_);
- persister_->ClearPersistedFiles();
- task_environment_.RunUntilIdle();
}
TEST_F(ExtensionTelemetryPersisterTest, WritePastFullCacheCheck) {
read_string_ = "No File was read";
std::string written_string = "Test String 1";
for (int i = 0; i < 12; i++) {
- persister_->WriteReport(written_string);
- task_environment_.RunUntilIdle();
+ persister_.AsyncCall(&ExtensionTelemetryPersister::WriteReport)
+ .WithArgs(written_string);
}
- persister_->ClearPersistedFiles();
- task_environment_.RunUntilIdle();
+ persister_.AsyncCall(&ExtensionTelemetryPersister::ClearPersistedFiles);
// After a clear no file should be there to read from.
- auto callback = base::BindPostTask(
- base::SequencedTaskRunnerHandle::Get(),
+ auto callback =
base::BindOnce(&ExtensionTelemetryPersisterTest::CallbackHelper,
- weak_factory_.GetWeakPtr()));
- persister_->ReadReport(std::move(callback));
+ weak_factory_.GetWeakPtr());
+ persister_.AsyncCall(&ExtensionTelemetryPersister::ReadReport)
+ .Then(std::move(callback));
task_environment_.RunUntilIdle();
EXPECT_NE(written_string, read_string_);
- persister_->ClearPersistedFiles();
- task_environment_.RunUntilIdle();
}
TEST_F(ExtensionTelemetryPersisterTest, ReadFullCache) {
// Fully load persisted cache.
std::string written_string = "Test String 1";
for (int i = 0; i < 10; i++) {
- persister_->WriteReport(written_string);
- task_environment_.RunUntilIdle();
+ persister_.AsyncCall(&ExtensionTelemetryPersister::WriteReport)
+ .WithArgs(written_string);
}
written_string = "Test String 2";
// Overwrite first 5 files.
for (int i = 0; i < 5; i++) {
- persister_->WriteReport(written_string);
- task_environment_.RunUntilIdle();
+ persister_.AsyncCall(&ExtensionTelemetryPersister::WriteReport)
+ .WithArgs(written_string);
}
// Read should start at highest numbered file.
for (int i = 0; i < 5; i++) {
- auto callback = base::BindPostTask(
- base::SequencedTaskRunnerHandle::Get(),
+ auto callback =
base::BindOnce(&ExtensionTelemetryPersisterTest::CallbackHelper,
- weak_factory_.GetWeakPtr()));
- persister_->ReadReport(std::move(callback));
+ weak_factory_.GetWeakPtr());
+ persister_.AsyncCall(&ExtensionTelemetryPersister::ReadReport)
+ .Then(std::move(callback));
task_environment_.RunUntilIdle();
- EXPECT_EQ(success_, true);
EXPECT_EQ("Test String 1", read_string_);
}
// Files 0-4 should be different.
for (int i = 0; i < 5; i++) {
- auto callback = base::BindPostTask(
- base::SequencedTaskRunnerHandle::Get(),
+ auto callback =
base::BindOnce(&ExtensionTelemetryPersisterTest::CallbackHelper,
- weak_factory_.GetWeakPtr()));
- persister_->ReadReport(std::move(callback));
+ weak_factory_.GetWeakPtr());
+ persister_.AsyncCall(&ExtensionTelemetryPersister::ReadReport)
+ .Then(std::move(callback));
task_environment_.RunUntilIdle();
- EXPECT_EQ(success_, true);
EXPECT_EQ("Test String 2", read_string_);
}
- auto callback = base::BindPostTask(
- base::SequencedTaskRunnerHandle::Get(),
+ auto callback =
base::BindOnce(&ExtensionTelemetryPersisterTest::CallbackHelper,
- weak_factory_.GetWeakPtr()));
- persister_->ReadReport(std::move(callback));
+ weak_factory_.GetWeakPtr());
+ persister_.AsyncCall(&ExtensionTelemetryPersister::ReadReport)
+ .Then(std::move(callback));
task_environment_.RunUntilIdle();
// Last read should not happen as all files have been read.
- EXPECT_EQ(success_, false);
- persister_->ClearPersistedFiles();
- task_environment_.RunUntilIdle();
+ EXPECT_EQ("", read_string_);
}
+
TEST_F(ExtensionTelemetryPersisterTest, MultiProfile) {
TestingProfile profile_2;
- std::unique_ptr<safe_browsing::ExtensionTelemetryPersister> persister_2 =
- std::make_unique<ExtensionTelemetryPersister>(kMaxNumFilesPersisted,
- &profile_2);
- persister_2->PersisterInit();
+ base::SequenceBound<safe_browsing::ExtensionTelemetryPersister> persister_2 =
+ base::SequenceBound<ExtensionTelemetryPersister>(
+ base::ThreadPool::CreateSequencedTaskRunner(
+ {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
+ base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN}),
+ kMaxNumFilesPersisted, profile_2.GetPath());
+ persister_2.AsyncCall(&ExtensionTelemetryPersister::PersisterInit);
task_environment_.RunUntilIdle();
// Perform a simple read write test on two separate profiles.
std::string written_string = "Test String 1";
std::string written_string_2 = "Test String 2";
- persister_->WriteReport(written_string);
- persister_->WriteReport(written_string);
- persister_2->WriteReport(written_string_2);
- persister_2->WriteReport(written_string_2);
- task_environment_.RunUntilIdle();
+ persister_.AsyncCall(&ExtensionTelemetryPersister::WriteReport)
+ .WithArgs(written_string);
+ persister_.AsyncCall(&ExtensionTelemetryPersister::WriteReport)
+ .WithArgs(written_string);
+ persister_2.AsyncCall(&ExtensionTelemetryPersister::WriteReport)
+ .WithArgs(written_string_2);
+ persister_2.AsyncCall(&ExtensionTelemetryPersister::WriteReport)
+ .WithArgs(written_string_2);
// Read through profile one persisted files
for (int i = 0; i < 2; i++) {
- auto callback = base::BindPostTask(
- base::SequencedTaskRunnerHandle::Get(),
+ auto callback =
base::BindOnce(&ExtensionTelemetryPersisterTest::CallbackHelper,
- weak_factory_.GetWeakPtr()));
- // Read report and check its contents.
- persister_->ReadReport(std::move(callback));
+ weak_factory_.GetWeakPtr());
+ persister_.AsyncCall(&ExtensionTelemetryPersister::ReadReport)
+ .Then(std::move(callback));
task_environment_.RunUntilIdle();
- EXPECT_EQ(success_, true);
EXPECT_EQ(written_string, read_string_);
}
- // Last file read should fail since two files were written per file.
- auto callback = base::BindPostTask(
- base::SequencedTaskRunnerHandle::Get(),
+ // Last file read should fail since two files were written per profile.
+ auto callback =
base::BindOnce(&ExtensionTelemetryPersisterTest::CallbackHelper,
- weak_factory_.GetWeakPtr()));
- persister_->ReadReport(std::move(callback));
+ weak_factory_.GetWeakPtr());
+ persister_.AsyncCall(&ExtensionTelemetryPersister::ReadReport)
+ .Then(std::move(callback));
task_environment_.RunUntilIdle();
- EXPECT_EQ(success_, false);
EXPECT_EQ("", read_string_);
// Repeat process for profile 2.
for (int i = 0; i < 2; i++) {
- auto callback = base::BindPostTask(
- base::SequencedTaskRunnerHandle::Get(),
+ auto callback2 =
base::BindOnce(&ExtensionTelemetryPersisterTest::CallbackHelper,
- weak_factory_.GetWeakPtr()));
- // Read report and check its contents.
- persister_2->ReadReport(std::move(callback));
+ weak_factory_.GetWeakPtr());
+ persister_2.AsyncCall(&ExtensionTelemetryPersister::ReadReport)
+ .Then(std::move(callback2));
task_environment_.RunUntilIdle();
- EXPECT_EQ(success_, true);
EXPECT_EQ(written_string_2, read_string_);
}
- callback = base::BindPostTask(
- base::SequencedTaskRunnerHandle::Get(),
+ auto callback2 =
base::BindOnce(&ExtensionTelemetryPersisterTest::CallbackHelper,
- weak_factory_.GetWeakPtr()));
- persister_2->ReadReport(std::move(callback));
+ weak_factory_.GetWeakPtr());
+ persister_2.AsyncCall(&ExtensionTelemetryPersister::ReadReport)
+ .Then(std::move(callback2));
task_environment_.RunUntilIdle();
- EXPECT_EQ(success_, false);
EXPECT_EQ("", read_string_);
- persister_2->ClearPersistedFiles();
- persister_->ClearPersistedFiles();
- task_environment_.RunUntilIdle();
}
} // namespace safe_browsing
diff --git a/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service.cc b/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service.cc
index cf5c0b7..e880563 100644
--- a/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service.cc
+++ b/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service.cc
@@ -14,6 +14,8 @@
#include "base/strings/string_number_conversions.h"
#include "base/strings/utf_string_conversions.h"
#include "base/task/bind_post_task.h"
+#include "base/task/thread_pool.h"
+#include "base/threading/sequence_bound.h"
#include "base/time/time.h"
#include "base/timer/timer.h"
#include "chrome/browser/extensions/extension_management.h"
@@ -30,6 +32,7 @@
#include "components/safe_browsing/core/common/features.h"
#include "components/safe_browsing/core/common/proto/csd.pb.h"
#include "components/safe_browsing/core/common/safe_browsing_prefs.h"
+#include "content/public/browser/browser_task_traits.h"
#include "content/public/browser/browser_thread.h"
#include "extensions/browser/blocklist_extension_prefs.h"
#include "extensions/browser/disable_reason.h"
@@ -194,9 +197,12 @@
// Instantiate persister which is used to read/write telemetry reports to
// disk and start timer for sending periodic telemetry reports.
if (base::FeatureList::IsEnabled(kExtensionTelemetryPersistence)) {
- persister_ = std::make_unique<ExtensionTelemetryPersister>(
- kMaxNumFilesPersisted, profile_);
- persister_->PersisterInit();
+ persister_ = base::SequenceBound<ExtensionTelemetryPersister>(
+ base::ThreadPool::CreateSequencedTaskRunner(
+ {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
+ base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN}),
+ kMaxNumFilesPersisted, profile_->GetPath());
+ persister_.AsyncCall(&ExtensionTelemetryPersister::PersisterInit);
int writes_per_interval = kExtensionTelemetryWritesPerInterval.Get();
// Ensure that the `writes_per_interval` is never larger than the
// persister cache or smaller than 1.
@@ -231,8 +237,8 @@
signal_processors_.clear();
// Delete persisted files.
if (base::FeatureList::IsEnabled(kExtensionTelemetryPersistence) &&
- persister_) {
- persister_->ClearPersistedFiles();
+ !persister_.is_null()) {
+ persister_.AsyncCall(&ExtensionTelemetryPersister::ClearPersistedFiles);
}
}
}
@@ -240,12 +246,13 @@
void ExtensionTelemetryService::Shutdown() {
if (enabled_ &&
base::FeatureList::IsEnabled(kExtensionTelemetryPersistence) &&
- SignalDataPresent() && persister_) {
+ SignalDataPresent() && !persister_.is_null()) {
// Saving data to disk.
active_report_ = CreateReport();
std::string write_string;
active_report_->SerializeToString(&write_string);
- persister_->WriteReportDuringShutdown(std::move(write_string));
+ persister_.AsyncCall(&ExtensionTelemetryPersister::WriteReport)
+ .WithArgs(std::move(write_string));
}
timer_.Stop();
pref_change_registrar_.RemoveAll();
@@ -299,21 +306,22 @@
void ExtensionTelemetryService::OnUploadComplete(bool success) {
if (base::FeatureList::IsEnabled(kExtensionTelemetryPersistence) &&
- enabled_ && persister_) {
+ enabled_ && !persister_.is_null()) {
// Upload saved report(s) if there are any.
if (success) {
// Bind the callback to our current thread.
- auto read_callback = base::BindPostTask(
- base::SequencedTaskRunnerHandle::Get(),
+ auto read_callback =
base::BindOnce(&ExtensionTelemetryService::UploadPersistedFile,
- weak_factory_.GetWeakPtr()));
- persister_->ReadReport(std::move(read_callback));
+ weak_factory_.GetWeakPtr());
+ persister_.AsyncCall(&ExtensionTelemetryPersister::ReadReport)
+ .Then(std::move(read_callback));
SetLastUploadTimeForExtensionTelemetry(*pref_service_, base::Time::Now());
} else {
// Save report to disk on a failed upload.
std::string write_string;
active_report_->SerializeToString(&write_string);
- persister_->WriteReport(std::move(write_string));
+ persister_.AsyncCall(&ExtensionTelemetryPersister::WriteReport)
+ .WithArgs(std::move(write_string));
active_uploader_.reset();
active_report_.reset();
}
@@ -323,11 +331,10 @@
}
}
-void ExtensionTelemetryService::UploadPersistedFile(std::string report,
- bool success) {
+void ExtensionTelemetryService::UploadPersistedFile(std::string report) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
- auto upload_data = std::make_unique<std::string>(report);
- if (success) {
+ if (!report.empty()) {
+ auto upload_data = std::make_unique<std::string>(report);
if (!active_report_->SerializeToString(upload_data.get())) {
active_report_.reset();
return;
@@ -375,7 +382,8 @@
active_report_.reset();
return;
}
- persister_->WriteReport(std::move(write_string));
+ persister_.AsyncCall(&ExtensionTelemetryPersister::WriteReport)
+ .WithArgs(std::move(write_string));
}
}
diff --git a/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service.h b/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service.h
index 1031065..73c68c4 100644
--- a/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service.h
+++ b/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service.h
@@ -12,6 +12,7 @@
#include "base/feature_list.h"
#include "base/memory/raw_ptr.h"
#include "base/memory/weak_ptr.h"
+#include "base/threading/sequence_bound.h"
#include "base/time/time.h"
#include "base/timer/timer.h"
#include "components/keyed_service/core/keyed_service.h"
@@ -106,7 +107,7 @@
std::unique_ptr<ExtensionTelemetryReportRequest_ExtensionInfo>
GetExtensionInfoForReport(const extensions::Extension& extension);
- void UploadPersistedFile(std::string report, bool success);
+ void UploadPersistedFile(std::string report);
// Creates access token fetcher based on profile log-in status.
// Returns nullptr when the user is not signed in.
@@ -124,7 +125,11 @@
// uploads telemetry data. Runs on a delayed post task on startup.
void StartUploadCheck();
- std::unique_ptr<safe_browsing::ExtensionTelemetryPersister> persister_;
+ // The persister object is bound to the threadpool. This prevents the
+ // the read/write operations the `persister_` runs from blocking
+ // the UI thread. It also allows the `persister_` object to be
+ // destroyed cleanly while running tasks during Chrome shutdown.
+ base::SequenceBound<ExtensionTelemetryPersister> persister_;
// The profile with which this instance of the service is associated.
const raw_ptr<Profile> profile_;
diff --git a/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service_unittest.cc b/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service_unittest.cc
index caa9079..14a97039 100644
--- a/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service_unittest.cc
+++ b/chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service_unittest.cc
@@ -505,4 +505,13 @@
EXPECT_EQ(info, nullptr);
}
}
+TEST_F(ExtensionTelemetryServiceTest, PersisterThreadSafetyCheck) {
+ scoped_feature_list.InitAndEnableFeature(kExtensionTelemetryPersistence);
+ std::unique_ptr<ExtensionTelemetryService> telemetry_service_2 =
+ std::make_unique<ExtensionTelemetryService>(
+ &profile_, test_url_loader_factory_.GetSafeWeakWrapper(),
+ extension_registry_, extension_prefs_);
+ telemetry_service_2->SetEnabled(true);
+ telemetry_service_2.reset();
+}
} // namespace safe_browsing