Convert base::MemoryMappedFile to use File instead of PlatformFile.

This also modifies consumers of MemoryMappedFile and fixes double handle
close on MediaFileChecker, media_file_checker_unittest and data_pack_unittests.

BUG=322664
TEST=unit tests

R=cpu@chromium.org, dalecurtis@chromium.org (media)

TBR (owners):
tony@chromium.org (resource)
jochen@chromium.org (chrome-content)
thakis@chromium.org (base)

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@242937 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 123 insertions, 110 deletions

diff --git a/base/files/file.cc b/base/files/file.cc
index 4902f15..508103a 100644
--- a/base/files/file.cc
+++ b/base/files/file.cc
@@ -31,14 +31,17 @@ File::File(const FilePath& name, uint32 flags)
       error_(FILE_OK),
       created_(false),
       async_(false) {
-  if (name.ReferencesParent()) {
-    error_ = FILE_ERROR_ACCESS_DENIED;
-    return;
-  }
-  CreateBaseFileUnsafe(name, flags);
+  Initialize(name, flags);
 }
 #endif
 
+File::File(PlatformFile platform_file)
+    : file_(platform_file),
+      error_(FILE_OK),
+      created_(false),
+      async_(false) {
+}
+
 File::File(RValue other)
     : file_(other.object->TakePlatformFile()),
       error_(other.object->error()),
@@ -61,4 +64,14 @@ File& File::operator=(RValue other) {
   return *this;
 }
 
+#if !defined(OS_NACL)
+void File::Initialize(const FilePath& name, uint32 flags) {
+  if (name.ReferencesParent()) {
+    error_ = FILE_ERROR_ACCESS_DENIED;
+    return;
+  }
+  InitializeUnsafe(name, flags);
+}
+#endif
+
 }  // namespace base
