mojo filesystem: Simplify full file reading/writing.

[This is the same patch as before, but this fixes the win x64 gn bot
which is a main waterfall tree closer, but doesn't have a default trybot
run.]

One common pattern that's coming up multiple times is that we want to
read or write the full contents of a file. The first attempt at an
interface to that had was put on the File object.

However, file seeking behaviour differs between platforms. Performing a
seek on an empty file is safe on posix and errors on Windows. And when
dealing with arbitrary File objects, we want to seek to the beginning
just in case there was any previous usage on the File object.

It was also cumbersome. The user was still responsible for opening the
file and closing it once they were done with it. Putting these
operations on Directory not only removes a bug, but also simplifies the
interface.

BUG=557405
First Review URL: https://codereview.chromium.org/1634293002
TBR=sky@chromium.org

Review URL: https://codereview.chromium.org/1646673002

Cr-Commit-Position: refs/heads/master@{#371953}

components/filesystem/directory_impl.cc

@@ -15,6 +15,7 @@
 #include "build/build_config.h"
 #include "components/filesystem/file_impl.h"
 #include "components/filesystem/util.h"
+#include "mojo/common/common_type_converters.h"
 
 namespace filesystem {
 
@@ -64,7 +65,7 @@ void DirectoryImpl::OpenFile(const mojo::String& raw_path,
 
 #if defined(OS_WIN)
   // On Windows, FILE_FLAG_BACKUP_SEMANTICS is needed to open a directory.
-  if (DirectoryExists(path))
+  if (base::DirectoryExists(path))
     open_flags |= base::File::FLAG_BACKUP_SEMANTICS;
 #endif  // OS_WIN
 
@@ -215,4 +216,69 @@ void DirectoryImpl::Flush(const FlushCallback& callback) {
   callback.Run(FileError::OK);
 }
 
+void DirectoryImpl::ReadEntireFile(const mojo::String& raw_path,
+                                   const ReadEntireFileCallback& callback) {
+  base::FilePath path;
+  FileError error = ValidatePath(raw_path, directory_path_, &path);
+  if (error != FileError::OK) {
+    callback.Run(error, mojo::Array<uint8_t>(0));
+    return;
+  }
+
+  if (base::DirectoryExists(path)) {
+    callback.Run(FileError::NOT_A_FILE, mojo::Array<uint8_t>(0));
+    return;
+  }
+
+  base::File base_file(path, base::File::FLAG_OPEN | base::File::FLAG_READ);
+  if (!base_file.IsValid()) {
+    callback.Run(GetError(base_file), mojo::Array<uint8_t>(0));
+    return;
+  }
+
+  std::string contents;
+  const int kBufferSize = 1 << 16;
+  scoped_ptr<char[]> buf(new char[kBufferSize]);
+  int len;
+  while ((len = base_file.ReadAtCurrentPos(buf.get(), kBufferSize)) > 0)
+    contents.append(buf.get(), len);
+
+  callback.Run(FileError::OK, mojo::Array<uint8_t>::From(contents));
+}
+
+void DirectoryImpl::WriteFile(const mojo::String& raw_path,
+                              mojo::Array<uint8_t> data,
+                              const WriteFileCallback& callback) {
+  base::FilePath path;
+  FileError error = ValidatePath(raw_path, directory_path_, &path);
+  if (error != FileError::OK) {
+    callback.Run(error);
+    return;
+  }
+
+  if (base::DirectoryExists(path)) {
+    callback.Run(FileError::NOT_A_FILE);
+    return;
+  }
+
+  base::File base_file(path,
+                       base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE);
+  if (!base_file.IsValid()) {
+    callback.Run(GetError(base_file));
+    return;
+  }
+
+  // If we're given empty data, we don't write and just truncate the file.
+  if (data.size()) {
+    const int data_size = static_cast<int>(data.size());
+    if (base_file.Write(0, reinterpret_cast<char*>(&data.front()),
+                        data_size) == -1) {
+      callback.Run(GetError(base_file));
+      return;
+    }
+  }
+
+  callback.Run(FileError::OK);
+}
+
 }  // namespace filesystem

components/filesystem/directory_impl.h

@@ -55,6 +55,11 @@ class DirectoryImpl : public Directory {
   void IsWritable(const mojo::String& path,
                   const IsWritableCallback& callback) override;
   void Flush(const FlushCallback& callback) override;
+  void ReadEntireFile(const mojo::String& path,
+                      const ReadEntireFileCallback& callback) override;
+  void WriteFile(const mojo::String& path,
+                 mojo::Array<uint8_t> data,
+                 const WriteFileCallback& callback) override;
 
  private:
   mojo::StrongBinding<Directory> binding_;

components/filesystem/directory_impl_unittest.cc

@@ -10,6 +10,7 @@
 
 #include "base/macros.h"
 #include "components/filesystem/files_test_base.h"
+#include "mojo/common/common_type_converters.h"
 #include "mojo/util/capture_util.h"
 
 using mojo::Capture;
@@ -153,6 +154,93 @@ TEST_F(DirectoryImplTest, CantOpenDirectoriesAsFiles) {
   }
 }
 
+TEST_F(DirectoryImplTest, WriteFileReadFile) {
+  DirectoryPtr directory;
+  GetTemporaryRoot(&directory);
+  FileError error;
+
+  std::string data("one two three");
+  {
+    directory->WriteFile("data", mojo::Array<uint8_t>::From(data),
+                         Capture(&error));
+    ASSERT_TRUE(directory.WaitForIncomingResponse());
+    EXPECT_EQ(FileError::OK, error);
+  }
+
+  {
+    mojo::Array<uint8_t> file_contents;
+    directory->ReadEntireFile("data", Capture(&error, &file_contents));
+    ASSERT_TRUE(directory.WaitForIncomingResponse());
+    EXPECT_EQ(FileError::OK, error);
+
+    EXPECT_EQ(data, file_contents.To<std::string>());
+  }
+}
+
+TEST_F(DirectoryImplTest, ReadEmptyFileIsNotFoundError) {
+  DirectoryPtr directory;
+  GetTemporaryRoot(&directory);
+  FileError error;
+
+  {
+    mojo::Array<uint8_t> file_contents;
+    directory->ReadEntireFile("doesnt_exist", Capture(&error, &file_contents));
+    ASSERT_TRUE(directory.WaitForIncomingResponse());
+    EXPECT_EQ(FileError::NOT_FOUND, error);
+  }
+}
+
+TEST_F(DirectoryImplTest, CantReadEntireFileOnADirectory) {
+  DirectoryPtr directory;
+  GetTemporaryRoot(&directory);
+  FileError error;
+
+  // Create a directory
+  {
+    DirectoryPtr my_file_directory;
+    error = FileError::FAILED;
+    directory->OpenDirectory(
+        "my_dir", GetProxy(&my_file_directory),
+        kFlagRead | kFlagWrite | kFlagCreate,
+        Capture(&error));
+    ASSERT_TRUE(directory.WaitForIncomingResponse());
+    EXPECT_EQ(FileError::OK, error);
+  }
+
+  // Try to read it as a file
+  {
+    mojo::Array<uint8_t> file_contents;
+    directory->ReadEntireFile("my_dir", Capture(&error, &file_contents));
+    ASSERT_TRUE(directory.WaitForIncomingResponse());
+    EXPECT_EQ(FileError::NOT_A_FILE, error);
+  }
+}
+
+TEST_F(DirectoryImplTest, CantWriteFileOnADirectory) {
+  DirectoryPtr directory;
+  GetTemporaryRoot(&directory);
+  FileError error;
+
+  // Create a directory
+  {
+    DirectoryPtr my_file_directory;
+    error = FileError::FAILED;
+    directory->OpenDirectory(
+        "my_dir", GetProxy(&my_file_directory),
+        kFlagRead | kFlagWrite | kFlagCreate,
+        Capture(&error));
+    ASSERT_TRUE(directory.WaitForIncomingResponse());
+    EXPECT_EQ(FileError::OK, error);
+  }
+
+  {
+    std::string data("one two three");
+    directory->WriteFile("my_dir", mojo::Array<uint8_t>::From(data),
+                         Capture(&error));
+    ASSERT_TRUE(directory.WaitForIncomingResponse());
+    EXPECT_EQ(FileError::NOT_A_FILE, error);
+  }
+}
 
 // TODO(vtl): Test delete flags.
 

components/filesystem/file_impl.cc

@@ -94,28 +94,6 @@ void FileImpl::Read(uint32_t num_bytes_to_read,
   callback.Run(FileError::OK, std::move(bytes_read));
 }
 
-void FileImpl::ReadEntireFile(const ReadEntireFileCallback& callback) {
-  if (!file_.IsValid()) {
-    callback.Run(GetError(file_), mojo::Array<uint8_t>());
-    return;
-  }
-
-  // Seek to the front of the file.
-  if (file_.Seek(base::File::FROM_BEGIN, 0) == -1) {
-    callback.Run(FileError::FAILED, mojo::Array<uint8_t>());
-    return;
-  }
-
-  std::string contents;
-  const int kBufferSize = 1 << 16;
-  scoped_ptr<char[]> buf(new char[kBufferSize]);
-  size_t len;
-  while ((len = file_.ReadAtCurrentPos(buf.get(), kBufferSize)) > 0)
-    contents.append(buf.get(), len);
-
-  callback.Run(FileError::OK, mojo::Array<uint8_t>::From(contents));
-}
-
 // TODO(vtl): Move the implementation to a thread pool.
 void FileImpl::Write(mojo::Array<uint8_t> bytes_to_write,
                      int64_t offset,

components/filesystem/file_impl.h

@@ -34,7 +34,6 @@ class FileImpl : public File {
             int64_t offset,
             Whence whence,
             const ReadCallback& callback) override;
-  void ReadEntireFile(const ReadEntireFileCallback& callback) override;
   void Write(mojo::Array<uint8_t> bytes_to_write,
              int64_t offset,
              Whence whence,

components/filesystem/public/cpp/prefs/filesystem_json_pref_store.cc

@@ -348,17 +348,6 @@ void FilesystemJsonPrefStore::OnOpenFilesystem(base::Closure callback,
 }
 
 void FilesystemJsonPrefStore::OnTempFileWriteStart() {
-  // Open up a temporary file and truncate it.
-  directory_->OpenFile(
-      "tmp", GetProxy(&temporary_file_), kFlagWrite | kFlagCreate,
-      Bind(&FilesystemJsonPrefStore::OnTempFileOpened, AsWeakPtr()));
-}
-
-void FilesystemJsonPrefStore::OnTempFileOpened(FileError err) {
-  // TODO(erg): Error handling. The JsonPrefStore code assumes that writing the
-  // file can never fail.
-  CHECK_EQ(FileError::OK, err);
-
   // Calculate what we want to write, and then write to the temporary file.
   pending_lossy_write_ = false;
 
@@ -370,50 +359,29 @@ void FilesystemJsonPrefStore::OnTempFileOpened(FileError err) {
   serializer.set_pretty_print(false);
   serializer.Serialize(*prefs_);
 
-  temporary_file_->Write(
-      mojo::Array<uint8_t>::From(output), 0, Whence::FROM_CURRENT,
+  directory_->WriteFile(
+      "tmp",
+      mojo::Array<uint8_t>::From(output),
       Bind(&FilesystemJsonPrefStore::OnTempFileWrite, AsWeakPtr()));
 }
 
-void FilesystemJsonPrefStore::OnTempFileWrite(FileError err,
-                                              uint32_t num_bytes_written) {
-  // TODO(erg): Error handling. The JsonPrefStore code assumes that writing the
-  // file can never fail.
-  CHECK_EQ(FileError::OK, err);
-
-  // Now that we've written the file, close it.
-  temporary_file_->Close(
-      Bind(&FilesystemJsonPrefStore::OnTempFileClosed, AsWeakPtr()));
-}
-
-void FilesystemJsonPrefStore::OnTempFileClosed(FileError err) {
+void FilesystemJsonPrefStore::OnTempFileWrite(FileError err) {
   // TODO(erg): Error handling. The JsonPrefStore code assumes that writing the
   // file can never fail.
   CHECK_EQ(FileError::OK, err);
 
-  temporary_file_.reset();
   directory_->Rename(
       "tmp", path_,
       Bind(&FilesystemJsonPrefStore::OnTempFileRenamed, AsWeakPtr()));
 }
 
-void FilesystemJsonPrefStore::OnTempFileRenamed(FileError err) {}
-
-void FilesystemJsonPrefStore::OnPreferencesReadStart() {
-  // TODO(erg): implement me.
-  directory_->OpenFile(
-      path_, GetProxy(&preferences_file_), kFlagRead | kFlagOpen,
-      Bind(&FilesystemJsonPrefStore::OnPreferencesFileOpened, AsWeakPtr()));
+void FilesystemJsonPrefStore::OnTempFileRenamed(FileError err) {
 }
 
-void FilesystemJsonPrefStore::OnPreferencesFileOpened(FileError err) {
-  // TODO(erg): Error handling.
-  if (err == FileError::OK) {
-    preferences_file_->ReadEntireFile(
-        Bind(&FilesystemJsonPrefStore::OnPreferencesFileRead, AsWeakPtr()));
-  } else {
-    OnPreferencesFileRead(err, mojo::Array<uint8_t>());
-  }
+void FilesystemJsonPrefStore::OnPreferencesReadStart() {
+  directory_->ReadEntireFile(
+      path_,
+      Bind(&FilesystemJsonPrefStore::OnPreferencesFileRead, AsWeakPtr()));
 }
 
 void FilesystemJsonPrefStore::OnPreferencesFileRead(
@@ -424,7 +392,8 @@ void FilesystemJsonPrefStore::OnPreferencesFileRead(
   // TODO(erg): Needs even better error handling.
   switch (err) {
     case FileError::IN_USE:
-    case FileError::ACCESS_DENIED: {
+    case FileError::ACCESS_DENIED:
+    case FileError::NOT_A_FILE: {
       read_only_ = true;
       break;
     }
@@ -445,8 +414,6 @@ void FilesystemJsonPrefStore::OnPreferencesFileRead(
     }
   }
 
-  preferences_file_.reset();
-
   OnFileRead(std::move(read_result));
 }
 

components/filesystem/public/cpp/prefs/filesystem_json_pref_store.h

@@ -140,14 +140,11 @@ class FilesystemJsonPrefStore
 
   // Asynchronous implementation details of PerformWrite().
   void OnTempFileWriteStart();
-  void OnTempFileOpened(FileError err);
-  void OnTempFileWrite(FileError err, uint32_t num_bytes_written);
-  void OnTempFileClosed(FileError err);
+  void OnTempFileWrite(FileError err);
   void OnTempFileRenamed(FileError err);
 
   // Asynchronous implementation details of ReadPrefsAsync().
   void OnPreferencesReadStart();
-  void OnPreferencesFileOpened(FileError err);
   void OnPreferencesFileRead(FileError err, mojo::Array<uint8_t> contents);
 
   const std::string path_;
@@ -158,9 +155,6 @@ class FilesystemJsonPrefStore
   // |filesystem. See OpenFilesystem().
   DirectoryPtr directory_;
 
-  FilePtr preferences_file_;
-  FilePtr temporary_file_;
-
   scoped_ptr<base::DictionaryValue> prefs_;
 
   bool read_only_;

components/filesystem/public/interfaces/directory.mojom

@@ -52,6 +52,12 @@ interface Directory {
   // fsync()/FlushFileBuffers().
   Flush() => (FileError error);
 
+  // Reads the contents of an entire file.
+  ReadEntireFile(string path) => (FileError error, array<uint8> data);
+
+  // Writes |data| to |path|, overwriting the file if it already exists.
+  WriteFile(string path, array<uint8> data) => (FileError error);
+
   // TODO(vtl): directory "streaming"?
   // TODO(vtl): "make root" (i.e., prevent cd-ing, etc., to parent); note that
   // this would require a much more complicated implementation (e.g., it needs

components/filesystem/public/interfaces/file.mojom

@@ -27,10 +27,6 @@ interface File {
   Read(uint32 num_bytes_to_read, int64 offset, Whence whence)
       => (FileError error, array<uint8>? bytes_read);
 
-  // Returns the entire contents of the file as an array of bytes. The mojo
-  // equivalent of base::ReadFileToString().
-  ReadEntireFile() => (FileError error, array<uint8>? bytes_read);
-
   // Writes |bytes_to_write| to the location specified by |offset|/|whence|.
   // TODO(vtl): Clarify behavior when |num_bytes_written| is less than the size
   // of |bytes_to_write|.