Add an async flag to mojo_base.mojo.File

Currently the async flag in base::File isn't serialized when passing
Files between processes. There are assertions and checks on the value
of this flag in several places, so it needs to be preserved during
serialization/deserialization. On Windows we can't check whether an
existing file handle is async or not, so we have to explicitly pass
the flag around.

Bug: 845612
Change-Id: I8847ecd9451c13f7419e96db465caa417f4c543d
Reviewed-on: https://chromium-review.googlesource.com/1096480
Commit-Queue: Robbie McElrath <rmcelrath@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568662}

base/files/file.cc

@@ -36,11 +36,13 @@ File::File(const FilePath& path, uint32_t flags)
 }
 #endif
 
-File::File(PlatformFile platform_file)
+File::File(PlatformFile platform_file) : File(platform_file, false) {}
+
+File::File(PlatformFile platform_file, bool async)
     : file_(platform_file),
       error_details_(FILE_OK),
       created_(false),
-      async_(false) {
+      async_(async) {
 #if defined(OS_POSIX) || defined(OS_FUCHSIA)
   DCHECK_GE(platform_file, -1);
 #endif
@@ -64,15 +66,6 @@ File::~File() {
   Close();
 }
 
-// static
-File File::CreateForAsyncHandle(PlatformFile platform_file) {
-  File file(platform_file);
-  // It would be nice if we could validate that |platform_file| was opened with
-  // FILE_FLAG_OVERLAPPED on Windows but this doesn't appear to be possible.
-  file.async_ = true;
-  return file;
-}
-
 File& File::operator=(File&& other) {
   Close();
   SetPlatformFile(other.TakePlatformFile());

base/files/file.h

@@ -153,9 +153,14 @@ class BASE_EXPORT File {
   // |path| contains path traversal ('..') components.
   File(const FilePath& path, uint32_t flags);
 
-  // Takes ownership of |platform_file|.
+  // Takes ownership of |platform_file| and sets async to false.
   explicit File(PlatformFile platform_file);
 
+  // Takes ownership of |platform_file| and sets async to the given value.
+  // This constructor exists because on Windows you can't check if platform_file
+  // is async or not.
+  File(PlatformFile platform_file, bool async);
+
   // Creates an object with a specific error_details code.
   explicit File(Error error_details);
 
@@ -163,9 +168,6 @@ class BASE_EXPORT File {
 
   ~File();
 
-  // Takes ownership of |platform_file|.
-  static File CreateForAsyncHandle(PlatformFile platform_file);
-
   File& operator=(File&& other);
 
   // Creates or opens the given file.

base/files/file_posix.cc

@@ -395,10 +395,7 @@ File File::Duplicate() const {
   if (other_fd == -1)
     return File(File::GetLastFileError());
 
-  File other(other_fd);
-  if (async())
-    other.async_ = true;
-  return other;
+  return File(other_fd, async());
 }
 
 // Static.

base/files/file_unittest.cc

@@ -745,4 +745,18 @@ TEST(FileTest, NoDeleteOnCloseWithMappedFile) {
   file.Close();
   ASSERT_TRUE(base::PathExists(file_path));
 }
+
+// Check that we handle the async bit being set incorrectly in a sane way.
+TEST(FileTest, UseSyncApiWithAsyncFile) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  FilePath file_path = temp_dir.GetPath().AppendASCII("file");
+
+  File file(file_path, base::File::FLAG_CREATE | base::File::FLAG_WRITE |
+                           base::File::FLAG_ASYNC);
+  File lying_file(file.TakePlatformFile(), false /* async */);
+  ASSERT_TRUE(lying_file.IsValid());
+
+  ASSERT_EQ(lying_file.WriteAtCurrentPos("12345", 5), -1);
+}
 #endif  // defined(OS_WIN)

base/files/file_win.cc

@@ -269,10 +269,7 @@ File File::Duplicate() const {
     return File(GetLastFileError());
   }
 
-  File other(other_handle);
-  if (async())
-    other.async_ = true;
-  return other;
+  return File(other_handle, async());
 }
 
 bool File::DeleteOnClose(bool delete_on_close) {

chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc

@@ -215,15 +215,17 @@ TEST_F(NativeMessagingTest, SingleSendMessageWrite) {
 #if defined(OS_WIN)
   base::string16 pipe_name = base::StringPrintf(
       L"\\\\.\\pipe\\chrome.nativeMessaging.out.%llx", base::RandUint64());
-  base::File write_handle = base::File::CreateForAsyncHandle(
-      CreateNamedPipeW(pipe_name.c_str(),
-                       PIPE_ACCESS_OUTBOUND | FILE_FLAG_OVERLAPPED |
-                           FILE_FLAG_FIRST_PIPE_INSTANCE,
-                       PIPE_TYPE_BYTE, 1, 0, 0, 5000, NULL));
+  base::File write_handle =
+      base::File(CreateNamedPipeW(pipe_name.c_str(),
+                                  PIPE_ACCESS_OUTBOUND | FILE_FLAG_OVERLAPPED |
+                                      FILE_FLAG_FIRST_PIPE_INSTANCE,
+                                  PIPE_TYPE_BYTE, 1, 0, 0, 5000, NULL),
+                 true /* async */);
   ASSERT_TRUE(write_handle.IsValid());
-  base::File read_handle = base::File::CreateForAsyncHandle(
+  base::File read_handle = base::File(
       CreateFileW(pipe_name.c_str(), GENERIC_READ, 0, NULL, OPEN_EXISTING,
-                  FILE_ATTRIBUTE_NORMAL | FILE_FLAG_OVERLAPPED, NULL));
+                  FILE_ATTRIBUTE_NORMAL | FILE_FLAG_OVERLAPPED, NULL),
+      true /* async */);
   ASSERT_TRUE(read_handle.IsValid());
 
   read_file = std::move(read_handle);

chrome/browser/extensions/api/messaging/native_process_launcher_win.cc

@@ -171,8 +171,8 @@ bool NativeProcessLauncher::LaunchNativeProcess(
   }
 
   *process = std::move(cmd_process);
-  *read_file = base::File::CreateForAsyncHandle(stdout_pipe.Take());
-  *write_file = base::File::CreateForAsyncHandle(stdin_pipe.Take());
+  *read_file = base::File(stdout_pipe.Take(), true /* async */);
+  *write_file = base::File(stdin_pipe.Take(), true /* async */);
 
   return true;
 }

mojo/public/cpp/base/file_mojom_traits.cc

@@ -23,7 +23,7 @@ bool StructTraits<mojo_base::mojom::FileDataView, base::File>::Read(
       MOJO_RESULT_OK) {
     return false;
   }
-  *file = base::File(platform_handle);
+  *file = base::File(platform_handle, data.async());
   return true;
 }
 

mojo/public/cpp/base/file_mojom_traits.h

@@ -19,6 +19,7 @@ struct COMPONENT_EXPORT(MOJO_BASE_MOJOM)
   static void SetToNull(base::File* file) { *file = base::File(); }
 
   static mojo::ScopedHandle fd(base::File& file);
+  static bool async(base::File& file) { return file.async(); }
   static bool Read(mojo_base::mojom::FileDataView data, base::File* file);
 };
 

mojo/public/cpp/base/file_unittest.cc

@@ -28,6 +28,7 @@ TEST(FileTest, File) {
       mojo::test::SerializeAndDeserialize<mojom::File>(&file, &file_out));
   std::vector<char> content(test_content.size());
   ASSERT_TRUE(file_out.IsValid());
+  ASSERT_FALSE(file_out.async());
   ASSERT_EQ(static_cast<int>(test_content.size()),
             file_out.Read(0, content.data(),
                           base::checked_cast<int>(test_content.size())));
@@ -35,6 +36,25 @@ TEST(FileTest, File) {
             base::StringPiece(content.data(), test_content.size()));
 }
 