diff --git a/base/files/file.h b/base/files/file.h
index d1e0e8c..e87dd7b 100644
--- a/base/files/file.h
+++ b/base/files/file.h
@@ -157,9 +157,12 @@ class BASE_EXPORT File {
   // Move operator= for C++03 move emulation of this type.
   File& operator=(RValue other);
 
+  // Creates or opens the given file.
+  void Initialize(const FilePath& name, uint32 flags);
+
   // Creates or opens the given file, allowing paths with traversal ('..')
   // components. Use only with extreme care.
-  void CreateBaseFileUnsafe(const FilePath& name, uint32 flags);
+  void InitializeUnsafe(const FilePath& name, uint32 flags);
 
   bool IsValid() const;
 
@@ -216,10 +219,13 @@ class BASE_EXPORT File {
   // platforms. Returns the number of bytes written, or -1 on error.
   int WriteAtCurrentPosNoBestEffort(const char* data, int size);
 
+  // Returns the current size of this file, or a negative number on failure.
+  int64 GetLength();
+
   // Truncates the file to the given length. If |length| is greater than the
   // current size of the file, the file is extended with zeros. If the file
   // doesn't exist, |false| is returned.
-  bool Truncate(int64 length);
+  bool SetLength(int64 length);
 
   // Flushes the buffers.
   bool Flush();
diff --git a/base/files/file_posix.cc b/base/files/file_posix.cc
index 9d97c33..e8b4d77 100644
--- a/base/files/file_posix.cc
+++ b/base/files/file_posix.cc
@@ -122,7 +122,7 @@ static File::Error CallFctnlFlock(PlatformFile file, bool do_lock) {
 // NaCl doesn't implement system calls to open files directly.
 #if !defined(OS_NACL)
 // TODO(erikkay): does it make sense to support FLAG_EXCLUSIVE_* here?
-void File::CreateBaseFileUnsafe(const FilePath& name, uint32 flags) {
+void File::InitializeUnsafe(const FilePath& name, uint32 flags) {
   base::ThreadRestrictions::AssertIOAllowed();
   DCHECK(!IsValid());
   DCHECK(!(flags & FLAG_ASYNC));
@@ -341,7 +341,17 @@ int File::WriteAtCurrentPosNoBestEffort(const char* data, int size) {
   return HANDLE_EINTR(write(file_, data, size));
 }
 
-bool File::Truncate(int64 length) {
+int64 File::GetLength() {
+  DCHECK(IsValid());
+
+  stat_wrapper_t file_info;
+  if (CallFstat(file_, &file_info))
+    return false;
+
+  return file_info.st_size;
+}
+
+bool File::SetLength(int64 length) {
   base::ThreadRestrictions::AssertIOAllowed();
   DCHECK(IsValid());
   return !CallFtruncate(file_, length);
diff --git a/base/files/file_unittest.cc b/base/files/file_unittest.cc
index b2e855d..16eece1 100644
--- a/base/files/file_unittest.cc
+++ b/base/files/file_unittest.cc
@@ -44,6 +44,19 @@ TEST(File, Create) {
   }
 
   {
+    // Open an existing file through Initialize
+    File file;
+    file.Initialize(file_path, base::File::FLAG_OPEN | base::File::FLAG_READ);
+    EXPECT_TRUE(file.IsValid());
+    EXPECT_FALSE(file.created());
+    EXPECT_EQ(base::File::FILE_OK, file.error());
+
+    // This time verify closing the file.
+    file.Close();
+    EXPECT_FALSE(file.IsValid());
+  }
+
+  {
     // Create a file that exists.
     File file(file_path, base::File::FLAG_CREATE | base::File::FLAG_READ);
     EXPECT_FALSE(file.IsValid());
@@ -221,7 +234,7 @@ TEST(File, Append) {
 }
 
 
-TEST(File, Truncate) {
+TEST(File, Length) {
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
   FilePath file_path = temp_dir.path().AppendASCII("truncate_file");
@@ -229,6 +242,7 @@ TEST(File, Truncate) {
             base::File::FLAG_CREATE | base::File::FLAG_READ |
                 base::File::FLAG_WRITE);
   ASSERT_TRUE(file.IsValid());
+  EXPECT_EQ(0, file.GetLength());
 
   // Write "test" to the file.
   char data_to_write[] = "test";
@@ -239,7 +253,8 @@ TEST(File, Truncate) {
   // Extend the file.
   const int kExtendedFileLength = 10;
   int64 file_size = 0;
-  EXPECT_TRUE(file.Truncate(kExtendedFileLength));
+  EXPECT_TRUE(file.SetLength(kExtendedFileLength));
+  EXPECT_EQ(kExtendedFileLength, file.GetLength());
   EXPECT_TRUE(GetFileSize(file_path, &file_size));
   EXPECT_EQ(kExtendedFileLength, file_size);
 
@@ -254,7 +269,8 @@ TEST(File, Truncate) {
 
   // Truncate the file.
   const int kTruncatedFileLength = 2;
-  EXPECT_TRUE(file.Truncate(kTruncatedFileLength));
+  EXPECT_TRUE(file.SetLength(kTruncatedFileLength));
+  EXPECT_EQ(kTruncatedFileLength, file.GetLength());
   EXPECT_TRUE(GetFileSize(file_path, &file_size));
   EXPECT_EQ(kTruncatedFileLength, file_size);
 
diff --git a/base/files/file_win.cc b/base/files/file_win.cc
index 94f4d7f..c7ce909 100644
--- a/base/files/file_win.cc
+++ b/base/files/file_win.cc
@@ -13,7 +13,7 @@
 
 namespace base {
 
-void File::CreateBaseFileUnsafe(const FilePath& name, uint32 flags) {
+void File::InitializeUnsafe(const FilePath& name, uint32 flags) {
   base::ThreadRestrictions::AssertIOAllowed();
   DCHECK(!IsValid());
 
@@ -197,7 +197,17 @@ int File::WriteAtCurrentPosNoBestEffort(const char* data, int size) {
   return WriteAtCurrentPos(data, size);
 }
 
-bool File::Truncate(int64 length) {
+int64 File::GetLength() {
+  base::ThreadRestrictions::AssertIOAllowed();
+  DCHECK(IsValid());
+  LARGE_INTEGER size;
+  if (!::GetFileSizeEx(file_.Get(), &size))
+    return -1;
+
+  return static_cast<int64>(size.QuadPart);
+}
+
+bool File::SetLength(int64 length) {
   base::ThreadRestrictions::AssertIOAllowed();
   DCHECK(IsValid());
 
@@ -218,6 +228,9 @@ bool File::Truncate(int64 length) {
   // Set the new file length and move the file pointer to its old position.
   // This is consistent with ftruncate()'s behavior, even when the file
   // pointer points to a location beyond the end of the file.
+  // TODO(rvargas): Emulating ftruncate details seem suspicious and it is not
+  // promised by the interface (nor was promised by PlatformFile). See if this
+  // implementation detail can be removed.
   return ((::SetEndOfFile(file_) != 0) &&
           (::SetFilePointerEx(file_, file_pointer, NULL, FILE_BEGIN) != 0));
 }
diff --git a/base/files/memory_mapped_file.cc b/base/files/memory_mapped_file.cc
index a48ec0c..ace4e11 100644
--- a/base/files/memory_mapped_file.cc
+++ b/base/files/memory_mapped_file.cc
@@ -17,7 +17,14 @@ bool MemoryMappedFile::Initialize(const FilePath& file_name) {
   if (IsValid())
     return false;
 
-  if (!MapFileToMemory(file_name)) {
+  file_.Initialize(file_name, File::FLAG_OPEN | File::FLAG_READ);
+
+  if (!file_.IsValid()) {
+    DLOG(ERROR) << "Couldn't open " << file_name.AsUTF8Unsafe();
+    return false;
+  }
+
+  if (!MapFileToMemory()) {
     CloseHandles();
     return false;
   }
@@ -25,13 +32,13 @@ bool MemoryMappedFile::Initialize(const FilePath& file_name) {
   return true;
 }
 
-bool MemoryMappedFile::Initialize(PlatformFile file) {
+bool MemoryMappedFile::Initialize(File file) {
   if (IsValid())
     return false;
 
-  file_ = file;
+  file_ = file.Pass();
 
-  if (!MapFileToMemoryInternal()) {
+  if (!MapFileToMemory()) {
     CloseHandles();
     return false;
   }
@@ -43,16 +50,4 @@ bool MemoryMappedFile::IsValid() const {
   return data_ != NULL;
 }
 
-bool MemoryMappedFile::MapFileToMemory(const FilePath& file_name) {
-  file_ = CreatePlatformFile(file_name, PLATFORM_FILE_OPEN | PLATFORM_FILE_READ,
-                             NULL, NULL);
-
-  if (file_ == kInvalidPlatformFileValue) {
-    DLOG(ERROR) << "Couldn't open " << file_name.AsUTF8Unsafe();
-    return false;
-  }
-
-  return MapFileToMemoryInternal();
-}
-
 }  // namespace base
diff --git a/base/files/memory_mapped_file.h b/base/files/memory_mapped_file.h
index 6df1bad..b02d8cf 100644
--- a/base/files/memory_mapped_file.h
+++ b/base/files/memory_mapped_file.h
@@ -7,7 +7,7 @@
 
 #include "base/base_export.h"
 #include "base/basictypes.h"
-#include "base/platform_file.h"
+#include "base/files/file.h"
 #include "build/build_config.h"
 
 #if defined(OS_WIN)
@@ -30,9 +30,10 @@ class BASE_EXPORT MemoryMappedFile {
   // the file does not exist, or the memory mapping fails, it will return false.
   // Later we may want to allow the user to specify access.
   bool Initialize(const FilePath& file_name);
-  // As above, but works with an already-opened file. MemoryMappedFile will take
-  // ownership of |file| and close it when done.
-  bool Initialize(PlatformFile file);
+
+  // As above, but works with an already-opened file. MemoryMappedFile takes
+  // ownership of |file| and closes it when done.
+  bool Initialize(File file);
 
 #if defined(OS_WIN)
   // Opens an existing file and maps it as an image section. Please refer to
@@ -47,27 +48,22 @@ class BASE_EXPORT MemoryMappedFile {
   bool IsValid() const;
 
  private:
-  // Open the given file and pass it to MapFileToMemoryInternal().
-  bool MapFileToMemory(const FilePath& file_name);
-
   // Map the file to memory, set data_ to that memory address. Return true on
   // success, false on any kind of failure. This is a helper for Initialize().
-  bool MapFileToMemoryInternal();
+  bool MapFileToMemory();
 
-  // Closes all open handles. Later we may want to make this public.
+  // Closes all open handles.
   void CloseHandles();
 
-#if defined(OS_WIN)
-  // MapFileToMemoryInternal calls this function. It provides the ability to
-  // pass in flags which control the mapped section.
-  bool MapFileToMemoryInternalEx(int flags);
-
-  HANDLE file_mapping_;
-#endif
-  PlatformFile file_;
+  File file_;
   uint8* data_;
   size_t length_;
 
+#if defined(OS_WIN)
+  win::ScopedHandle file_mapping_;
+  bool image_;  // Map as an image.
+#endif
+
   DISALLOW_COPY_AND_ASSIGN(MemoryMappedFile);
 };
 
diff --git a/base/files/memory_mapped_file_posix.cc b/base/files/memory_mapped_file_posix.cc
index c4c477a..5d7e007 100644
--- a/base/files/memory_mapped_file_posix.cc
+++ b/base/files/memory_mapped_file_posix.cc
@@ -13,26 +13,23 @@
 
 namespace base {
 
-MemoryMappedFile::MemoryMappedFile()
-    : file_(kInvalidPlatformFileValue),
-      data_(NULL),
-      length_(0) {
+MemoryMappedFile::MemoryMappedFile() : data_(NULL), length_(0) {
 }
 
-bool MemoryMappedFile::MapFileToMemoryInternal() {
+bool MemoryMappedFile::MapFileToMemory() {
   ThreadRestrictions::AssertIOAllowed();
 
   struct stat file_stat;
-  if (fstat(file_, &file_stat) == kInvalidPlatformFileValue) {
-    DPLOG(ERROR) << "fstat " << file_;
+  if (fstat(file_.GetPlatformFile(), &file_stat) == -1 ) {
+    DPLOG(ERROR) << "fstat " << file_.GetPlatformFile();
     return false;
   }
   length_ = file_stat.st_size;
 
   data_ = static_cast<uint8*>(
-      mmap(NULL, length_, PROT_READ, MAP_SHARED, file_, 0));
+      mmap(NULL, length_, PROT_READ, MAP_SHARED, file_.GetPlatformFile(), 0));
   if (data_ == MAP_FAILED)
-    DPLOG(ERROR) << "mmap " << file_;
+    DPLOG(ERROR) << "mmap " << file_.GetPlatformFile();
 
   return data_ != MAP_FAILED;
 }
@@ -42,12 +39,10 @@ void MemoryMappedFile::CloseHandles() {
 
   if (data_ != NULL)
     munmap(data_, length_);
-  if (file_ != kInvalidPlatformFileValue)
-    close(file_);
+  file_.Close();
 
   data_ = NULL;
   length_ = 0;
-  file_ = kInvalidPlatformFileValue;
 }
 
 }  // namespace base
diff --git a/base/files/memory_mapped_file_win.cc b/base/files/memory_mapped_file_win.cc
index 6942129..f382287 100644
--- a/base/files/memory_mapped_file_win.cc
+++ b/base/files/memory_mapped_file_win.cc
@@ -5,83 +5,52 @@
 #include "base/files/memory_mapped_file.h"
 
 #include "base/files/file_path.h"
-#include "base/logging.h"
-#include "base/metrics/histogram.h"
 #include "base/strings/string16.h"
 #include "base/threading/thread_restrictions.h"
 
 namespace base {
 
-MemoryMappedFile::MemoryMappedFile()
-    : file_(INVALID_HANDLE_VALUE),
-      file_mapping_(INVALID_HANDLE_VALUE),
-      data_(NULL),
-      length_(INVALID_FILE_SIZE) {
+MemoryMappedFile::MemoryMappedFile() : data_(NULL), length_(0), image_(false) {
 }
 
 bool MemoryMappedFile::InitializeAsImageSection(const FilePath& file_name) {
-  if (IsValid())
-    return false;
-  file_ = CreatePlatformFile(file_name, PLATFORM_FILE_OPEN | PLATFORM_FILE_READ,
-                             NULL, NULL);
-
-  if (file_ == kInvalidPlatformFileValue) {
-    DLOG(ERROR) << "Couldn't open " << file_name.AsUTF8Unsafe();
-    return false;
-  }
-
-  if (!MapFileToMemoryInternalEx(SEC_IMAGE)) {
-    CloseHandles();
-    return false;
-  }
-
-  return true;
-}
-
-bool MemoryMappedFile::MapFileToMemoryInternal() {
-  return MapFileToMemoryInternalEx(0);
+  image_ = true;
+  return Initialize(file_name);
 }
 
-bool MemoryMappedFile::MapFileToMemoryInternalEx(int flags) {
+bool MemoryMappedFile::MapFileToMemory() {
   ThreadRestrictions::AssertIOAllowed();
 
-  if (file_ == INVALID_HANDLE_VALUE)
+  if (!file_.IsValid())
     return false;
 
-  length_ = ::GetFileSize(file_, NULL);
-  if (length_ == INVALID_FILE_SIZE)
+  int64 len = file_.GetLength();
+  if (len <= 0 || len > kint32max)
     return false;
+  length_ = static_cast<size_t>(len);
+
+  int flags = image_ ? SEC_IMAGE | PAGE_READONLY : PAGE_READONLY;
 
-  file_mapping_ = ::CreateFileMapping(file_, NULL, PAGE_READONLY | flags,
-                                      0, 0, NULL);
-  if (!file_mapping_) {
-    // According to msdn, system error codes are only reserved up to 15999.
-    // http://msdn.microsoft.com/en-us/library/ms681381(v=VS.85).aspx.
-    UMA_HISTOGRAM_ENUMERATION("MemoryMappedFile.CreateFileMapping",
-                              logging::GetLastSystemErrorCode(), 16000);
+  file_mapping_.Set(::CreateFileMapping(file_.GetPlatformFile(), NULL,
+                                        flags, 0, 0, NULL));
+  if (!file_mapping_.IsValid())
     return false;
-  }
 
-  data_ = static_cast<uint8*>(
-      ::MapViewOfFile(file_mapping_, FILE_MAP_READ, 0, 0, 0));
-  if (!data_) {
-    UMA_HISTOGRAM_ENUMERATION("MemoryMappedFile.MapViewOfFile",
-                              logging::GetLastSystemErrorCode(), 16000);
-  }
+  data_ = static_cast<uint8*>(::MapViewOfFile(file_mapping_.Get(),
+                                              FILE_MAP_READ, 0, 0, 0));
   return data_ != NULL;
 }
 
 void MemoryMappedFile::CloseHandles() {
   if (data_)
     ::UnmapViewOfFile(data_);
-  if (file_mapping_ != INVALID_HANDLE_VALUE)
-    ::CloseHandle(file_mapping_);
-  if (file_ != INVALID_HANDLE_VALUE)
-    ::CloseHandle(file_);
+  if (file_mapping_.IsValid())
+    file_mapping_.Close();
+  if (file_.IsValid())
+    file_.Close();
 
   data_ = NULL;
-  file_mapping_ = file_ = INVALID_HANDLE_VALUE;
-  length_ = INVALID_FILE_SIZE;
+  length_ = 0;
 }
 
 }  // namespace base