Add synchronous callback support to important_file_writer.cc

base/files/important_file_writer_unittest.cc

Index: base/files/important_file_writer_unittest.cc
diff --git a/base/files/important_file_writer_unittest.cc b/base/files/important_file_writer_unittest.cc
index 43e051ebcfadf108e3a244420c5a01d5397396a9..98a3450976d52487bd29d9acbcc786dc79ce868e 100644
--- a/base/files/important_file_writer_unittest.cc
+++ b/base/files/important_file_writer_unittest.cc
@@ -71,7 +71,7 @@ class SuccessfulWriteObserver {
 
 void SuccessfulWriteObserver::ObserveNextSuccessfulWrite(
     ImportantFileWriter* writer) {
-  writer->RegisterOnNextSuccessfulWriteCallback(base::Bind(
+  writer->RegisterOnNextSuccessfulWriteReply(base::Bind(
       &SuccessfulWriteObserver::on_successful_write, base::Unretained(this)));
 }
 
@@ -81,6 +81,51 @@ bool SuccessfulWriteObserver::GetAndResetObservationState() {
   return was_successful_write_observed;
 }
 
+class SynchronousWriteCallbackObserver {
+ public:
+  SynchronousWriteCallbackObserver()
+      : write_callback_observed_(false), write_callback_success_value_(false) {}
+
+  // Register on_write() to be called on the next write of |writer|.
+  void ObserveNextWriteCallback(ImportantFileWriter* writer);
+
+  struct WriteCallbackObservationState {
+    bool was_called;
+    bool success_param_value;
+  };

[gab, 2016/08/31 19:45:34]

Use an enum instead of a struct to keep it to a single member (easier
verification below).

i.e.

enum WriteCallbackObservationState {
  NOT_CALLED,
  CALLED_WITH_ERROR,
  CALLED_WITH_SUCCESS,
};

[proberge, 2016/08/31 21:06:44]

Done.

+
+  // Returns true if a write was observed via on_write()
+  // and resets the observation state to false regardless.
+  WriteCallbackObservationState GetAndResetObservationState();
+
+ private:
+  void on_write(bool success) {
+    EXPECT_FALSE(write_callback_observed_);
+    write_callback_observed_ = true;
+    write_callback_success_value_ = success;
+  }
+
+  bool write_callback_observed_;
+  bool write_callback_success_value_;
+
+  DISALLOW_COPY_AND_ASSIGN(SynchronousWriteCallbackObserver);
+};
+
+void SynchronousWriteCallbackObserver::ObserveNextWriteCallback(
+    ImportantFileWriter* writer) {
+  writer->RegisterOnNextWriteSynchronousCallback(base::Bind(
+      &SynchronousWriteCallbackObserver::on_write, base::Unretained(this)));
+}
+
+SynchronousWriteCallbackObserver::WriteCallbackObservationState
+SynchronousWriteCallbackObserver::GetAndResetObservationState() {
+  WriteCallbackObservationState observation_state;
+  observation_state.was_called = write_callback_observed_;
+  observation_state.success_param_value = write_callback_success_value_;
+  write_callback_observed_ = false;
+  return observation_state;
+}
+
 }  // namespace
 
 class ImportantFileWriterTest : public testing::Test {
@@ -93,6 +138,7 @@ class ImportantFileWriterTest : public testing::Test {
 
  protected:
   SuccessfulWriteObserver successful_write_observer_;
+  SynchronousWriteCallbackObserver write_callback_observer_;
   FilePath file_;
   MessageLoop loop_;
 
@@ -104,48 +150,93 @@ TEST_F(ImportantFileWriterTest, Basic) {
   ImportantFileWriter writer(file_, ThreadTaskRunnerHandle::Get());
   EXPECT_FALSE(PathExists(writer.path()));
   EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  EXPECT_FALSE(
+      write_callback_observer_.GetAndResetObservationState().was_called);
   writer.WriteNow(WrapUnique(new std::string("foo")));
   RunLoop().RunUntilIdle();
 
   EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  EXPECT_FALSE(
+      write_callback_observer_.GetAndResetObservationState().was_called);
   ASSERT_TRUE(PathExists(writer.path()));
   EXPECT_EQ("foo", GetFileContent(writer.path()));
 }
 
-TEST_F(ImportantFileWriterTest, BasicWithSuccessfulWriteObserver) {
+TEST_F(ImportantFileWriterTest, BasicWithWriteObservers) {
   ImportantFileWriter writer(file_, ThreadTaskRunnerHandle::Get());
   EXPECT_FALSE(PathExists(writer.path()));
   EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  EXPECT_FALSE(
+      write_callback_observer_.GetAndResetObservationState().was_called);
   successful_write_observer_.ObserveNextSuccessfulWrite(&writer);
+  write_callback_observer_.ObserveNextWriteCallback(&writer);
   writer.WriteNow(WrapUnique(new std::string("foo")));
   RunLoop().RunUntilIdle();
 
-  // Confirm that the observer is invoked.
+  // Confirm that the observers are invoked.
   EXPECT_TRUE(successful_write_observer_.GetAndResetObservationState());
+  SynchronousWriteCallbackObserver::WriteCallbackObservationState cb_state =
+      write_callback_observer_.GetAndResetObservationState();
+  EXPECT_TRUE(cb_state.was_called);
+  EXPECT_TRUE(cb_state.success_param_value);
   ASSERT_TRUE(PathExists(writer.path()));
   EXPECT_EQ("foo", GetFileContent(writer.path()));
 
-  // Confirm that re-installing the observer works for another write.
+  // Confirm that re-installing the observers works for another write.
   EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  EXPECT_FALSE(
+      write_callback_observer_.GetAndResetObservationState().was_called);
   successful_write_observer_.ObserveNextSuccessfulWrite(&writer);
+  write_callback_observer_.ObserveNextWriteCallback(&writer);
   writer.WriteNow(WrapUnique(new std::string("bar")));
   RunLoop().RunUntilIdle();
 
   EXPECT_TRUE(successful_write_observer_.GetAndResetObservationState());
+  cb_state = write_callback_observer_.GetAndResetObservationState();

[gab, 2016/08/31 19:45:34]

The |write_callback_observer_| should have been called during WriteNow(), add a
verification step between WriteNow() and RunUntilIdle() to confirm that the
synchronous callback was invoked already but not the other one.

[proberge, 2016/08/31 21:06:44]

Not quite. WriteNow() uses PostTaskAndReply to WriteFileAtomically , so we still
need to step in the message loop for the |write_callback_observer_| to have been
called.

[gab, 2016/09/01 19:59:53]

I see, then the way to split this is to bind ImportantFileWriter to another
base::Thread. i.e., at the top of this test, initialize a base::Thread and pass
its task_runner() to ImportantFileWriter, then you can independently flush the
message loops.

Note that you can only RunUntilIdle() from a thread itself, one way to flush a
message loop without having to manually RunUntilIdle() on its thread is to do
this:

writer.WriteNow(WrapUnique(new std::string("bar")));

// Make sure tasks posted by WriteNow() have ran before
// continuing.
WaitableEvent done(...MANUAL, ...NOT_SIGNALED);
thread.task_runner().PostTask(&WaitableEvent::Signal, &done);
done.Wait();

// The |successful_write_observer_| should not yet have been
// notified yet but the |write_callback_observer_| should have been.
EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
EXPECT_EQ(CALLED_WITH_SUCCESS, ...

// There should be a pending task to notify |successful_write_observer_|
// in this message loop.
RunLoop().RunUntilIdle();

EXPECT_TRUE(...

[proberge, 2016/09/07 19:07:15]

Done.

+  EXPECT_TRUE(cb_state.was_called);
+  EXPECT_TRUE(cb_state.success_param_value);
   ASSERT_TRUE(PathExists(writer.path()));
   EXPECT_EQ("bar", GetFileContent(writer.path()));
 
-  // Confirm that writing again without re-installing the observer doesn't
+  // Confirm that writing again without re-installing the observers doesn't
   // result in a notification.
   EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  EXPECT_FALSE(
+      write_callback_observer_.GetAndResetObservationState().was_called);
   writer.WriteNow(WrapUnique(new std::string("baz")));
   RunLoop().RunUntilIdle();
 
   EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  EXPECT_FALSE(
+      write_callback_observer_.GetAndResetObservationState().was_called);
   ASSERT_TRUE(PathExists(writer.path()));
   EXPECT_EQ("baz", GetFileContent(writer.path()));
 }
 
+TEST_F(ImportantFileWriterTest, FailedWriteWithWriteObservers) {
+  // Use an invalid file path to get a FILE_ERROR_ACCESS_DENIED error when
+  // trying to write the file.
+  ImportantFileWriter writer(FilePath().AppendASCII("bad/../path"),

[gab, 2016/08/31 19:45:34]

The ".." here merely makes this more likely to not be an invalid path doesn't it
(i.e. only path/ needs to exist)

[proberge, 2016/08/31 21:06:44]

This is actually a hack to make WriteFileAtomically fail. The File constructor
fails if |path| contains path traversal ('..') components.
See
https://cs.chromium.org/chromium/src/base/files/file.h?dr=Ss&sq=package:chrom....

WriteScopedStringToFileAtomically and WriteFileAtomically are both static, which
makes mocking/stubbing difficult. We don't want to mock WriteNow, as we're
testing its logic.

File has a constructor which takes an error code, but then we would need
WriteNow to take in a File instead of a FilePath and to pass that file to a
different thread with PostTask, which seems sketchy.

My initial attempt (patch set 1) created the file and locked it before calling
WriteNow(), which works only on Windows since Lock is 'advisory only' on POSIX. 
See
https://cs.chromium.org/chromium/src/base/files/file.h?dr=Ss&sq=package:chrom...

What do you think is less bad?
1. Keep "bad/../path" and hope no one tries to change the File ctor to accept
path traversal components.
2. Use Lock() and enable the test only on Windows
3. Add a static variable which, when set to true, causes WriteFileAtomically to
bail with a failure.

[gab, 2016/09/01 19:59:53]

Ah ok I see, then (1.) is best IMO, but clarify the comment above to make it
explicit that the relative path is what's inferring the error.

[proberge, 2016/09/07 19:07:15]

Done.

+                             ThreadTaskRunnerHandle::Get());
+  EXPECT_FALSE(PathExists(writer.path()));
+  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  EXPECT_FALSE(
+      write_callback_observer_.GetAndResetObservationState().was_called);
+  successful_write_observer_.ObserveNextSuccessfulWrite(&writer);
+  write_callback_observer_.ObserveNextWriteCallback(&writer);
+  writer.WriteNow(WrapUnique(new std::string("foo")));
+  RunLoop().RunUntilIdle();
+
+  // Confirm that the successful write observer was not invoked, and that the
+  // write observer was invoked with its boolean parameter set to false.
+  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  SynchronousWriteCallbackObserver::WriteCallbackObservationState cb_state =
+      write_callback_observer_.GetAndResetObservationState();
+  EXPECT_TRUE(cb_state.was_called);
+  EXPECT_FALSE(cb_state.success_param_value);
+  EXPECT_FALSE(PathExists(writer.path()));
+}
+
 TEST_F(ImportantFileWriterTest, ScheduleWrite) {
   ImportantFileWriter writer(file_,
                              ThreadTaskRunnerHandle::Get(),