Avoid out param in ImportantFileWriter::SerializeData

This CL removes the use of an out param in
ImportantFileWriter::SerializeData in favour of returning an optional.

Additionally, this CL removes usages of JSONStringValueSerializer from
the files in question.

Bug: 1421354
Change-Id: I32e59e2e49c4e226eae85cbcfe280ab34c39a661
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4321452
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Claudio DeSouza <cdesouza@igalia.com>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1116324}

base/files/important_file_writer.cc

@@ -122,8 +122,9 @@ void ImportantFileWriter::ProduceAndWriteStringToFileAtomically(
     OnceCallback<void(bool success)> after_write_callback,
     const std::string& histogram_suffix) {
   // Produce the actual data string on the background sequence.
-  std::string data;
-  if (!std::move(data_producer_for_background_sequence).Run(&data)) {
+  absl::optional<std::string> data =
+      std::move(data_producer_for_background_sequence).Run();
+  if (!data) {
     DLOG(WARNING) << "Failed to serialize data to be saved in " << path.value();
     return;
   }
@@ -134,7 +135,7 @@ void ImportantFileWriter::ProduceAndWriteStringToFileAtomically(
   // Calling the impl by way of the private
   // ProduceAndWriteStringToFileAtomically, which originated from an
   // ImportantFileWriter instance, so |from_instance| is true.
-  const bool result = WriteFileAtomicallyImpl(path, data, histogram_suffix,
+  const bool result = WriteFileAtomicallyImpl(path, *data, histogram_suffix,
                                               /*from_instance=*/true);
 
   if (!after_write_callback.is_null())
@@ -302,19 +303,16 @@ bool ImportantFileWriter::HasPendingWrite() const {
   return timer().IsRunning();
 }
 
-void ImportantFileWriter::WriteNow(std::unique_ptr<std::string> data) {
+void ImportantFileWriter::WriteNow(std::string data) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (!IsValueInRangeForNumericType<int32_t>(data->length())) {
+  if (!IsValueInRangeForNumericType<int32_t>(data.length())) {
     NOTREACHED();
     return;
   }
 
   WriteNowWithBackgroundDataProducer(base::BindOnce(
-      [](std::string data, std::string* output) {
-        *output = std::move(data);
-        return true;
-      },
-      std::move(*data)));
+      [](std::string data) { return absl::make_optional(std::move(data)); },
+      std::move(data)));
 }
 
 void ImportantFileWriter::WriteNowWithBackgroundDataProducer(
@@ -376,27 +374,19 @@ void ImportantFileWriter::DoScheduledWrite() {
   BackgroundDataProducerCallback data_producer_for_background_sequence;
 
   if (absl::holds_alternative<DataSerializer*>(serializer_)) {
-    std::string data;
-
-    // Pre-allocate previously needed memory plus 1kB for potential growth of
-    // data. Reduces the number of memory allocations to grow |data| step by
-    // step from tiny to very large.
-    data.reserve(previous_data_size_ + 1024);
-
-    if (!absl::get<DataSerializer*>(serializer_)->SerializeData(&data)) {
+    absl::optional<std::string> data;
+    data = absl::get<DataSerializer*>(serializer_)->SerializeData();
+    if (!data) {
       DLOG(WARNING) << "Failed to serialize data to be saved in "
                     << path_.value();
       ClearPendingWrite();
       return;
     }
 
-    previous_data_size_ = data.size();
+    previous_data_size_ = data->size();
     data_producer_for_background_sequence = base::BindOnce(
-        [](std::string data, std::string* result) {
-          *result = std::move(data);
-          return true;
-        },
-        std::move(data));
+        [](std::string data) { return absl::make_optional(std::move(data)); },
+        std::move(data).value());
   } else {
     data_producer_for_background_sequence =
         absl::get<BackgroundDataSerializer*>(serializer_)

base/files/important_file_writer.h

@@ -16,6 +16,7 @@
 #include "base/strings/string_piece.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
+#include "third_party/abseil-cpp/absl/types/optional.h"
 #include "third_party/abseil-cpp/absl/types/variant.h"
 
 namespace base {
@@ -41,16 +42,17 @@ class BASE_EXPORT ImportantFileWriter {
   // Promise-like callback that returns (via output parameter) the serialized
   // data to be written. This callback is invoked on the sequence where I/O
   // operations are executed. Returning false indicates an error.
-  using BackgroundDataProducerCallback = base::OnceCallback<bool(std::string*)>;
+  using BackgroundDataProducerCallback =
+      base::OnceCallback<absl::optional<std::string>()>;
 
   // Used by ScheduleSave to lazily provide the data to be saved. Allows us
   // to also batch data serializations.
   class BASE_EXPORT DataSerializer {
    public:
-    // Should put serialized string in |data| and return true on successful
-    // serialization. Will be called on the same thread on which
-    // ImportantFileWriter has been created.
-    virtual bool SerializeData(std::string* data) = 0;
+    // Returns a string for serialisation when successful, or a nullopt in case
+    // it failed to generate the data. Will be called on the same thread on
+    // which ImportantFileWriter has been created.
+    virtual absl::optional<std::string> SerializeData() = 0;
 
    protected:
     virtual ~DataSerializer() = default;
@@ -108,7 +110,7 @@ class BASE_EXPORT ImportantFileWriter {
 
   // Save |data| to target filename. Does not block. If there is a pending write
   // scheduled by ScheduleWrite(), it is cancelled.
-  void WriteNow(std::unique_ptr<std::string> data);
+  void WriteNow(std::string data);
 
   // Schedule a save to target filename. Data will be serialized and saved
   // to disk after the commit interval. If another ScheduleWrite is issued

base/files/important_file_writer_unittest.cc

@@ -22,6 +22,7 @@
 #include "base/time/time.h"
 #include "base/timer/mock_timer.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/abseil-cpp/absl/types/optional.h"
 
 namespace base {
 
@@ -40,10 +41,9 @@ class DataSerializer : public ImportantFileWriter::DataSerializer {
   explicit DataSerializer(const std::string& data) : data_(data) {
   }
 
-  bool SerializeData(std::string* output) override {
+  absl::optional<std::string> SerializeData() override {
     EXPECT_TRUE(sequence_checker_.CalledOnValidSequence());
-    output->assign(data_);
-    return true;
+    return data_;
   }
 
  private:
@@ -53,7 +53,7 @@ class DataSerializer : public ImportantFileWriter::DataSerializer {
 
 class FailingDataSerializer : public ImportantFileWriter::DataSerializer {
  public:
-  bool SerializeData(std::string* output) override { return false; }
+  absl::optional<std::string> SerializeData() override { return absl::nullopt; }
 };
 
 class BackgroundDataSerializer
@@ -162,7 +162,7 @@ TEST_F(ImportantFileWriterTest, Basic) {
                              SingleThreadTaskRunner::GetCurrentDefault());
   EXPECT_FALSE(PathExists(writer.path()));
   EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
-  writer.WriteNow(std::make_unique<std::string>("foo"));
+  writer.WriteNow("foo");
   RunLoop().RunUntilIdle();
 
   EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
@@ -178,7 +178,7 @@ TEST_F(ImportantFileWriterTest, WriteWithObserver) {
 
   // Confirm that the observer is invoked.
   write_callback_observer_.ObserveNextWriteCallbacks(&writer);
-  writer.WriteNow(std::make_unique<std::string>("foo"));
+  writer.WriteNow("foo");
   RunLoop().RunUntilIdle();
 
   EXPECT_EQ(CALLED_WITH_SUCCESS,
@@ -189,7 +189,7 @@ TEST_F(ImportantFileWriterTest, WriteWithObserver) {
   // Confirm that re-installing the observer works for another write.
   EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
   write_callback_observer_.ObserveNextWriteCallbacks(&writer);
-  writer.WriteNow(std::make_unique<std::string>("bar"));
+  writer.WriteNow("bar");
   RunLoop().RunUntilIdle();
 
   EXPECT_EQ(CALLED_WITH_SUCCESS,
@@ -200,7 +200,7 @@ TEST_F(ImportantFileWriterTest, WriteWithObserver) {
   // Confirm that writing again without re-installing the observer doesn't
   // result in a notification.
   EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
-  writer.WriteNow(std::make_unique<std::string>("baz"));
+  writer.WriteNow("baz");
   RunLoop().RunUntilIdle();
 
   EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
@@ -216,7 +216,7 @@ TEST_F(ImportantFileWriterTest, FailedWriteWithObserver) {
   EXPECT_FALSE(PathExists(writer.path()));
   EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
   write_callback_observer_.ObserveNextWriteCallbacks(&writer);
-  writer.WriteNow(std::make_unique<std::string>("foo"));
+  writer.WriteNow("foo");
   RunLoop().RunUntilIdle();
 
   // Confirm that the write observer was invoked with its boolean parameter set
@@ -242,7 +242,7 @@ TEST_F(ImportantFileWriterTest, CallbackRunsOnWriterThread) {
                                 base::Unretained(&wait_helper)));
 
   write_callback_observer_.ObserveNextWriteCallbacks(&writer);
-  writer.WriteNow(std::make_unique<std::string>("foo"));
+  writer.WriteNow("foo");
   RunLoop().RunUntilIdle();
 
   // Expect the callback to not have been executed before the
@@ -337,7 +337,7 @@ TEST_F(ImportantFileWriterTest, ScheduleWrite_WriteNow) {
   DataSerializer serializer("foo");
   writer.ScheduleWrite(&serializer);
   EXPECT_TRUE(writer.HasPendingWrite());
-  writer.WriteNow(std::make_unique<std::string>("bar"));
+  writer.WriteNow("bar");
   EXPECT_FALSE(writer.HasPendingWrite());
   EXPECT_FALSE(timer.IsRunning());
 
@@ -380,11 +380,10 @@ TEST_F(ImportantFileWriterTest, ScheduleWriteWithBackgroundDataSerializer) {
   EXPECT_FALSE(writer.HasPendingWrite());
   ASSERT_FALSE(file_writer_thread.task_runner()->RunsTasksInCurrentSequence());
   BackgroundDataSerializer serializer(
-      base::BindLambdaForTesting([&](std::string* data) {
+      base::BindLambdaForTesting([&]() -> absl::optional<std::string> {
         EXPECT_TRUE(
             file_writer_thread.task_runner()->RunsTasksInCurrentSequence());
-        *data = "foo";
-        return true;
+        return "foo";
       }));
   writer.ScheduleWriteWithBackgroundDataSerializer(&serializer);
   EXPECT_TRUE(writer.HasPendingWrite());
@@ -417,10 +416,10 @@ TEST_F(ImportantFileWriterTest,
   EXPECT_FALSE(writer.HasPendingWrite());
   ASSERT_FALSE(file_writer_thread.task_runner()->RunsTasksInCurrentSequence());
   BackgroundDataSerializer serializer(
-      base::BindLambdaForTesting([&](std::string* data) {
+      base::BindLambdaForTesting([&]() -> absl::optional<std::string> {
         EXPECT_TRUE(
             file_writer_thread.task_runner()->RunsTasksInCurrentSequence());
-        return false;
+        return absl::nullopt;
       }));
   writer.ScheduleWriteWithBackgroundDataSerializer(&serializer);
   EXPECT_TRUE(writer.HasPendingWrite());

components/account_manager_core/chromeos/account_manager.cc

@@ -639,7 +639,7 @@ void AccountManager::PersistAccountsAsync() {
 
   // Schedule (immediately) a non-blocking write.
   DCHECK(writer_);
-  writer_->WriteNow(std::make_unique<std::string>(GetSerializedAccounts()));
+  writer_->WriteNow(GetSerializedAccounts());
 }
 
 std::string AccountManager::GetSerializedAccounts() {

components/bookmarks/browser/bookmark_storage.cc

@@ -14,7 +14,7 @@
 #include "base/functional/bind.h"
 #include "base/guid.h"
 #include "base/json/json_file_value_serializer.h"
-#include "base/json/json_string_value_serializer.h"
+#include "base/json/json_writer.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/task/sequenced_task_runner.h"
 #include "base/task/task_traits.h"
@@ -90,11 +90,14 @@ BookmarkStorage::GetSerializedDataProducerForBackgroundSequence() {
       codec.Encode(model_, model_->client()->EncodeBookmarkSyncMetadata()));
 
   return base::BindOnce(
-      [](base::Value value, std::string* output) {
+      [](base::Value value) -> absl::optional<std::string> {
         // This runs on the background sequence.
-        JSONStringValueSerializer serializer(output);
-        serializer.set_pretty_print(true);
-        return serializer.Serialize(value);
+        std::string output;
+        if (!base::JSONWriter::WriteWithOptions(
+                value, base::JSONWriter::OPTIONS_PRETTY_PRINT, &output)) {
+          return absl::nullopt;
+        }
+        return output;
       },
       std::move(value));
 }

components/browsing_topics/browsing_topics_state.cc

@@ -8,7 +8,7 @@
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/json/json_file_value_serializer.h"
-#include "base/json/json_string_value_serializer.h"
+#include "base/json/json_writer.h"
 #include "base/json/values_util.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/task/task_traits.h"
@@ -222,11 +222,14 @@ BrowsingTopicsState::GetSerializedDataProducerForBackgroundSequence() {
   DCHECK(loaded_);
 
   return base::BindOnce(
-      [](base::Value value, std::string* output) {
+      [](base::Value value) -> absl::optional<std::string> {
         // This runs on the background sequence.
-        JSONStringValueSerializer serializer(output);
-        serializer.set_pretty_print(true);
-        return serializer.Serialize(value);
+        std::string output;
+        if (!base::JSONWriter::WriteWithOptions(
+                value, base::JSONWriter::OPTIONS_PRETTY_PRINT, &output)) {
+          return absl::nullopt;
+        }
+        return output;
       },
       base::Value(ToDictValue()));
 }

components/prefs/json_pref_store.cc

@@ -17,10 +17,11 @@
 #include "base/functional/callback.h"
 #include "base/functional/callback_helpers.h"
 #include "base/json/json_file_value_serializer.h"
-#include "base/json/json_string_value_serializer.h"
+#include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/metrics/histogram.h"
+#include "base/notreached.h"
 #include "base/observer_list.h"
 #include "base/ranges/algorithm.h"
 #include "base/strings/string_number_conversions.h"
@@ -152,21 +153,18 @@ const char* GetHistogramSuffix(const base::FilePath& path) {
   return it != kAllowList.end() ? *it : "";
 }
 
-bool DoSerialize(base::ValueView value,
-                 const base::FilePath& path,
-                 std::string* output) {
-  JSONStringValueSerializer serializer(output);
-  serializer.set_pretty_print(false);
-  const bool success = serializer.Serialize(value);
-  if (!success) {
+absl::optional<std::string> DoSerialize(base::ValueView value,
+                                        const base::FilePath& path) {
+  std::string output;
+  if (!base::JSONWriter::Write(value, &output)) {
     // Failed to serialize prefs file. Backup the existing prefs file and
     // crash.
     BackupPrefsFile(path);
-    CHECK(false) << "Failed to serialize preferences : " << path
-                 << "\nBacked up under "
-                 << path.ReplaceExtension(kBadExtension);
+    NOTREACHED_NORETURN() << "Failed to serialize preferences : " << path
+                          << "\nBacked up under "
+                          << path.ReplaceExtension(kBadExtension);
   }
-  return success;
+  return output;
 }
 
 }  // namespace
@@ -511,9 +509,9 @@ JsonPrefStore::~JsonPrefStore() {
   CommitPendingWrite();
 }
 
-bool JsonPrefStore::SerializeData(std::string* output) {
+absl::optional<std::string> JsonPrefStore::SerializeData() {
   PerformPreserializationTasks();
-  return DoSerialize(prefs_, path_, output);
+  return DoSerialize(prefs_, path_);
 }
 
 base::ImportantFileWriter::BackgroundDataProducerCallback

components/prefs/json_pref_store.h

@@ -25,6 +25,7 @@
 #include "components/prefs/persistent_pref_store.h"
 #include "components/prefs/pref_filter.h"
 #include "components/prefs/prefs_export.h"
+#include "third_party/abseil-cpp/absl/types/optional.h"
 
 class PrefFilter;
 
@@ -165,7 +166,7 @@ class COMPONENTS_PREFS_EXPORT JsonPrefStore
   void OnFileRead(std::unique_ptr<ReadResult> read_result);
 
   // ImportantFileWriter::DataSerializer overrides:
-  bool SerializeData(std::string* output) override;
+  absl::optional<std::string> SerializeData() override;
   // ImportantFileWriter::BackgroundDataSerializer implementation.
   base::ImportantFileWriter::BackgroundDataProducerCallback
   GetSerializedDataProducerForBackgroundSequence() override;