From fc46ac15bce89405e0bfff3e246be0111f466318 Mon Sep 17 00:00:00 2001 From: proberge Date: Wed, 21 Sep 2016 11:03:00 -0700 Subject: [PATCH] Add synchronous callback support to important_file_writer.cc This is required to support post-write callbacks which need to execute even if Chrome is shutting down. Carved out of https://codereview.chromium.org/2204943002/, new call will be used there. BUG=624858 Review-Url: https://codereview.chromium.org/2299523003 Cr-Commit-Position: refs/heads/master@{#420102} --- base/files/important_file_writer.cc | 50 ++-- base/files/important_file_writer.h | 24 +- base/files/important_file_writer_unittest.cc | 112 ++++++--- .../prefs/profile_pref_store_manager.cc | 4 +- components/prefs/json_pref_store.cc | 67 ++++- components/prefs/json_pref_store.h | 33 ++- components/prefs/json_pref_store_unittest.cc | 234 +++++++++++++++++- 7 files changed, 427 insertions(+), 97 deletions(-) diff --git a/base/files/important_file_writer.cc b/base/files/important_file_writer.cc index edd400ce183f13..0099cda0b55a8b 100644 --- a/base/files/important_file_writer.cc +++ b/base/files/important_file_writer.cc @@ -57,9 +57,13 @@ void LogFailure(const FilePath& path, TempFileFailure failure_code, // Helper function to call WriteFileAtomically() with a // std::unique_ptr. -bool WriteScopedStringToFileAtomically(const FilePath& path, - std::unique_ptr data) { - return ImportantFileWriter::WriteFileAtomically(path, *data); +void WriteScopedStringToFileAtomically(const FilePath& path, + std::unique_ptr data, + Callback callback) { + bool result = ImportantFileWriter::WriteFileAtomically(path, *data); + + if (!callback.is_null()) + callback.Run(result); } } // namespace @@ -168,8 +172,10 @@ void ImportantFileWriter::WriteNow(std::unique_ptr data) { if (HasPendingWrite()) timer_.Stop(); - auto task = Bind(&WriteScopedStringToFileAtomically, path_, Passed(&data)); - if (!PostWriteTask(task)) { + Closure task = Bind(&WriteScopedStringToFileAtomically, path_, Passed(&data), + Passed(&on_next_write_callback_)); + + if (!task_runner_->PostTask(FROM_HERE, MakeCriticalClosure(task))) { // Posting the task to background message loop is not expected // to fail, but if it does, avoid losing data and just hit the disk // on the current thread. @@ -203,37 +209,9 @@ void ImportantFileWriter::DoScheduledWrite() { serializer_ = nullptr; } -void ImportantFileWriter::RegisterOnNextSuccessfulWriteCallback( - const Closure& on_next_successful_write) { - DCHECK(on_next_successful_write_.is_null()); - on_next_successful_write_ = on_next_successful_write; -} - -bool ImportantFileWriter::PostWriteTask(const Callback& task) { - // TODO(gab): This code could always use PostTaskAndReplyWithResult and let - // ForwardSuccessfulWrite() no-op if |on_next_successful_write_| is null, but - // PostTaskAndReply causes memory leaks in tests (crbug.com/371974) and - // suppressing all of those is unrealistic hence we avoid most of them by - // using PostTask() in the typical scenario below. - if (!on_next_successful_write_.is_null()) { - return PostTaskAndReplyWithResult( - task_runner_.get(), - FROM_HERE, - MakeCriticalClosure(task), - Bind(&ImportantFileWriter::ForwardSuccessfulWrite, - weak_factory_.GetWeakPtr())); - } - return task_runner_->PostTask( - FROM_HERE, - MakeCriticalClosure(Bind(IgnoreResult(task)))); -} - -void ImportantFileWriter::ForwardSuccessfulWrite(bool result) { - DCHECK(CalledOnValidThread()); - if (result && !on_next_successful_write_.is_null()) { - on_next_successful_write_.Run(); - on_next_successful_write_.Reset(); - } +void ImportantFileWriter::RegisterOnNextWriteCallback( + const Callback& on_next_write_callback) { + on_next_write_callback_ = on_next_write_callback; } } // namespace base diff --git a/base/files/important_file_writer.h b/base/files/important_file_writer.h index 6e8646af3dab1d..cd724e326691a2 100644 --- a/base/files/important_file_writer.h +++ b/base/files/important_file_writer.h @@ -94,25 +94,23 @@ class BASE_EXPORT ImportantFileWriter : public NonThreadSafe { // Serialize data pending to be saved and execute write on backend thread. void DoScheduledWrite(); - // Registers |on_next_successful_write| to be called once, on the next - // successful write event. Only one callback can be set at once. - void RegisterOnNextSuccessfulWriteCallback( - const Closure& on_next_successful_write); + // Registers |on_next_write_callback| to be synchronously invoked from + // WriteFileAtomically() on its next write (i.e. from |task_runner_|), with + // |success| indicating whether it succeeded or not. + // |on_next_write_callback| must be thread safe, as it will be called on + // |task_runner_| and may be called during Chrome shutdown. + // If called more than once before a write is scheduled on |task_runner|, the + // latest callback clobbers the others. + void RegisterOnNextWriteCallback( + const Callback& on_next_write_callback); TimeDelta commit_interval() const { return commit_interval_; } private: - // Helper method for WriteNow(). - bool PostWriteTask(const Callback& task); - - // If |result| is true and |on_next_successful_write_| is set, invokes - // |on_successful_write_| and then resets it; no-ops otherwise. - void ForwardSuccessfulWrite(bool result); - - // Invoked once and then reset on the next successful write event. - Closure on_next_successful_write_; + // Invoked synchronously on the next write event. + Callback on_next_write_callback_; // Path being written to. const FilePath path_; diff --git a/base/files/important_file_writer_unittest.cc b/base/files/important_file_writer_unittest.cc index 3fd71e75949a2b..b8920a07679e89 100644 --- a/base/files/important_file_writer_unittest.cc +++ b/base/files/important_file_writer_unittest.cc @@ -46,39 +46,45 @@ class DataSerializer : public ImportantFileWriter::DataSerializer { const std::string data_; }; -class SuccessfulWriteObserver { +enum WriteCallbackObservationState { + NOT_CALLED, + CALLED_WITH_ERROR, + CALLED_WITH_SUCCESS, +}; + +class WriteCallbackObserver { public: - SuccessfulWriteObserver() : successful_write_observed_(false) {} + WriteCallbackObserver() : observation_state_(NOT_CALLED) {} - // Register on_successful_write() to be called on the next successful write - // of |writer|. - void ObserveNextSuccessfulWrite(ImportantFileWriter* writer); + // Register OnWrite() to be called on the next write of |writer|. + void ObserveNextWriteCallback(ImportantFileWriter* writer); - // Returns true if a successful write was observed via on_successful_write() + // Returns true if a write was observed via OnWrite() // and resets the observation state to false regardless. - bool GetAndResetObservationState(); + WriteCallbackObservationState GetAndResetObservationState(); private: - void on_successful_write() { - EXPECT_FALSE(successful_write_observed_); - successful_write_observed_ = true; + void OnWrite(bool success) { + EXPECT_EQ(NOT_CALLED, observation_state_); + observation_state_ = success ? CALLED_WITH_SUCCESS : CALLED_WITH_ERROR; } - bool successful_write_observed_; + WriteCallbackObservationState observation_state_; - DISALLOW_COPY_AND_ASSIGN(SuccessfulWriteObserver); + DISALLOW_COPY_AND_ASSIGN(WriteCallbackObserver); }; -void SuccessfulWriteObserver::ObserveNextSuccessfulWrite( +void WriteCallbackObserver::ObserveNextWriteCallback( ImportantFileWriter* writer) { - writer->RegisterOnNextSuccessfulWriteCallback(base::Bind( - &SuccessfulWriteObserver::on_successful_write, base::Unretained(this))); + writer->RegisterOnNextWriteCallback( + base::Bind(&WriteCallbackObserver::OnWrite, base::Unretained(this))); } -bool SuccessfulWriteObserver::GetAndResetObservationState() { - bool was_successful_write_observed = successful_write_observed_; - successful_write_observed_ = false; - return was_successful_write_observed; +WriteCallbackObservationState +WriteCallbackObserver::GetAndResetObservationState() { + WriteCallbackObservationState state = observation_state_; + observation_state_ = NOT_CALLED; + return state; } } // namespace @@ -92,7 +98,7 @@ class ImportantFileWriterTest : public testing::Test { } protected: - SuccessfulWriteObserver successful_write_observer_; + WriteCallbackObserver write_callback_observer_; FilePath file_; MessageLoop loop_; @@ -103,49 +109,91 @@ class ImportantFileWriterTest : public testing::Test { TEST_F(ImportantFileWriterTest, Basic) { ImportantFileWriter writer(file_, ThreadTaskRunnerHandle::Get()); EXPECT_FALSE(PathExists(writer.path())); - EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState()); + EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); writer.WriteNow(MakeUnique("foo")); RunLoop().RunUntilIdle(); - EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState()); + EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); ASSERT_TRUE(PathExists(writer.path())); EXPECT_EQ("foo", GetFileContent(writer.path())); } -TEST_F(ImportantFileWriterTest, BasicWithSuccessfulWriteObserver) { +TEST_F(ImportantFileWriterTest, WriteWithObserver) { ImportantFileWriter writer(file_, ThreadTaskRunnerHandle::Get()); EXPECT_FALSE(PathExists(writer.path())); - EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState()); - successful_write_observer_.ObserveNextSuccessfulWrite(&writer); + EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); + + // Confirm that the observer is invoked. + write_callback_observer_.ObserveNextWriteCallback(&writer); writer.WriteNow(MakeUnique("foo")); RunLoop().RunUntilIdle(); - // Confirm that the observer is invoked. - EXPECT_TRUE(successful_write_observer_.GetAndResetObservationState()); + EXPECT_EQ(CALLED_WITH_SUCCESS, + write_callback_observer_.GetAndResetObservationState()); ASSERT_TRUE(PathExists(writer.path())); EXPECT_EQ("foo", GetFileContent(writer.path())); // Confirm that re-installing the observer works for another write. - EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState()); - successful_write_observer_.ObserveNextSuccessfulWrite(&writer); + EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); + write_callback_observer_.ObserveNextWriteCallback(&writer); writer.WriteNow(MakeUnique("bar")); RunLoop().RunUntilIdle(); - EXPECT_TRUE(successful_write_observer_.GetAndResetObservationState()); + EXPECT_EQ(CALLED_WITH_SUCCESS, + write_callback_observer_.GetAndResetObservationState()); ASSERT_TRUE(PathExists(writer.path())); EXPECT_EQ("bar", GetFileContent(writer.path())); // Confirm that writing again without re-installing the observer doesn't // result in a notification. - EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState()); + EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); writer.WriteNow(MakeUnique("baz")); RunLoop().RunUntilIdle(); - EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState()); + EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); ASSERT_TRUE(PathExists(writer.path())); EXPECT_EQ("baz", GetFileContent(writer.path())); } +TEST_F(ImportantFileWriterTest, FailedWriteWithObserver) { + // Use an invalid file path (relative paths are invalid) to get a + // FILE_ERROR_ACCESS_DENIED error when trying to write the file. + ImportantFileWriter writer(FilePath().AppendASCII("bad/../path"), + ThreadTaskRunnerHandle::Get()); + EXPECT_FALSE(PathExists(writer.path())); + EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); + write_callback_observer_.ObserveNextWriteCallback(&writer); + writer.WriteNow(MakeUnique("foo")); + RunLoop().RunUntilIdle(); + + // Confirm that the write observer was invoked with its boolean parameter set + // to false. + EXPECT_EQ(CALLED_WITH_ERROR, + write_callback_observer_.GetAndResetObservationState()); + EXPECT_FALSE(PathExists(writer.path())); +} + +TEST_F(ImportantFileWriterTest, CallbackRunsOnWriterThread) { + base::Thread file_writer_thread("ImportantFileWriter test thread"); + file_writer_thread.Start(); + ImportantFileWriter writer(file_, file_writer_thread.task_runner()); + EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); + + write_callback_observer_.ObserveNextWriteCallback(&writer); + writer.WriteNow(MakeUnique("foo")); + RunLoop().RunUntilIdle(); + + // Expect the callback to not have been executed before the write. + EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); + + // Make sure tasks posted by WriteNow() have ran before continuing. + file_writer_thread.FlushForTesting(); + EXPECT_EQ(CALLED_WITH_SUCCESS, + write_callback_observer_.GetAndResetObservationState()); + ASSERT_TRUE(PathExists(writer.path())); + EXPECT_EQ("foo", GetFileContent(writer.path())); +} + TEST_F(ImportantFileWriterTest, ScheduleWrite) { ImportantFileWriter writer(file_, ThreadTaskRunnerHandle::Get(), diff --git a/chrome/browser/prefs/profile_pref_store_manager.cc b/chrome/browser/prefs/profile_pref_store_manager.cc index 1417ef28c5f76a..63d7dbe6f5e9da 100644 --- a/chrome/browser/prefs/profile_pref_store_manager.cc +++ b/chrome/browser/prefs/profile_pref_store_manager.cc @@ -134,9 +134,9 @@ PersistentPrefStore* ProfilePrefStoreManager::CreateProfilePrefStore( unprotected_pref_names, protected_pref_names, base::Bind(&RemoveValueSilently, unprotected_pref_store->AsWeakPtr()), base::Bind(&RemoveValueSilently, protected_pref_store->AsWeakPtr()), - base::Bind(&JsonPrefStore::RegisterOnNextSuccessfulWriteCallback, + base::Bind(&JsonPrefStore::RegisterOnNextSuccessfulWriteReply, unprotected_pref_store->AsWeakPtr()), - base::Bind(&JsonPrefStore::RegisterOnNextSuccessfulWriteCallback, + base::Bind(&JsonPrefStore::RegisterOnNextSuccessfulWriteReply, protected_pref_store->AsWeakPtr()), GetPrefHashStore(false), GetPrefHashStore(true), raw_unprotected_pref_hash_filter, raw_protected_pref_hash_filter); diff --git a/components/prefs/json_pref_store.cc b/components/prefs/json_pref_store.cc index 779c2009730366..884829a7ac0392 100644 --- a/components/prefs/json_pref_store.cc +++ b/components/prefs/json_pref_store.cc @@ -22,6 +22,7 @@ #include "base/strings/string_number_conversions.h" #include "base/strings/string_util.h" #include "base/task_runner_util.h" +#include "base/threading/sequenced_task_runner_handle.h" #include "base/threading/sequenced_worker_pool.h" #include "base/time/default_clock.h" #include "base/values.h" @@ -171,6 +172,8 @@ JsonPrefStore::JsonPrefStore( filtering_in_progress_(false), pending_lossy_write_(false), read_error_(PREF_READ_ERROR_NONE), + has_pending_successful_write_reply_(false), + has_pending_write_callback_(false), write_count_histogram_(writer_.commit_interval(), path_) { DCHECK(!path_.empty()); } @@ -323,11 +326,69 @@ void JsonPrefStore::ReportValueChanged(const std::string& key, uint32_t flags) { ScheduleWrite(flags); } -void JsonPrefStore::RegisterOnNextSuccessfulWriteCallback( - const base::Closure& on_next_successful_write) { +void JsonPrefStore::RunOrScheduleNextSuccessfulWriteCallback( + bool write_success) { DCHECK(CalledOnValidThread()); - writer_.RegisterOnNextSuccessfulWriteCallback(on_next_successful_write); + has_pending_write_callback_ = false; + if (has_pending_successful_write_reply_) { + has_pending_successful_write_reply_ = false; + if (write_success) { + on_next_successful_write_reply_.Run(); + } else { + RegisterOnNextSuccessfulWriteReply(on_next_successful_write_reply_); + } + } +} + +// static +void JsonPrefStore::PostWriteCallback( + const base::Callback& on_next_write_reply, + const base::Callback& on_next_write_callback, + scoped_refptr reply_task_runner, + bool write_success) { + if (!on_next_write_callback.is_null()) + on_next_write_callback.Run(write_success); + + // We can't run |on_next_write_reply| on the current thread. Bounce back to + // the |reply_task_runner| which is the correct sequenced thread. + reply_task_runner->PostTask(FROM_HERE, + base::Bind(on_next_write_reply, write_success)); +} + +void JsonPrefStore::RegisterOnNextSuccessfulWriteReply( + const base::Closure& on_next_successful_write_reply) { + DCHECK(CalledOnValidThread()); + DCHECK(!has_pending_successful_write_reply_); + + has_pending_successful_write_reply_ = true; + on_next_successful_write_reply_ = on_next_successful_write_reply; + + // If there already is a pending callback, avoid erasing it; the reply will + // be used as we set |on_next_successful_write_reply_|. Otherwise, setup a + // reply with an empty callback. + if (!has_pending_write_callback_) { + writer_.RegisterOnNextWriteCallback(base::Bind( + &PostWriteCallback, + base::Bind(&JsonPrefStore::RunOrScheduleNextSuccessfulWriteCallback, + AsWeakPtr()), + base::Callback(), + base::SequencedTaskRunnerHandle::Get())); + } +} + +void JsonPrefStore::RegisterOnNextWriteSynchronousCallback( + const base::Callback& on_next_write_callback) { + DCHECK(CalledOnValidThread()); + DCHECK(!has_pending_write_callback_); + + has_pending_write_callback_ = true; + + writer_.RegisterOnNextWriteCallback(base::Bind( + &PostWriteCallback, + base::Bind(&JsonPrefStore::RunOrScheduleNextSuccessfulWriteCallback, + AsWeakPtr()), + on_next_write_callback, base::SequencedTaskRunnerHandle::Get())); } void JsonPrefStore::ClearMutableValues() { diff --git a/components/prefs/json_pref_store.h b/components/prefs/json_pref_store.h index 047fe74abcd888..218b2051b0dc76 100644 --- a/components/prefs/json_pref_store.h +++ b/components/prefs/json_pref_store.h @@ -30,6 +30,7 @@ class Clock; class DictionaryValue; class FilePath; class HistogramBase; +class JsonPrefStoreCallbackTest; class JsonPrefStoreLossyWriteTest; class SequencedTaskRunner; class SequencedWorkerPool; @@ -106,10 +107,18 @@ class COMPONENTS_PREFS_EXPORT JsonPrefStore // cleanup that shouldn't otherwise alert observers. void RemoveValueSilently(const std::string& key, uint32_t flags); - // Registers |on_next_successful_write| to be called once, on the next + // Registers |on_next_successful_write_reply| to be called once, on the next // successful write event of |writer_|. - void RegisterOnNextSuccessfulWriteCallback( - const base::Closure& on_next_successful_write); + // |on_next_successful_write_reply| will be called on the thread from which + // this method is called and does not need to be thread safe. + void RegisterOnNextSuccessfulWriteReply( + const base::Closure& on_next_successful_write_reply); + + // Registers |on_next_write_callback| to be called once synchronously, on the + // next write event of |writer_|. + // |on_next_write_callback| must be thread-safe. + void RegisterOnNextWriteSynchronousCallback( + const base::Callback& on_next_write_callback); void ClearMutableValues() override; @@ -170,10 +179,24 @@ class COMPONENTS_PREFS_EXPORT JsonPrefStore WriteCountHistogramTestMultiplePeriods); FRIEND_TEST_ALL_PREFIXES(base::JsonPrefStoreTest, WriteCountHistogramTestPeriodWithGaps); + friend class base::JsonPrefStoreCallbackTest; friend class base::JsonPrefStoreLossyWriteTest; ~JsonPrefStore() override; + // If |write_success| is true, runs |on_next_successful_write_|. + // Otherwise, re-registers |on_next_successful_write_|. + void RunOrScheduleNextSuccessfulWriteCallback(bool write_success); + + // Handles the result of a write with result |write_success|. Runs + // |on_next_write| callback on the current thread and posts + // |RunOrScheduleNextSuccessfulWriteCallback| on |reply_task_runner|. + static void PostWriteCallback( + const base::Callback& on_next_write_reply, + const base::Callback& on_next_write_callback, + scoped_refptr reply_task_runner, + bool write_success); + // This method is called after the JSON file has been read. It then hands // |value| (or an empty dictionary in some read error cases) to the // |pref_filter| if one is set. It also gives a callback pointing at @@ -223,6 +246,10 @@ class COMPONENTS_PREFS_EXPORT JsonPrefStore std::set keys_need_empty_value_; + bool has_pending_successful_write_reply_; + bool has_pending_write_callback_; + base::Closure on_next_successful_write_reply_; + WriteCountHistogram write_count_histogram_; DISALLOW_COPY_AND_ASSIGN(JsonPrefStore); diff --git a/components/prefs/json_pref_store_unittest.cc b/components/prefs/json_pref_store_unittest.cc index 2255c2e0971e68..cf93e7ef3352a1 100644 --- a/components/prefs/json_pref_store_unittest.cc +++ b/components/prefs/json_pref_store_unittest.cc @@ -26,6 +26,7 @@ #include "base/strings/utf_string_conversions.h" #include "base/test/histogram_tester.h" #include "base/test/simple_test_clock.h" +#include "base/threading/sequenced_task_runner_handle.h" #include "base/threading/sequenced_worker_pool.h" #include "base/threading/thread.h" #include "base/values.h" @@ -121,6 +122,9 @@ class MockReadErrorDelegate : public PersistentPrefStore::ReadErrorDelegate { } // namespace class JsonPrefStoreTest : public testing::Test { + public: + JsonPrefStoreTest() = default; + protected: void SetUp() override { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); @@ -136,6 +140,8 @@ class JsonPrefStoreTest : public testing::Test { base::ScopedTempDir temp_dir_; // A message loop that we can use as the file thread message loop. MessageLoop message_loop_; + + DISALLOW_COPY_AND_ASSIGN(JsonPrefStoreTest); }; // Test fallback behavior for a nonexistent file. @@ -817,21 +823,22 @@ TEST_F(JsonPrefStoreTest, WriteCountHistogramTestPeriodWithGaps) { } class JsonPrefStoreLossyWriteTest : public JsonPrefStoreTest { + public: + JsonPrefStoreLossyWriteTest() = default; + protected: void SetUp() override { JsonPrefStoreTest::SetUp(); test_file_ = temp_dir_.GetPath().AppendASCII("test.json"); } - // Creates a JsonPrefStore with the given |file_writer|. scoped_refptr CreatePrefStore() { return new JsonPrefStore(test_file_, message_loop_.task_runner(), std::unique_ptr()); } // Return the ImportantFileWriter for a given JsonPrefStore. - ImportantFileWriter* GetImportantFileWriter( - scoped_refptr pref_store) { + ImportantFileWriter* GetImportantFileWriter(JsonPrefStore* pref_store) { return &(pref_store->writer_); } @@ -846,11 +853,13 @@ class JsonPrefStoreLossyWriteTest : public JsonPrefStoreTest { private: base::FilePath test_file_; + + DISALLOW_COPY_AND_ASSIGN(JsonPrefStoreLossyWriteTest); }; TEST_F(JsonPrefStoreLossyWriteTest, LossyWriteBasic) { scoped_refptr pref_store = CreatePrefStore(); - ImportantFileWriter* file_writer = GetImportantFileWriter(pref_store); + ImportantFileWriter* file_writer = GetImportantFileWriter(pref_store.get()); // Set a normal pref and check that it gets scheduled to be written. ASSERT_FALSE(file_writer->HasPendingWrite()); @@ -896,7 +905,7 @@ TEST_F(JsonPrefStoreLossyWriteTest, LossyWriteBasic) { TEST_F(JsonPrefStoreLossyWriteTest, LossyWriteMixedLossyFirst) { scoped_refptr pref_store = CreatePrefStore(); - ImportantFileWriter* file_writer = GetImportantFileWriter(pref_store); + ImportantFileWriter* file_writer = GetImportantFileWriter(pref_store.get()); // Set a lossy pref and check that it is not scheduled to be written. ASSERT_FALSE(file_writer->HasPendingWrite()); @@ -918,7 +927,7 @@ TEST_F(JsonPrefStoreLossyWriteTest, LossyWriteMixedLossyFirst) { TEST_F(JsonPrefStoreLossyWriteTest, LossyWriteMixedLossySecond) { scoped_refptr pref_store = CreatePrefStore(); - ImportantFileWriter* file_writer = GetImportantFileWriter(pref_store); + ImportantFileWriter* file_writer = GetImportantFileWriter(pref_store.get()); // Set a normal pref and check that it is scheduled to be written. ASSERT_FALSE(file_writer->HasPendingWrite()); @@ -940,7 +949,7 @@ TEST_F(JsonPrefStoreLossyWriteTest, LossyWriteMixedLossySecond) { TEST_F(JsonPrefStoreLossyWriteTest, ScheduleLossyWrite) { scoped_refptr pref_store = CreatePrefStore(); - ImportantFileWriter* file_writer = GetImportantFileWriter(pref_store); + ImportantFileWriter* file_writer = GetImportantFileWriter(pref_store.get()); // Set a lossy pref and check that it is not scheduled to be written. pref_store->SetValue("lossy", base::MakeUnique("lossy"), @@ -958,4 +967,213 @@ TEST_F(JsonPrefStoreLossyWriteTest, ScheduleLossyWrite) { ASSERT_EQ("{\"lossy\":\"lossy\"}", GetTestFileContents()); } -} // namespace base +class SuccessfulWriteReplyObserver { + public: + SuccessfulWriteReplyObserver() = default; + + // Returns true if a successful write was observed via on_successful_write() + // and resets the observation state to false regardless. + bool GetAndResetObservationState() { + bool was_successful_write_observed = successful_write_reply_observed_; + successful_write_reply_observed_ = false; + return was_successful_write_observed; + } + + // Register OnWrite() to be called on the next write of |json_pref_store|. + void ObserveNextWriteCallback(JsonPrefStore* json_pref_store); + + void OnSuccessfulWrite() { + EXPECT_FALSE(successful_write_reply_observed_); + successful_write_reply_observed_ = true; + } + + private: + bool successful_write_reply_observed_ = false; + + DISALLOW_COPY_AND_ASSIGN(SuccessfulWriteReplyObserver); +}; + +void SuccessfulWriteReplyObserver::ObserveNextWriteCallback( + JsonPrefStore* json_pref_store) { + json_pref_store->RegisterOnNextSuccessfulWriteReply( + base::Bind(&SuccessfulWriteReplyObserver::OnSuccessfulWrite, + base::Unretained(this))); +} + +enum WriteCallbackObservationState { + NOT_CALLED, + CALLED_WITH_ERROR, + CALLED_WITH_SUCCESS, +}; + +class WriteCallbackObserver { + public: + WriteCallbackObserver() = default; + + // Register OnWrite() to be called on the next write of |json_pref_store|. + void ObserveNextWriteCallback(JsonPrefStore* json_pref_store); + + // Returns true if a write was observed via OnWrite() + // and resets the observation state to false regardless. + WriteCallbackObservationState GetAndResetObservationState(); + + void OnWrite(bool success) { + EXPECT_EQ(NOT_CALLED, observation_state_); + observation_state_ = success ? CALLED_WITH_SUCCESS : CALLED_WITH_ERROR; + } + + private: + WriteCallbackObservationState observation_state_ = NOT_CALLED; + + DISALLOW_COPY_AND_ASSIGN(WriteCallbackObserver); +}; + +void WriteCallbackObserver::ObserveNextWriteCallback(JsonPrefStore* writer) { + writer->RegisterOnNextWriteSynchronousCallback( + base::Bind(&WriteCallbackObserver::OnWrite, base::Unretained(this))); +} + +WriteCallbackObservationState +WriteCallbackObserver::GetAndResetObservationState() { + WriteCallbackObservationState state = observation_state_; + observation_state_ = NOT_CALLED; + return state; +} + +class JsonPrefStoreCallbackTest : public JsonPrefStoreTest { + public: + JsonPrefStoreCallbackTest() = default; + + protected: + void SetUp() override { + JsonPrefStoreTest::SetUp(); + test_file_ = temp_dir_.path().AppendASCII("test.json"); + } + + scoped_refptr CreatePrefStore() { + return new JsonPrefStore(test_file_, message_loop_.task_runner(), + std::unique_ptr()); + } + + // Return the ImportantFileWriter for a given JsonPrefStore. + ImportantFileWriter* GetImportantFileWriter(JsonPrefStore* pref_store) { + return &(pref_store->writer_); + } + + void TriggerFakeWriteForCallback(JsonPrefStore* pref_store, bool success) { + JsonPrefStore::PostWriteCallback( + base::Bind(&JsonPrefStore::RunOrScheduleNextSuccessfulWriteCallback, + pref_store->AsWeakPtr()), + base::Bind(&WriteCallbackObserver::OnWrite, + base::Unretained(&write_callback_observer_)), + base::SequencedTaskRunnerHandle::Get(), success); + } + + SuccessfulWriteReplyObserver successful_write_reply_observer_; + WriteCallbackObserver write_callback_observer_; + + private: + base::FilePath test_file_; + + DISALLOW_COPY_AND_ASSIGN(JsonPrefStoreCallbackTest); +}; + +TEST_F(JsonPrefStoreCallbackTest, TestPostWriteCallback) { + scoped_refptr pref_store = CreatePrefStore(); + ImportantFileWriter* file_writer = GetImportantFileWriter(pref_store.get()); + + // Test RegisterOnNextWriteSynchronousCallback after + // RegisterOnNextSuccessfulWriteReply. + successful_write_reply_observer_.ObserveNextWriteCallback(pref_store.get()); + write_callback_observer_.ObserveNextWriteCallback(pref_store.get()); + file_writer->WriteNow(MakeUnique("foo")); + RunLoop().RunUntilIdle(); + EXPECT_TRUE(successful_write_reply_observer_.GetAndResetObservationState()); + EXPECT_TRUE(write_callback_observer_.GetAndResetObservationState()); + + // Test RegisterOnNextSuccessfulWriteReply after + // RegisterOnNextWriteSynchronousCallback. + successful_write_reply_observer_.ObserveNextWriteCallback(pref_store.get()); + write_callback_observer_.ObserveNextWriteCallback(pref_store.get()); + file_writer->WriteNow(MakeUnique("foo")); + RunLoop().RunUntilIdle(); + EXPECT_TRUE(successful_write_reply_observer_.GetAndResetObservationState()); + EXPECT_TRUE(write_callback_observer_.GetAndResetObservationState()); + + // Test RegisterOnNextSuccessfulWriteReply only. + successful_write_reply_observer_.ObserveNextWriteCallback(pref_store.get()); + file_writer->WriteNow(MakeUnique("foo")); + RunLoop().RunUntilIdle(); + EXPECT_TRUE(successful_write_reply_observer_.GetAndResetObservationState()); + EXPECT_FALSE(write_callback_observer_.GetAndResetObservationState()); + + // Test RegisterOnNextWriteSynchronousCallback only. + write_callback_observer_.ObserveNextWriteCallback(pref_store.get()); + file_writer->WriteNow(MakeUnique("foo")); + RunLoop().RunUntilIdle(); + EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState()); + EXPECT_TRUE(write_callback_observer_.GetAndResetObservationState()); +} + +TEST_F(JsonPrefStoreCallbackTest, TestPostWriteCallbackWithFakeFailure) { + scoped_refptr pref_store = CreatePrefStore(); + + // Confirm that the observers are invoked. + successful_write_reply_observer_.ObserveNextWriteCallback(pref_store.get()); + TriggerFakeWriteForCallback(pref_store.get(), true); + RunLoop().RunUntilIdle(); + EXPECT_TRUE(successful_write_reply_observer_.GetAndResetObservationState()); + EXPECT_EQ(CALLED_WITH_SUCCESS, + write_callback_observer_.GetAndResetObservationState()); + + // Confirm that the observation states were reset. + EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState()); + EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); + + // Confirm that re-installing the observers works for another write. + successful_write_reply_observer_.ObserveNextWriteCallback(pref_store.get()); + TriggerFakeWriteForCallback(pref_store.get(), true); + RunLoop().RunUntilIdle(); + EXPECT_TRUE(successful_write_reply_observer_.GetAndResetObservationState()); + EXPECT_EQ(CALLED_WITH_SUCCESS, + write_callback_observer_.GetAndResetObservationState()); + + // Confirm that the successful observer is not invoked by an unsuccessful + // write, and that the synchronous observer is invoked. + successful_write_reply_observer_.ObserveNextWriteCallback(pref_store.get()); + TriggerFakeWriteForCallback(pref_store.get(), false); + RunLoop().RunUntilIdle(); + EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState()); + EXPECT_EQ(CALLED_WITH_ERROR, + write_callback_observer_.GetAndResetObservationState()); + + // Do a real write, and confirm that the successful observer was invoked after + // being set by |PostWriteCallback| by the last TriggerFakeWriteCallback. + ImportantFileWriter* file_writer = GetImportantFileWriter(pref_store.get()); + file_writer->WriteNow(MakeUnique("foo")); + RunLoop().RunUntilIdle(); + EXPECT_TRUE(successful_write_reply_observer_.GetAndResetObservationState()); + EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); +} + +TEST_F(JsonPrefStoreCallbackTest, TestPostWriteCallbackDuringProfileDeath) { + // Create a JsonPrefStore and attach observers to it, then delete it by making + // it go out of scope to simulate profile switch or Chrome shutdown. + { + scoped_refptr soon_out_of_scope_pref_store = + CreatePrefStore(); + ImportantFileWriter* file_writer = + GetImportantFileWriter(soon_out_of_scope_pref_store.get()); + successful_write_reply_observer_.ObserveNextWriteCallback( + soon_out_of_scope_pref_store.get()); + write_callback_observer_.ObserveNextWriteCallback( + soon_out_of_scope_pref_store.get()); + file_writer->WriteNow(MakeUnique("foo")); + } + RunLoop().RunUntilIdle(); + EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState()); + EXPECT_EQ(CALLED_WITH_SUCCESS, + write_callback_observer_.GetAndResetObservationState()); +} + +} // namespace base \ No newline at end of file