Implement lossy pref behavior for JsonPrefStore.

Writes to lossy prefs should not trigger writes to disk to be scheduled.
This behavior is implemented for JsonPrefStore. CommitPendingWrites will
ensure that all outstanding lossy writes are also written to disk.

BUG=476800

Committed: https://crrev.com/e9d8c9882ca272c803711e26ded4dd7d5203a59e
Cr-Commit-Position: refs/heads/master@{#329122}

Review URL: https://codereview.chromium.org/1127963002

Cr-Commit-Position: refs/heads/master@{#329287}

base/prefs/json_pref_store.cc

@@ -150,17 +150,10 @@ JsonPrefStore::JsonPrefStore(
     const base::FilePath& filename,
     const scoped_refptr<base::SequencedTaskRunner>& sequenced_task_runner,
     scoped_ptr<PrefFilter> pref_filter)
-    : path_(filename),
-      sequenced_task_runner_(sequenced_task_runner),
-      prefs_(new base::DictionaryValue()),
-      read_only_(false),
-      writer_(filename, sequenced_task_runner),
-      pref_filter_(pref_filter.Pass()),
-      initialized_(false),
-      filtering_in_progress_(false),
-      read_error_(PREF_READ_ERROR_NONE),
-      write_count_histogram_(writer_.commit_interval(), path_) {
-  DCHECK(!path_.empty());
+    : JsonPrefStore(filename,
+                    base::FilePath(),
+                    sequenced_task_runner,
+                    pref_filter.Pass()) {
 }
 
 JsonPrefStore::JsonPrefStore(
@@ -177,6 +170,7 @@ JsonPrefStore::JsonPrefStore(
       pref_filter_(pref_filter.Pass()),
       initialized_(false),
       filtering_in_progress_(false),
+      pending_lossy_write_(false),
       read_error_(PREF_READ_ERROR_NONE),
       write_count_histogram_(writer_.commit_interval(), path_) {
   DCHECK(!path_.empty());
@@ -252,8 +246,7 @@ void JsonPrefStore::SetValueSilently(const std::string& key,
   prefs_->Get(key, &old_value);
   if (!old_value || !value->Equals(old_value)) {
     prefs_->Set(key, new_value.release());
-    if (!read_only_)
-      writer_.ScheduleWrite(this);
+    ScheduleWrite(flags);
   }
 }
 
@@ -268,8 +261,7 @@ void JsonPrefStore::RemoveValueSilently(const std::string& key, uint32 flags) {
   DCHECK(CalledOnValidThread());
 
   prefs_->RemovePath(key, NULL);
-  if (!read_only_)
-    writer_.ScheduleWrite(this);
+  ScheduleWrite(flags);
 }
 
 bool JsonPrefStore::ReadOnly() const {
@@ -309,6 +301,11 @@ void JsonPrefStore::ReadPrefsAsync(ReadErrorDelegate* error_delegate) {
 void JsonPrefStore::CommitPendingWrite() {
   DCHECK(CalledOnValidThread());
 
+  // Schedule a write for any lossy writes that are outstanding to ensure that
+  // they get flushed when this function is called.
+  if (pending_lossy_write_)
+    writer_.ScheduleWrite(this);
+
   if (writer_.HasPendingWrite() && !read_only_)
     writer_.DoScheduledWrite();
 }
@@ -321,8 +318,7 @@ void JsonPrefStore::ReportValueChanged(const std::string& key, uint32 flags) {
 
   FOR_EACH_OBSERVER(PrefStore::Observer, observers_, OnPrefValueChanged(key));
 
-  if (!read_only_)
-    writer_.ScheduleWrite(this);
+  ScheduleWrite(flags);
 }
 
 void JsonPrefStore::RegisterOnNextSuccessfulWriteCallback(
@@ -401,6 +397,8 @@ JsonPrefStore::~JsonPrefStore() {
 bool JsonPrefStore::SerializeData(std::string* output) {
   DCHECK(CalledOnValidThread());
 
+  pending_lossy_write_ = false;
+
   write_count_histogram_.RecordWriteOccured();
 
   if (pref_filter_)
@@ -432,8 +430,8 @@ void JsonPrefStore::FinalizeFileRead(bool initialization_successful,
 
   initialized_ = true;
 
-  if (schedule_write && !read_only_)
-    writer_.ScheduleWrite(this);
+  if (schedule_write)
+    ScheduleWrite(DEFAULT_PREF_WRITE_FLAGS);
 
   if (error_delegate_ && read_error_ != PREF_READ_ERROR_NONE)
     error_delegate_->OnError(read_error_);
@@ -445,6 +443,16 @@ void JsonPrefStore::FinalizeFileRead(bool initialization_successful,
   return;
 }
 
+void JsonPrefStore::ScheduleWrite(uint32 flags) {
+  if (read_only_)
+    return;
+
+  if (flags & LOSSY_PREF_WRITE_FLAG)
+    pending_lossy_write_ = true;
+  else
+    writer_.ScheduleWrite(this);
+}
+
 // NOTE: This value should NOT be changed without renaming the histogram
 // otherwise it will create incompatible buckets.
 const int32_t

base/prefs/json_pref_store.h

@@ -29,6 +29,7 @@ class Clock;
 class DictionaryValue;
 class FilePath;
 class HistogramBase;
+class JsonPrefStoreLossyWriteTest;
 class SequencedTaskRunner;
 class SequencedWorkerPool;
 class Value;
@@ -165,6 +166,7 @@ class BASE_PREFS_EXPORT JsonPrefStore
                            WriteCountHistogramTestMultiplePeriods);
   FRIEND_TEST_ALL_PREFIXES(base::JsonPrefStoreTest,
                            WriteCountHistogramTestPeriodWithGaps);
+  friend class base::JsonPrefStoreLossyWriteTest;
 
   ~JsonPrefStore() override;
 
@@ -190,6 +192,10 @@ class BASE_PREFS_EXPORT JsonPrefStore
                         scoped_ptr<base::DictionaryValue> prefs,
                         bool schedule_write);
 
+  // Schedule a write with the file writer as long as |flags| doesn't contain
+  // WriteablePrefStore::LOSSY_PREF_WRITE_FLAG.
+  void ScheduleWrite(uint32 flags);
+
   const base::FilePath path_;
   const base::FilePath alternate_path_;
   const scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_;
@@ -208,6 +214,7 @@ class BASE_PREFS_EXPORT JsonPrefStore
 
   bool initialized_;
   bool filtering_in_progress_;
+  bool pending_lossy_write_;
   PrefReadError read_error_;
 
   std::set<std::string> keys_need_empty_value_;

base/prefs/json_pref_store_unittest.cc

@@ -108,9 +108,7 @@ class JsonPrefStoreTest : public testing::Test {
   void TearDown() override {
     // Make sure all pending tasks have been processed (e.g., deleting the
     // JsonPrefStore may post write tasks).
-    message_loop_.task_runner()->PostTask(FROM_HERE,
-                                          MessageLoop::QuitWhenIdleClosure());
-    message_loop_.Run();
+    RunLoop().RunUntilIdle();
   }
 
   // The path to temporary directory used to contain the test operations.
@@ -127,7 +125,7 @@ class JsonPrefStoreTest : public testing::Test {
 
 // Test fallback behavior for a nonexistent file.
 TEST_F(JsonPrefStoreTest, NonExistentFile) {
-  base::FilePath bogus_input_file = data_dir_.AppendASCII("read.txt");
+  base::FilePath bogus_input_file = temp_dir_.path().AppendASCII("read.txt");
   ASSERT_FALSE(PathExists(bogus_input_file));
   scoped_refptr<JsonPrefStore> pref_store = new JsonPrefStore(
       bogus_input_file, message_loop_.task_runner(), scoped_ptr<PrefFilter>());
@@ -138,9 +136,9 @@ TEST_F(JsonPrefStoreTest, NonExistentFile) {
 
 // Test fallback behavior for a nonexistent file and alternate file.
 TEST_F(JsonPrefStoreTest, NonExistentFileAndAlternateFile) {
-  base::FilePath bogus_input_file = data_dir_.AppendASCII("read.txt");
+  base::FilePath bogus_input_file = temp_dir_.path().AppendASCII("read.txt");
   base::FilePath bogus_alternate_input_file =
-      data_dir_.AppendASCII("read_alternate.txt");
+      temp_dir_.path().AppendASCII("read_alternate.txt");
   ASSERT_FALSE(PathExists(bogus_input_file));
   ASSERT_FALSE(PathExists(bogus_alternate_input_file));
   scoped_refptr<JsonPrefStore> pref_store =
@@ -321,7 +319,7 @@ TEST_F(JsonPrefStoreTest, PreserveEmptyValues) {
 
   // Write to file.
   pref_store->CommitPendingWrite();
-  MessageLoop::current()->RunUntilIdle();
+  RunLoop().RunUntilIdle();
 
   // Reload.
   pref_store = new JsonPrefStore(pref_file, message_loop_.task_runner(),
@@ -360,7 +358,7 @@ TEST_F(JsonPrefStoreTest, RemoveClearsEmptyParent) {
 
 // Tests asynchronous reading of the file when there is no file.
 TEST_F(JsonPrefStoreTest, AsyncNonExistingFile) {
-  base::FilePath bogus_input_file = data_dir_.AppendASCII("read.txt");
+  base::FilePath bogus_input_file = temp_dir_.path().AppendASCII("read.txt");
   ASSERT_FALSE(PathExists(bogus_input_file));
   scoped_refptr<JsonPrefStore> pref_store = new JsonPrefStore(
       bogus_input_file, message_loop_.task_runner(), scoped_ptr<PrefFilter>());
@@ -809,4 +807,125 @@ TEST_F(JsonPrefStoreTest, WriteCountHistogramTestPeriodWithGaps) {
   ASSERT_EQ(6, samples->TotalCount());
 }
 
+class JsonPrefStoreLossyWriteTest : public JsonPrefStoreTest {
+ protected:
+  void SetUp() override {
+    JsonPrefStoreTest::SetUp();
+    test_file_ = temp_dir_.path().AppendASCII("test.json");
+  }
+
+  // Creates a JsonPrefStore with the given |file_writer|.
+  scoped_refptr<JsonPrefStore> CreatePrefStore() {
+    return new JsonPrefStore(test_file_, message_loop_.task_runner(),
+                             scoped_ptr<PrefFilter>());
+  }
+
+  // Return the ImportantFileWriter for a given JsonPrefStore.
+  ImportantFileWriter* GetImportantFileWriter(
+      scoped_refptr<JsonPrefStore> pref_store) {
+    return &(pref_store->writer_);
+  }
+
+  // Get the contents of kTestFile. Pumps the message loop before returning the
+  // result.
+  std::string GetTestFileContents() {
+    RunLoop().RunUntilIdle();
+    std::string file_contents;
+    ReadFileToString(test_file_, &file_contents);
+    return file_contents;
+  }
+
+ private:
+  base::FilePath test_file_;
+};
+
+TEST_F(JsonPrefStoreLossyWriteTest, LossyWriteBasic) {
+  scoped_refptr<JsonPrefStore> pref_store = CreatePrefStore();
+  ImportantFileWriter* file_writer = GetImportantFileWriter(pref_store);
+
+  // Set a normal pref and check that it gets scheduled to be written.
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+  pref_store->SetValue("normal", new base::StringValue("normal"),
+                       WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS);
+  ASSERT_TRUE(file_writer->HasPendingWrite());
+  file_writer->DoScheduledWrite();
+  ASSERT_EQ("{\"normal\":\"normal\"}", GetTestFileContents());
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+
+  // Set a lossy pref and check that it is not scheduled to be written.
+  // SetValue/RemoveValue.
+  pref_store->SetValue("lossy", new base::StringValue("lossy"),
+                       WriteablePrefStore::LOSSY_PREF_WRITE_FLAG);
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+  pref_store->RemoveValue("lossy", WriteablePrefStore::LOSSY_PREF_WRITE_FLAG);
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+
+  // SetValueSilently/RemoveValueSilently.
+  pref_store->SetValueSilently("lossy", new base::StringValue("lossy"),
+                               WriteablePrefStore::LOSSY_PREF_WRITE_FLAG);
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+  pref_store->RemoveValueSilently("lossy",
+                                  WriteablePrefStore::LOSSY_PREF_WRITE_FLAG);
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+
+  // ReportValueChanged.
+  pref_store->SetValue("lossy", new base::StringValue("lossy"),
+                       WriteablePrefStore::LOSSY_PREF_WRITE_FLAG);
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+  pref_store->ReportValueChanged("lossy",
+                                 WriteablePrefStore::LOSSY_PREF_WRITE_FLAG);
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+
+  // Call CommitPendingWrite and check that the lossy pref and the normal pref
+  // are there with the last values set above.
+  pref_store->CommitPendingWrite();
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+  ASSERT_EQ("{\"lossy\":\"lossy\",\"normal\":\"normal\"}",
+            GetTestFileContents());
+}
+
+TEST_F(JsonPrefStoreLossyWriteTest, LossyWriteMixedLossyFirst) {
+  scoped_refptr<JsonPrefStore> pref_store = CreatePrefStore();
+  ImportantFileWriter* file_writer = GetImportantFileWriter(pref_store);
+
+  // Set a lossy pref and check that it is not scheduled to be written.
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+  pref_store->SetValue("lossy", new base::StringValue("lossy"),
+                       WriteablePrefStore::LOSSY_PREF_WRITE_FLAG);
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+
+  // Set a normal pref and check that it is scheduled to be written.
+  pref_store->SetValue("normal", new base::StringValue("normal"),
+                       WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS);
+  ASSERT_TRUE(file_writer->HasPendingWrite());
+
+  // Call DoScheduledWrite and check both prefs get written.
+  file_writer->DoScheduledWrite();
+  ASSERT_EQ("{\"lossy\":\"lossy\",\"normal\":\"normal\"}",
+            GetTestFileContents());
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+}
+
+TEST_F(JsonPrefStoreLossyWriteTest, LossyWriteMixedLossySecond) {
+  scoped_refptr<JsonPrefStore> pref_store = CreatePrefStore();
+  ImportantFileWriter* file_writer = GetImportantFileWriter(pref_store);
+
+  // Set a normal pref and check that it is scheduled to be written.
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+  pref_store->SetValue("normal", new base::StringValue("normal"),
+                       WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS);
+  ASSERT_TRUE(file_writer->HasPendingWrite());
+
+  // Set a lossy pref and check that the write is still scheduled.
+  pref_store->SetValue("lossy", new base::StringValue("lossy"),
+                       WriteablePrefStore::LOSSY_PREF_WRITE_FLAG);
+  ASSERT_TRUE(file_writer->HasPendingWrite());
+
+  // Call DoScheduledWrite and check both prefs get written.
+  file_writer->DoScheduledWrite();
+  ASSERT_EQ("{\"lossy\":\"lossy\",\"normal\":\"normal\"}",
+            GetTestFileContents());
+  ASSERT_FALSE(file_writer->HasPendingWrite());
+}
+
 }  // namespace base