[PrefService] Introduce a synchronous option to CommitPendingWrite()

This is required to remove the |local_state_task_runner| member
of BrowserProcessImpl only used to implicitly wait on pref store.
Ref. https://crrev.com/c/1163628.
Synchronous callback semantics are required on EndSession() as a nested
RunLoop is not suitable to observe a reply.
https://chromium-review.googlesource.com/c/chromium/src/+/1163628/8/chrome/browser/browser_process_impl.cc#594

Also implemented in services/preferences' SegregatedPrefStore but
not in the Mojom interface where I don't think it's used yet? Or if
it is then it was already wrong as |local_state_task_runner| is
decoupled from that Mojom. The DCHECK will tell and make this future
proof.

Bug: 848615
Cq-Include-Trybots: luci.chromium.try:linux_mojo;master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ie72f2d30d30bfa7f96a04d780d1591949a173b78
Reviewed-on: https://chromium-review.googlesource.com/1164522Reviewed-by: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581324}

components/prefs/json_pref_store.cc

@@ -272,7 +272,9 @@ void JsonPrefStore::ReadPrefsAsync(ReadErrorDelegate* error_delegate) {
       base::Bind(&JsonPrefStore::OnFileRead, AsWeakPtr()));
 }
 
-void JsonPrefStore::CommitPendingWrite(base::OnceClosure done_callback) {
+void JsonPrefStore::CommitPendingWrite(
+    base::OnceClosure reply_callback,
+    base::OnceClosure synchronous_done_callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   // Schedule a write for any lossy writes that are outstanding to ensure that
@@ -282,13 +284,19 @@ void JsonPrefStore::CommitPendingWrite(base::OnceClosure done_callback) {
   if (writer_.HasPendingWrite() && !read_only_)
     writer_.DoScheduledWrite();
 
-  if (done_callback) {
-    // Since disk operations occur on |file_task_runner_|, the reply of a task
-    // posted to |file_task_runner_| will run after currently pending disk
-    // operations. Also, by definition of PostTaskAndReply(), the reply will run
-    // on the current sequence.
+  // Since disk operations occur on |file_task_runner_|, the reply of a task
+  // posted to |file_task_runner_| will run after currently pending disk
+  // operations. Also, by definition of PostTaskAndReply(), the reply (in the
+  // |reply_callback| case will run on the current sequence.
+
+  if (synchronous_done_callback) {
+    file_task_runner_->PostTask(FROM_HERE,
+                                std::move(synchronous_done_callback));
+  }
+
+  if (reply_callback) {
     file_task_runner_->PostTaskAndReply(FROM_HERE, base::DoNothing(),
-                                        std::move(done_callback));
+                                        std::move(reply_callback));
   }
 }
 
@@ -444,7 +452,7 @@ void JsonPrefStore::OnFileRead(std::unique_ptr<ReadResult> read_result) {
 
 JsonPrefStore::~JsonPrefStore() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  CommitPendingWrite(base::OnceClosure());
+  CommitPendingWrite();
 }
 
 bool JsonPrefStore::SerializeData(std::string* output) {

components/prefs/json_pref_store.h

@@ -94,7 +94,10 @@ class COMPONENTS_PREFS_EXPORT JsonPrefStore
   // See details in pref_filter.h.
   PrefReadError ReadPrefs() override;
   void ReadPrefsAsync(ReadErrorDelegate* error_delegate) override;
-  void CommitPendingWrite(base::OnceClosure done_callback) override;
+  void CommitPendingWrite(
+      base::OnceClosure reply_callback = base::OnceClosure(),
+      base::OnceClosure synchronous_done_callback =
+          base::OnceClosure()) override;
   void SchedulePendingLossyWrites() override;
   void ReportValueChanged(const std::string& key, uint32_t flags) override;
 

components/prefs/json_pref_store_unittest.cc

@@ -10,6 +10,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/compiler_specific.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/location.h"
@@ -22,6 +23,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_task_environment.h"
 #include "base/test/simple_test_clock.h"
@@ -130,19 +132,50 @@ class MockReadErrorDelegate : public PersistentPrefStore::ReadErrorDelegate {
 };
 
 enum class CommitPendingWriteMode {
+  // Basic mode.
   WITHOUT_CALLBACK,
+  // With reply callback.
   WITH_CALLBACK,
+  // With synchronous notify callback (synchronous after the write -- shouldn't
+  // require pumping messages to observe).
+  WITH_SYNCHRONOUS_CALLBACK,
 };
 
+base::test::ScopedTaskEnvironment::ExecutionMode GetExecutionMode(
+    CommitPendingWriteMode commit_mode) {
+  switch (commit_mode) {
+    case CommitPendingWriteMode::WITHOUT_CALLBACK:
+      FALLTHROUGH;
+    case CommitPendingWriteMode::WITH_CALLBACK:
+      return base::test::ScopedTaskEnvironment::ExecutionMode::QUEUED;
+    case CommitPendingWriteMode::WITH_SYNCHRONOUS_CALLBACK:
+      // Synchronous callbacks require async tasks to run on their own.
+      return base::test::ScopedTaskEnvironment::ExecutionMode::ASYNC;
+  }
+}
+
 void CommitPendingWrite(
     JsonPrefStore* pref_store,
     CommitPendingWriteMode commit_pending_write_mode,
     base::test::ScopedTaskEnvironment* scoped_task_environment) {
-  if (commit_pending_write_mode == CommitPendingWriteMode::WITHOUT_CALLBACK) {
-    pref_store->CommitPendingWrite(OnceClosure());
-    scoped_task_environment->RunUntilIdle();
-  } else {
-    TestCommitPendingWriteWithCallback(pref_store, scoped_task_environment);
+  switch (commit_pending_write_mode) {
+    case CommitPendingWriteMode::WITHOUT_CALLBACK: {
+      pref_store->CommitPendingWrite();
+      scoped_task_environment->RunUntilIdle();
+      break;
+    }
+    case CommitPendingWriteMode::WITH_CALLBACK: {
+      TestCommitPendingWriteWithCallback(pref_store, scoped_task_environment);
+      break;
+    }
+    case CommitPendingWriteMode::WITH_SYNCHRONOUS_CALLBACK: {
+      base::WaitableEvent written;
+      pref_store->CommitPendingWrite(
+          base::OnceClosure(),
+          base::BindOnce(&base::WaitableEvent::Signal, Unretained(&written)));
+      written.Wait();
+      break;
+    }
   }
 }
 
@@ -152,7 +185,7 @@ class JsonPrefStoreTest
   JsonPrefStoreTest()
       : scoped_task_environment_(
             base::test::ScopedTaskEnvironment::MainThreadType::DEFAULT,
-            base::test::ScopedTaskEnvironment::ExecutionMode::QUEUED) {}
+            GetExecutionMode(GetParam())) {}
 
  protected:
   void SetUp() override {
@@ -170,7 +203,7 @@ class JsonPrefStoreTest
 }  // namespace
 
 // Test fallback behavior for a nonexistent file.
-TEST_F(JsonPrefStoreTest, NonExistentFile) {
+TEST_P(JsonPrefStoreTest, NonExistentFile) {
   base::FilePath bogus_input_file = temp_dir_.GetPath().AppendASCII("read.txt");
   ASSERT_FALSE(PathExists(bogus_input_file));
   auto pref_store = base::MakeRefCounted<JsonPrefStore>(bogus_input_file);
@@ -180,7 +213,7 @@ TEST_F(JsonPrefStoreTest, NonExistentFile) {
 }
 
 // Test fallback behavior for an invalid file.
-TEST_F(JsonPrefStoreTest, InvalidFile) {
+TEST_P(JsonPrefStoreTest, InvalidFile) {
   base::FilePath invalid_file = temp_dir_.GetPath().AppendASCII("invalid.json");
   ASSERT_LT(0, base::WriteFile(invalid_file,
                                kInvalidJson, arraysize(kInvalidJson) - 1));
@@ -371,7 +404,7 @@ TEST_P(JsonPrefStoreTest, PreserveEmptyValues) {
 
 // This test is just documenting some potentially non-obvious behavior. It
 // shouldn't be taken as normative.
-TEST_F(JsonPrefStoreTest, RemoveClearsEmptyParent) {
+TEST_P(JsonPrefStoreTest, RemoveClearsEmptyParent) {
   FilePath pref_file = temp_dir_.GetPath().AppendASCII("empty_values.json");
 
   auto pref_store = base::MakeRefCounted<JsonPrefStore>(pref_file);
@@ -390,7 +423,7 @@ TEST_F(JsonPrefStoreTest, RemoveClearsEmptyParent) {
 }
 
 // Tests asynchronous reading of the file when there is no file.
-TEST_F(JsonPrefStoreTest, AsyncNonExistingFile) {
+TEST_P(JsonPrefStoreTest, AsyncNonExistingFile) {
   base::FilePath bogus_input_file = temp_dir_.GetPath().AppendASCII("read.txt");
   ASSERT_FALSE(PathExists(bogus_input_file));
   auto pref_store = base::MakeRefCounted<JsonPrefStore>(bogus_input_file);
@@ -514,7 +547,7 @@ TEST_P(JsonPrefStoreTest, ReadAsyncWithInterceptor) {
                             &scoped_task_environment_);
 }
 
-TEST_F(JsonPrefStoreTest, WriteCountHistogramTestBasic) {
+TEST_P(JsonPrefStoreTest, WriteCountHistogramTestBasic) {
   base::HistogramTester histogram_tester;
 
   SimpleTestClock* test_clock = new SimpleTestClock;
@@ -542,7 +575,7 @@ TEST_F(JsonPrefStoreTest, WriteCountHistogramTestBasic) {
   ASSERT_TRUE(histogram.GetHistogram()->HasConstructionArguments(1, 30, 31));
 }
 
-TEST_F(JsonPrefStoreTest, WriteCountHistogramTestSinglePeriod) {
+TEST_P(JsonPrefStoreTest, WriteCountHistogramTestSinglePeriod) {
   base::HistogramTester histogram_tester;
 
   SimpleTestClock* test_clock = new SimpleTestClock;
@@ -580,7 +613,7 @@ TEST_F(JsonPrefStoreTest, WriteCountHistogramTestSinglePeriod) {
   histogram_tester.ExpectTotalCount(histogram_name, 1);
 }
 
-TEST_F(JsonPrefStoreTest, WriteCountHistogramTestMultiplePeriods) {
+TEST_P(JsonPrefStoreTest, WriteCountHistogramTestMultiplePeriods) {
   base::HistogramTester histogram_tester;
 
   SimpleTestClock* test_clock = new SimpleTestClock;
@@ -620,7 +653,7 @@ TEST_F(JsonPrefStoreTest, WriteCountHistogramTestMultiplePeriods) {
   histogram_tester.ExpectTotalCount(histogram_name, 3);
 }
 
-TEST_F(JsonPrefStoreTest, WriteCountHistogramTestPeriodWithGaps) {
+TEST_P(JsonPrefStoreTest, WriteCountHistogramTestPeriodWithGaps) {
   base::HistogramTester histogram_tester;
 
   SimpleTestClock* test_clock = new SimpleTestClock;
@@ -671,6 +704,10 @@ INSTANTIATE_TEST_CASE_P(
     WithCallback,
     JsonPrefStoreTest,
     ::testing::Values(CommitPendingWriteMode::WITH_CALLBACK));
+INSTANTIATE_TEST_CASE_P(
+    WithSynchronousCallback,
+    JsonPrefStoreTest,
+    ::testing::Values(CommitPendingWriteMode::WITH_SYNCHRONOUS_CALLBACK));
 
 class JsonPrefStoreLossyWriteTest : public JsonPrefStoreTest {
  public:
@@ -706,7 +743,7 @@ class JsonPrefStoreLossyWriteTest : public JsonPrefStoreTest {
   DISALLOW_COPY_AND_ASSIGN(JsonPrefStoreLossyWriteTest);
 };
 
-TEST_F(JsonPrefStoreLossyWriteTest, LossyWriteBasic) {
+TEST_P(JsonPrefStoreLossyWriteTest, LossyWriteBasic) {
   scoped_refptr<JsonPrefStore> pref_store = CreatePrefStore();
   ImportantFileWriter* file_writer = GetImportantFileWriter(pref_store.get());
 
@@ -751,7 +788,7 @@ TEST_F(JsonPrefStoreLossyWriteTest, LossyWriteBasic) {
             GetTestFileContents());
 }
 
-TEST_F(JsonPrefStoreLossyWriteTest, LossyWriteMixedLossyFirst) {
+TEST_P(JsonPrefStoreLossyWriteTest, LossyWriteMixedLossyFirst) {
   scoped_refptr<JsonPrefStore> pref_store = CreatePrefStore();
   ImportantFileWriter* file_writer = GetImportantFileWriter(pref_store.get());
 
@@ -773,7 +810,7 @@ TEST_F(JsonPrefStoreLossyWriteTest, LossyWriteMixedLossyFirst) {
   ASSERT_FALSE(file_writer->HasPendingWrite());
 }
 
-TEST_F(JsonPrefStoreLossyWriteTest, LossyWriteMixedLossySecond) {
+TEST_P(JsonPrefStoreLossyWriteTest, LossyWriteMixedLossySecond) {
   scoped_refptr<JsonPrefStore> pref_store = CreatePrefStore();
   ImportantFileWriter* file_writer = GetImportantFileWriter(pref_store.get());
 
@@ -795,7 +832,7 @@ TEST_F(JsonPrefStoreLossyWriteTest, LossyWriteMixedLossySecond) {
   ASSERT_FALSE(file_writer->HasPendingWrite());
 }
 
-TEST_F(JsonPrefStoreLossyWriteTest, ScheduleLossyWrite) {
+TEST_P(JsonPrefStoreLossyWriteTest, ScheduleLossyWrite) {
   scoped_refptr<JsonPrefStore> pref_store = CreatePrefStore();
   ImportantFileWriter* file_writer = GetImportantFileWriter(pref_store.get());
 
@@ -815,6 +852,19 @@ TEST_F(JsonPrefStoreLossyWriteTest, ScheduleLossyWrite) {
   ASSERT_EQ("{\"lossy\":\"lossy\"}", GetTestFileContents());
 }
 
+INSTANTIATE_TEST_CASE_P(
+    WithoutCallback,
+    JsonPrefStoreLossyWriteTest,
+    ::testing::Values(CommitPendingWriteMode::WITHOUT_CALLBACK));
+INSTANTIATE_TEST_CASE_P(
+    WithReply,
+    JsonPrefStoreLossyWriteTest,
+    ::testing::Values(CommitPendingWriteMode::WITH_CALLBACK));
+INSTANTIATE_TEST_CASE_P(
+    WithNotify,
+    JsonPrefStoreLossyWriteTest,
+    ::testing::Values(CommitPendingWriteMode::WITH_SYNCHRONOUS_CALLBACK));
+
 class SuccessfulWriteReplyObserver {
  public:
   SuccessfulWriteReplyObserver() = default;
@@ -912,13 +962,13 @@ WriteCallbacksObserver::GetAndResetPostWriteObservationState() {
   return state;
 }
 
-class JsonPrefStoreCallbackTest : public JsonPrefStoreTest {
+class JsonPrefStoreCallbackTest : public testing::Test {
  public:
   JsonPrefStoreCallbackTest() = default;
 
  protected:
   void SetUp() override {
-    JsonPrefStoreTest::SetUp();
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     test_file_ = temp_dir_.GetPath().AppendASCII("test.json");
   }
 
@@ -943,6 +993,13 @@ class JsonPrefStoreCallbackTest : public JsonPrefStoreTest {
   SuccessfulWriteReplyObserver successful_write_reply_observer_;
   WriteCallbacksObserver write_callback_observer_;
 
+ protected:
+  base::test::ScopedTaskEnvironment scoped_task_environment_{
+      base::test::ScopedTaskEnvironment::MainThreadType::DEFAULT,
+      base::test::ScopedTaskEnvironment::ExecutionMode::QUEUED};
+
+  base::ScopedTempDir temp_dir_;
+
  private:
   base::FilePath test_file_;
 

components/prefs/overlay_user_pref_store.cc

@@ -180,8 +180,11 @@ void OverlayUserPrefStore::ReadPrefsAsync(
   OnInitializationCompleted(/* ephemeral */ false, true);
 }
 
-void OverlayUserPrefStore::CommitPendingWrite(base::OnceClosure done_callback) {
-  persistent_user_pref_store_->CommitPendingWrite(std::move(done_callback));
+void OverlayUserPrefStore::CommitPendingWrite(
+    base::OnceClosure reply_callback,
+    base::OnceClosure synchronous_done_callback) {
+  persistent_user_pref_store_->CommitPendingWrite(
+      std::move(reply_callback), std::move(synchronous_done_callback));
   // We do not write our content intentionally.
 }
 

components/prefs/overlay_user_pref_store.h

@@ -57,7 +57,8 @@ class COMPONENTS_PREFS_EXPORT OverlayUserPrefStore
   PrefReadError GetReadError() const override;
   PrefReadError ReadPrefs() override;
   void ReadPrefsAsync(ReadErrorDelegate* delegate) override;
-  void CommitPendingWrite(base::OnceClosure done_callback) override;
+  void CommitPendingWrite(base::OnceClosure reply_callback,
+                          base::OnceClosure synchronous_done_callback) override;
   void SchedulePendingLossyWrites() override;
   void ReportValueChanged(const std::string& key, uint32_t flags) override;
 

components/prefs/persistent_pref_store.cc

@@ -8,11 +8,20 @@
 
 #include "base/threading/sequenced_task_runner_handle.h"
 
-void PersistentPrefStore::CommitPendingWrite(base::OnceClosure done_callback) {
+void PersistentPrefStore::CommitPendingWrite(
+    base::OnceClosure reply_callback,
+    base::OnceClosure synchronous_done_callback) {
   // Default behavior for PersistentPrefStore implementation that don't issue
   // disk operations: schedule the callback immediately.
-  if (done_callback) {
+  // |synchronous_done_callback| is allowed to be invoked synchronously (and
+  // must be here since we have no other way to post it which isn't the current
+  // sequence).
+
+  if (synchronous_done_callback)
+    std::move(synchronous_done_callback).Run();
+
+  if (reply_callback) {
     base::SequencedTaskRunnerHandle::Get()->PostTask(FROM_HERE,
-                                                     std::move(done_callback));
+                                                     std::move(reply_callback));
   }
 }

components/prefs/persistent_pref_store.h

@@ -62,10 +62,16 @@ class COMPONENTS_PREFS_EXPORT PersistentPrefStore : public WriteablePrefStore {
   // Owns |error_delegate|.
   virtual void ReadPrefsAsync(ReadErrorDelegate* error_delegate) = 0;
 
-  // Starts an asynchronous attempt to commit pending writes to disk. Posts a
-  // task to run |done_callback| on the current sequence when disk operations,
-  // if any, are complete (even if they are unsuccessful).
-  virtual void CommitPendingWrite(base::OnceClosure done_callback);
+  // Lands pending writes to disk. |reply_callback| will be posted to the
+  // current sequence when changes have been written.
+  // |synchronous_done_callback| on the other hand will be invoked right away
+  // wherever the writes complete (could even be invoked synchronously if no
+  // writes need to occur); this is useful when the current thread cannot pump
+  // messages to observe the reply (e.g. nested loops banned on main thread
+  // during shutdown). |synchronous_done_callback| must be thread-safe.
+  virtual void CommitPendingWrite(
+      base::OnceClosure reply_callback = base::OnceClosure(),
+      base::OnceClosure synchronous_done_callback = base::OnceClosure());
 
   // Schedule a write if there is any lossy data pending. Unlike
   // CommitPendingWrite() this does not immediately sync to disk, instead it

components/prefs/pref_service.cc

@@ -97,13 +97,12 @@ void PrefService::InitFromStorage(bool async) {
   }
 }
 
-void PrefService::CommitPendingWrite() {
-  CommitPendingWrite(base::OnceClosure());
-}
-
-void PrefService::CommitPendingWrite(base::OnceClosure done_callback) {
+void PrefService::CommitPendingWrite(
+    base::OnceClosure reply_callback,
+    base::OnceClosure synchronous_done_callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  user_pref_store_->CommitPendingWrite(std::move(done_callback));
+  user_pref_store_->CommitPendingWrite(std::move(reply_callback),
+                                       std::move(synchronous_done_callback));
 }
 
 void PrefService::SchedulePendingLossyWrites() {

components/prefs/pref_service.h

@@ -176,13 +176,16 @@ class COMPONENTS_PREFS_EXPORT PrefService {
   virtual ~PrefService();
 
   // Lands pending writes to disk. This should only be used if we need to save
-  // immediately (basically, during shutdown).
-  void CommitPendingWrite();
-
-  // Lands pending writes to disk. This should only be used if we need to save
-  // immediately. |done_callback| will be invoked when changes have been
-  // written.
-  void CommitPendingWrite(base::OnceClosure done_callback);
+  // immediately (basically, during shutdown). |reply_callback| will be posted
+  // to the current sequence when changes have been written.
+  // |synchronous_done_callback| on the other hand will be invoked right away
+  // wherever the writes complete (could even be invoked synchronously if no
+  // writes need to occur); this is useful when the current thread cannot pump
+  // messages to observe the reply (e.g. nested loops banned on main thread
+  // during shutdown). |synchronous_done_callback| must be thread-safe.
+  void CommitPendingWrite(
+      base::OnceClosure reply_callback = base::OnceClosure(),
+      base::OnceClosure synchronous_done_callback = base::OnceClosure());
 
   // Schedule a write if there is any lossy data pending. Unlike
   // CommitPendingWrite() this does not immediately sync to disk, instead it

components/prefs/testing_pref_store.cc

@@ -98,9 +98,12 @@ void TestingPrefStore::ReadPrefsAsync(ReadErrorDelegate* error_delegate) {
     NotifyInitializationCompleted();
 }
 
-void TestingPrefStore::CommitPendingWrite(base::OnceClosure done_callback) {
+void TestingPrefStore::CommitPendingWrite(
+    base::OnceClosure reply_callback,
+    base::OnceClosure synchronous_done_callback) {
   committed_ = true;
-  PersistentPrefStore::CommitPendingWrite(std::move(done_callback));
+  PersistentPrefStore::CommitPendingWrite(std::move(reply_callback),
+                                          std::move(synchronous_done_callback));
 }
 
 void TestingPrefStore::SchedulePendingLossyWrites() {}

components/prefs/testing_pref_store.h

@@ -45,7 +45,8 @@ class TestingPrefStore : public PersistentPrefStore {
   PrefReadError GetReadError() const override;
   PersistentPrefStore::PrefReadError ReadPrefs() override;
   void ReadPrefsAsync(ReadErrorDelegate* error_delegate) override;
-  void CommitPendingWrite(base::OnceClosure done_callback) override;
+  void CommitPendingWrite(base::OnceClosure reply_callback,
+                          base::OnceClosure synchronous_done_callback) override;
   void SchedulePendingLossyWrites() override;
 
   // Marks the store as having completed initialization.

services/preferences/persistent_pref_store_impl.cc

@@ -129,6 +129,10 @@ class PersistentPrefStoreImpl::Connection : public mojom::PersistentPrefStore {
   }
 
   void CommitPendingWrite(CommitPendingWriteCallback done_callback) override {
+    // Note: PersistentPrefStore's synchronous callback part of the
+    // CommitPendingWrite() API isn't supported on mojom::PersistentPrefStore at
+    // the moment (see PersistentPrefStoreClient::CommitPendingWrite() for
+    // details).
     pref_store_->CommitPendingWrite(std::move(done_callback));
   }
   void SchedulePendingLossyWrites() override {

services/preferences/persistent_pref_store_impl_unittest.cc

@@ -23,9 +23,12 @@ namespace {
 
 class PersistentPrefStoreMock : public InMemoryPrefStore {
  public:
-  void CommitPendingWrite(base::OnceClosure callback) override {
+  void CommitPendingWrite(
+      base::OnceClosure reply_callback,
+      base::OnceClosure synchronous_done_callback) override {
     CommitPendingWriteMock();
-    InMemoryPrefStore::CommitPendingWrite(std::move(callback));
+    InMemoryPrefStore::CommitPendingWrite(std::move(reply_callback),
+                                          std::move(synchronous_done_callback));
   }
 
   MOCK_METHOD0(CommitPendingWriteMock, void());

services/preferences/public/cpp/persistent_pref_store_client.cc

@@ -225,11 +225,17 @@ void PersistentPrefStoreClient::ReadPrefsAsync(
     ReadErrorDelegate* error_delegate) {}
 
 void PersistentPrefStoreClient::CommitPendingWrite(
-    base::OnceClosure done_callback) {
+    base::OnceClosure reply_callback,
+    base::OnceClosure synchronous_done_callback) {
+  // Supporting |synchronous_done_callback| semantics would require a sync IPC.
+  // This isn't implemented as such at the moment as this functionality isn't
+  // used in practice (if it ever becomes necessary, this check will fire).
+  DCHECK(!synchronous_done_callback);
+
   DCHECK(pref_store_);
   if (!pending_writes_.empty())
     FlushPendingWrites();
-  pref_store_->CommitPendingWrite(std::move(done_callback));
+  pref_store_->CommitPendingWrite(std::move(reply_callback));
 }
 
 void PersistentPrefStoreClient::SchedulePendingLossyWrites() {
@@ -251,7 +257,7 @@ PersistentPrefStoreClient::~PersistentPrefStoreClient() {
   if (!pref_store_)
     return;
 
-  CommitPendingWrite(base::OnceClosure());
+  CommitPendingWrite();
 }
 
 void PersistentPrefStoreClient::QueueWrite(

services/preferences/public/cpp/persistent_pref_store_client.h

@@ -54,7 +54,10 @@ class PersistentPrefStoreClient
   PrefReadError GetReadError() const override;
   PrefReadError ReadPrefs() override;
   void ReadPrefsAsync(ReadErrorDelegate* error_delegate) override;
-  void CommitPendingWrite(base::OnceClosure done_callback) override;
+  void CommitPendingWrite(
+      base::OnceClosure reply_callback = base::OnceClosure(),
+      base::OnceClosure synchronous_done_callback =
+          base::OnceClosure()) override;
   void SchedulePendingLossyWrites() override;
   void ClearMutableValues() override;
   void OnStoreDeletionFromDisk() override;

services/preferences/tracked/segregated_pref_store.cc

@@ -161,12 +161,27 @@ void SegregatedPrefStore::ReadPrefsAsync(ReadErrorDelegate* error_delegate) {
   selected_pref_store_->ReadPrefsAsync(NULL);
 }
 
-void SegregatedPrefStore::CommitPendingWrite(base::OnceClosure done_callback) {
-  base::RepeatingClosure done_callback_wrapper =
-      done_callback ? base::BarrierClosure(2, std::move(done_callback))
-                    : base::RepeatingClosure();
-  default_pref_store_->CommitPendingWrite(done_callback_wrapper);
-  selected_pref_store_->CommitPendingWrite(done_callback_wrapper);
+void SegregatedPrefStore::CommitPendingWrite(
+    base::OnceClosure reply_callback,
+    base::OnceClosure synchronous_done_callback) {
+  // A BarrierClosure will run its callback wherever the last instance of the
+  // returned wrapper is invoked. As such it is guaranteed to respect the reply
+  // vs synchronous semantics assuming |default_pref_store_| and
+  // |selected_pref_store_| honor it.
+
+  base::RepeatingClosure reply_callback_wrapper =
+      reply_callback ? base::BarrierClosure(2, std::move(reply_callback))
+                     : base::RepeatingClosure();
+
+  base::RepeatingClosure synchronous_callback_wrapper =
+      synchronous_done_callback
+          ? base::BarrierClosure(2, std::move(synchronous_done_callback))
+          : base::RepeatingClosure();
+
+  default_pref_store_->CommitPendingWrite(reply_callback_wrapper,
+                                          synchronous_callback_wrapper);
+  selected_pref_store_->CommitPendingWrite(reply_callback_wrapper,
+                                           synchronous_callback_wrapper);
 }
 
 void SegregatedPrefStore::SchedulePendingLossyWrites() {

services/preferences/tracked/segregated_pref_store.h

@@ -72,7 +72,10 @@ class SegregatedPrefStore : public PersistentPrefStore {
   PrefReadError GetReadError() const override;
   PrefReadError ReadPrefs() override;
   void ReadPrefsAsync(ReadErrorDelegate* error_delegate) override;
-  void CommitPendingWrite(base::OnceClosure done_callback) override;
+  void CommitPendingWrite(
+      base::OnceClosure reply_callback = base::OnceClosure(),
+      base::OnceClosure synchronous_done_callback =
+          base::OnceClosure()) override;
   void SchedulePendingLossyWrites() override;
   void ClearMutableValues() override;
   void OnStoreDeletionFromDisk() override;

services/preferences/tracked/segregated_pref_store_unittest.cc

@@ -13,6 +13,7 @@
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
 #include "base/run_loop.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/test/scoped_task_environment.h"
 #include "base/values.h"
 #include "components/prefs/persistent_pref_store.h"
@@ -56,8 +57,13 @@ class MockReadErrorDelegate : public PersistentPrefStore::ReadErrorDelegate {
 };
 
 enum class CommitPendingWriteMode {
+  // Basic mode.
   WITHOUT_CALLBACK,
+  // With reply callback.
   WITH_CALLBACK,
+  // With synchronous notify callback (synchronous after the write -- shouldn't
+  // require pumping messages to observe).
+  WITH_SYNCHRONOUS_CALLBACK,
 };
 
 class SegregatedPrefStoreTest
@@ -131,13 +137,28 @@ TEST_P(SegregatedPrefStoreTest, StoreValues) {
   ASSERT_FALSE(selected_store_->committed());
   ASSERT_FALSE(default_store_->committed());
 
-  if (GetParam() == CommitPendingWriteMode::WITHOUT_CALLBACK) {
-    segregated_store_->CommitPendingWrite(base::OnceClosure());
-    base::RunLoop().RunUntilIdle();
-  } else {
-    base::RunLoop run_loop;
-    segregated_store_->CommitPendingWrite(run_loop.QuitClosure());
-    run_loop.Run();
+  switch (GetParam()) {
+    case CommitPendingWriteMode::WITHOUT_CALLBACK: {
+      segregated_store_->CommitPendingWrite();
+      base::RunLoop().RunUntilIdle();
+      break;
+    }
+
+    case CommitPendingWriteMode::WITH_CALLBACK: {
+      base::RunLoop run_loop;
+      segregated_store_->CommitPendingWrite(run_loop.QuitClosure());
+      run_loop.Run();
+      break;
+    }
+
+    case CommitPendingWriteMode::WITH_SYNCHRONOUS_CALLBACK: {
+      base::WaitableEvent written;
+      segregated_store_->CommitPendingWrite(
+          base::OnceClosure(),
+          base::BindOnce(&base::WaitableEvent::Signal, Unretained(&written)));
+      written.Wait();
+      break;
+    }
   }
 
   ASSERT_TRUE(selected_store_->committed());
@@ -327,3 +348,7 @@ INSTANTIATE_TEST_CASE_P(
     WithCallback,
     SegregatedPrefStoreTest,
     ::testing::Values(CommitPendingWriteMode::WITH_CALLBACK));
+INSTANTIATE_TEST_CASE_P(
+    WithSynchronousCallback,
+    SegregatedPrefStoreTest,
+    ::testing::Values(CommitPendingWriteMode::WITH_SYNCHRONOUS_CALLBACK));