commit d9f706c881e376e32feb308b0f9f8ec74051b812
author Robbie McElrath <rmcelrath@chromium.org> Wed Jun 20 00:24:46 2018
committer Commit Bot <commit-bot@chromium.org> Wed Jun 20 00:24:46 2018
tree b1e5daaa543d9241d6060188209cce6691dc2b91
parent a243c7d260e62dcd289872dd86c0d073d229628d

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-Original-Commit-Position: refs/heads/master@{#568662}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: d798079d315b6acedc797101a6c808ff826d32a0

files/file.cc

diff --git a/files/file.cc b/files/file.cc
index 1a4ee37..e8934b1 100644
--- a/files/file.cc
+++ b/files/file.cc
@@ -36,11 +36,13 @@
 }
 #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 @@
   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());

files/file.h

diff --git a/files/file.h b/files/file.h
index c3a31d8..ca99b13 100644
--- a/files/file.h
+++ b/files/file.h
@@ -153,9 +153,14 @@
   // |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 @@
 
   ~File();
 
-  // Takes ownership of |platform_file|.
-  static File CreateForAsyncHandle(PlatformFile platform_file);
-
   File& operator=(File&& other);
 
   // Creates or opens the given file.

files/file_posix.cc

diff --git a/files/file_posix.cc b/files/file_posix.cc
index 45cef58..fffec5e 100644
--- a/files/file_posix.cc
+++ b/files/file_posix.cc
@@ -395,10 +395,7 @@
   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.

files/file_unittest.cc

diff --git a/files/file_unittest.cc b/files/file_unittest.cc
index 65bf62d..8a57322 100644
--- a/files/file_unittest.cc
+++ b/files/file_unittest.cc
@@ -745,4 +745,18 @@
   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)

files/file_win.cc

diff --git a/files/file_win.cc b/files/file_win.cc
index d7bffc3..82cd7e8 100644
--- a/files/file_win.cc
+++ b/files/file_win.cc
@@ -269,10 +269,7 @@
     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) {