+TEST(FileTest, AsyncFile) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  base::FilePath path = temp_dir.GetPath().AppendASCII("async_test_file.txt");
+
+  base::File write_file(path, base::File::FLAG_CREATE | base::File::FLAG_WRITE);
+  const base::StringPiece test_content = "test string";
+  write_file.WriteAtCurrentPos(test_content.data(),
+                               base::checked_cast<int>(test_content.size()));
+  write_file.Close();
+
+  base::File file(path, base::File::FLAG_OPEN | base::File::FLAG_READ |
+                            base::File::FLAG_ASYNC);
+  base::File file_out;
+  ASSERT_TRUE(
+      mojo::test::SerializeAndDeserialize<mojom::File>(&file, &file_out));
+  ASSERT_TRUE(file_out.async());
+}
+
 TEST(FileTest, InvalidFile) {
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());

mojo/public/mojom/base/file.mojom

@@ -7,4 +7,5 @@ module mojo_base.mojom;
 // Corresponds to |base::File| in base/files/file.h
 struct File {
   handle fd;
+  bool async;
 };

net/base/file_stream_unittest.cc

@@ -814,8 +814,7 @@ TEST_F(FileStreamTest, AsyncFlagMismatch) {
   // handle but with the async flag set to true.
   uint32_t flags = base::File::FLAG_OPEN | base::File::FLAG_READ;
   base::File file(temp_file_path(), flags);
-  base::File lying_file =
-      base::File::CreateForAsyncHandle(file.TakePlatformFile());
+  base::File lying_file(file.TakePlatformFile(), true);
   ASSERT_TRUE(lying_file.IsValid());
 
   FileStream stream(std::move(lying_file), base::ThreadTaskRunnerHandle::Get());