Add synchronous callback support to important_file_writer.cc

components/prefs/json_pref_store.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/prefs/json_pref_store.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #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/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/metrics/histogram.h"
 #include "base/sequenced_task_runner.h"
 #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"
 #include "components/prefs/pref_filter.h"
 
 // Result returned from internal read tasks.
 struct JsonPrefStore::ReadResult {
  public:
   ReadResult();
   ~ReadResult();
@@ skipping 128 matching lines @@
     : path_(pref_filename),
       alternate_path_(pref_alternate_filename),
       sequenced_task_runner_(sequenced_task_runner),
       prefs_(new base::DictionaryValue()),
       read_only_(false),
       writer_(pref_filename, sequenced_task_runner),
       pref_filter_(std::move(pref_filter)),
       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());
 }
 
 bool JsonPrefStore::GetValue(const std::string& key,
                              const base::Value** result) const {
   DCHECK(CalledOnValidThread());
 
   base::Value* tmp = nullptr;
@@ skipping 133 matching lines @@
   DCHECK(CalledOnValidThread());
 
   if (pref_filter_)
     pref_filter_->FilterUpdate(key);
 
   FOR_EACH_OBSERVER(PrefStore::Observer, observers_, OnPrefValueChanged(key));
 
   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);
+  }
 
-  writer_.RegisterOnNextSuccessfulWriteCallback(on_next_successful_write);
+  // 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_.RegisterOnNextWriteCallback(
+      base::Bind(&PostWriteCallback, this->AsWeakPtr(), on_next_write_callback,
+                 base::SequencedTaskRunnerHandle::Get()));
 }
 
 void JsonPrefStore::ClearMutableValues() {
   NOTIMPLEMENTED();
 }
 
 void JsonPrefStore::OnFileRead(std::unique_ptr<ReadResult> read_result) {
   DCHECK(CalledOnValidThread());
 
   DCHECK(read_result);
@@ skipping 192 matching lines @@
   DCHECK_EQ(31, num_buckets);
 
   // The histogram below is an expansion of the UMA_HISTOGRAM_CUSTOM_COUNTS
   // macro adapted to allow for a dynamically suffixed histogram name.
   // Note: The factory creates and owns the histogram.
   base::HistogramBase* histogram = base::Histogram::FactoryGet(
       histogram_name, min_value, max_value, num_buckets,
       base::HistogramBase::kUmaTargetedHistogramFlag);
   return histogram;
 }