Add synchronous callback support to important_file_writer.cc

components/prefs/json_pref_store.cc

Index: components/prefs/json_pref_store.cc
diff --git a/components/prefs/json_pref_store.cc b/components/prefs/json_pref_store.cc
index 779c2009730366aba4d8fae689d340b2fb81ad71..c3adaad1c0e993792c35297d8c49e91493c980ec 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"
@@ -170,6 +171,8 @@ JsonPrefStore::JsonPrefStore(
       initialized_(false),
       filtering_in_progress_(false),
       pending_lossy_write_(false),
+      has_pending_successful_write_reply_(false),
+      has_pending_write_callback_(false),
       read_error_(PREF_READ_ERROR_NONE),
       write_count_histogram_(writer_.commit_interval(), path_) {
   DCHECK(!path_.empty());
@@ -323,11 +326,58 @@ void JsonPrefStore::ReportValueChanged(const std::string& key, uint32_t flags) {
   ScheduleWrite(flags);
 }
 
-void JsonPrefStore::RegisterOnNextSuccessfulWriteCallback(
-    const base::Closure& on_next_successful_write) {
-  DCHECK(CalledOnValidThread());
+// Static

[gab, 2016/09/19 18:01:30]

"static" (lower-case)

[proberge, 2016/09/19 21:24:25]

Done.

+void JsonPrefStore::PostWriteCallback(
+    base::WeakPtr<JsonPrefStore> calling_store,
+    const base::Callback<void(bool success)>& on_next_write,
+    scoped_refptr<base::SequencedTaskRunner> reply_task_runner,
+    bool write_success) {
+  if (!on_next_write.is_null()) {
+    on_next_write.Run(write_success);
+  }
+
+  // The calling JsonPrefStore can be null when the current profile is being
+  // destroyed (for example during shutdown).
+  if (calling_store) {
+    calling_store->has_pending_write_callback_ = false;

[gab, 2016/09/19 18:01:30]

Member state can only be modified on the owning sequence. This callback runs
off-sequence and as such touching member state without synchronization (e.g.
locks) is racy -- and I don't think we want to add explicit synchronization.

I'm trying to think of the best way to achieve what we want here but will post
these comments before that to give you some head way (or let you find the best
way before I get to it :-)).

[proberge, 2016/09/19 21:24:25]

Hmm. Not sure if this is better than locks. PTAL.
(static instead of member method as it sounds like trouble if the JsonPrefStore
dies after the FileWriter thread PostTasks.
https://cs.chromium.org/chromium/src/docs/callback.md?dr=Ss&q=callback%5C.md&...)

[gab, 2016/09/20 01:27:41]

Yes, much better than locks :-), well done!

+
+    if (calling_store->has_pending_successful_write_reply_) {
+      calling_store->has_pending_successful_write_reply_ = false;
+      if (write_success) {
+        reply_task_runner->PostTask(
+            FROM_HERE, calling_store->on_next_successful_write_reply_);
+      } else {
+        calling_store->RegisterOnNextSuccessfulWriteReply(
+            calling_store->on_next_successful_write_reply_);
+      }
+    }
+  }
+}
+
+void JsonPrefStore::RegisterOnNextSuccessfulWriteReply(
+    const base::Closure& on_next_successful_write_reply_reply) {

[gab, 2016/09/19 18:01:30]

s/on_next_successful_write_reply_reply/on_next_successful_write_reply/

[proberge, 2016/09/19 21:24:25]

Done.

+  DCHECK(!has_pending_successful_write_reply_);
+
+  has_pending_successful_write_reply_ = true;
+  on_next_successful_write_reply_ = on_next_successful_write_reply_reply;
+
+  if (!has_pending_write_callback_) {
+    writer_.RegisterOnNextWriteCallback(
+        base::Bind(&PostWriteCallback, this->AsWeakPtr(),
+                   base::Callback<void(bool success)>(),
+                   base::SequencedTaskRunnerHandle::Get()));
+  }
+}
+
+void JsonPrefStore::RegisterOnNextWriteCallback(
+    const base::Callback<void(bool success)>& on_next_write_callback) {
+  DCHECK(!has_pending_write_callback_);
+
+  has_pending_write_callback_ = true;
 
-  writer_.RegisterOnNextSuccessfulWriteCallback(on_next_successful_write);
+  writer_.RegisterOnNextWriteCallback(
+      base::Bind(&PostWriteCallback, this->AsWeakPtr(), on_next_write_callback,
+                 base::SequencedTaskRunnerHandle::Get()));
 }
 
 void JsonPrefStore::ClearMutableValues() {