MemoryMappedFile: Make Initialize WARN_UNUSED_RESULT

It is easy to misuse this class and either:
1. Ignore the return value for |Initialize|
2. Forget to check |IsValid| after initialization

This CL makes it impossible to ignore initialize, so callers must
handler mmap failures.

Bug: None
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: Idc39565ccd80234ed54aafdd6174d37fff259e9f
Reviewed-on: https://chromium-review.googlesource.com/c/1262758Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Fredrik Hubinette <hubbe@chromium.org>
Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597221}

base/debug/activity_analyzer.cc

@@ -118,8 +118,7 @@ std::unique_ptr<GlobalActivityAnalyzer> GlobalActivityAnalyzer::CreateWithFile(
   // Map the file read-write so it can guarantee consistency between
   // the analyzer and any trackers that my still be active.
   std::unique_ptr<MemoryMappedFile> mmfile(new MemoryMappedFile());
-  mmfile->Initialize(file_path, MemoryMappedFile::READ_WRITE);
-  if (!mmfile->IsValid()) {
+  if (!mmfile->Initialize(file_path, MemoryMappedFile::READ_WRITE)) {
     LogAnalyzerCreationError(kInvalidMemoryMappedFile);
     return nullptr;
   }

base/debug/elf_reader_linux_unittest.cc

@@ -55,7 +55,7 @@ TEST(ElfReaderTest, ReadElfLibraryName) {
 
   // Memory map the so file and use it to test reading so name.
   MemoryMappedFile file;
-  file.Initialize(FilePath(filename));
+  ASSERT_TRUE(file.Initialize(FilePath(filename)));
   const void* addr = file.data();
 #endif
 

base/files/memory_mapped_file.h

@@ -69,8 +69,8 @@ class BASE_EXPORT MemoryMappedFile {
   // to a valid memory mapped file then this method will fail and return
   // false. If it cannot open the file, the file does not exist, or the
   // memory mapping fails, it will return false.
-  bool Initialize(const FilePath& file_name, Access access);
-  bool Initialize(const FilePath& file_name) {
+  WARN_UNUSED_RESULT bool Initialize(const FilePath& file_name, Access access);
+  WARN_UNUSED_RESULT bool Initialize(const FilePath& file_name) {
     return Initialize(file_name, READ_ONLY);
   }
 
@@ -79,8 +79,8 @@ class BASE_EXPORT MemoryMappedFile {
   // of |file| and closes it when done. |file| must have been opened with
   // permissions suitable for |access|. If the memory mapping fails, it will
   // return false.
-  bool Initialize(File file, Access access);
-  bool Initialize(File file) {
+  WARN_UNUSED_RESULT bool Initialize(File file, Access access);
+  WARN_UNUSED_RESULT bool Initialize(File file) {
     return Initialize(std::move(file), READ_ONLY);
   }
 
@@ -88,8 +88,10 @@ class BASE_EXPORT MemoryMappedFile {
   // |access| are allowed. If READ_WRITE_EXTEND is specified then |region|
   // provides the maximum size of the file. If the memory mapping fails, it
   // return false.
-  bool Initialize(File file, const Region& region, Access access);
-  bool Initialize(File file, const Region& region) {
+  WARN_UNUSED_RESULT bool Initialize(File file,
+                                     const Region& region,
+                                     Access access);
+  WARN_UNUSED_RESULT bool Initialize(File file, const Region& region) {
     return Initialize(std::move(file), region, READ_ONLY);
   }
 

base/files/memory_mapped_file_unittest.cc

@@ -63,7 +63,7 @@ TEST_F(MemoryMappedFileTest, MapWholeFileByPath) {
   const size_t kFileSize = 68 * 1024;
   CreateTemporaryTestFile(kFileSize);
   MemoryMappedFile map;
-  map.Initialize(temp_file_path());
+  ASSERT_TRUE(map.Initialize(temp_file_path()));
   ASSERT_EQ(kFileSize, map.length());
   ASSERT_TRUE(map.data() != nullptr);
   EXPECT_TRUE(map.IsValid());
@@ -74,7 +74,8 @@ TEST_F(MemoryMappedFileTest, MapWholeFileByFD) {
   const size_t kFileSize = 68 * 1024;
   CreateTemporaryTestFile(kFileSize);
   MemoryMappedFile map;
-  map.Initialize(File(temp_file_path(), File::FLAG_OPEN | File::FLAG_READ));
+  ASSERT_TRUE(map.Initialize(
+      File(temp_file_path(), File::FLAG_OPEN | File::FLAG_READ)));
   ASSERT_EQ(kFileSize, map.length());
   ASSERT_TRUE(map.data() != nullptr);
   EXPECT_TRUE(map.IsValid());
@@ -85,7 +86,7 @@ TEST_F(MemoryMappedFileTest, MapSmallFile) {
   const size_t kFileSize = 127;
   CreateTemporaryTestFile(kFileSize);
   MemoryMappedFile map;
-  map.Initialize(temp_file_path());
+  ASSERT_TRUE(map.Initialize(temp_file_path()));
   ASSERT_EQ(kFileSize, map.length());
   ASSERT_TRUE(map.data() != nullptr);
   EXPECT_TRUE(map.IsValid());
@@ -98,7 +99,8 @@ TEST_F(MemoryMappedFileTest, MapWholeFileUsingRegion) {
   MemoryMappedFile map;
 
   File file(temp_file_path(), File::FLAG_OPEN | File::FLAG_READ);
-  map.Initialize(std::move(file), MemoryMappedFile::Region::kWholeFile);
+  ASSERT_TRUE(
+      map.Initialize(std::move(file), MemoryMappedFile::Region::kWholeFile));
   ASSERT_EQ(kFileSize, map.length());
   ASSERT_TRUE(map.data() != nullptr);
   EXPECT_TRUE(map.IsValid());
@@ -113,7 +115,7 @@ TEST_F(MemoryMappedFileTest, MapPartialRegionAtBeginning) {
 
   File file(temp_file_path(), File::FLAG_OPEN | File::FLAG_READ);
   MemoryMappedFile::Region region = {0, kPartialSize};
-  map.Initialize(std::move(file), region);
+  ASSERT_TRUE(map.Initialize(std::move(file), region));
   ASSERT_EQ(kPartialSize, map.length());
   ASSERT_TRUE(map.data() != nullptr);
   EXPECT_TRUE(map.IsValid());
@@ -129,7 +131,7 @@ TEST_F(MemoryMappedFileTest, MapPartialRegionAtEnd) {
 
   File file(temp_file_path(), File::FLAG_OPEN | File::FLAG_READ);
   MemoryMappedFile::Region region = {kOffset, kPartialSize};
-  map.Initialize(std::move(file), region);
+  ASSERT_TRUE(map.Initialize(std::move(file), region));
   ASSERT_EQ(kPartialSize, map.length());
   ASSERT_TRUE(map.data() != nullptr);
   EXPECT_TRUE(map.IsValid());
@@ -146,7 +148,7 @@ TEST_F(MemoryMappedFileTest, MapSmallPartialRegionInTheMiddle) {
 
   File file(temp_file_path(), File::FLAG_OPEN | File::FLAG_READ);
   MemoryMappedFile::Region region = {kOffset, kPartialSize};
-  map.Initialize(std::move(file), region);
+  ASSERT_TRUE(map.Initialize(std::move(file), region));
   ASSERT_EQ(kPartialSize, map.length());
   ASSERT_TRUE(map.data() != nullptr);
   EXPECT_TRUE(map.IsValid());
@@ -163,7 +165,7 @@ TEST_F(MemoryMappedFileTest, MapLargePartialRegionInTheMiddle) {
 
   File file(temp_file_path(), File::FLAG_OPEN | File::FLAG_READ);
   MemoryMappedFile::Region region = {kOffset, kPartialSize};
-  map.Initialize(std::move(file), region);
+  ASSERT_TRUE(map.Initialize(std::move(file), region));
   ASSERT_EQ(kPartialSize, map.length());
   ASSERT_TRUE(map.data() != nullptr);
   EXPECT_TRUE(map.IsValid());
@@ -176,7 +178,7 @@ TEST_F(MemoryMappedFileTest, WriteableFile) {
 
   {
     MemoryMappedFile map;
-    map.Initialize(temp_file_path(), MemoryMappedFile::READ_WRITE);
+    ASSERT_TRUE(map.Initialize(temp_file_path(), MemoryMappedFile::READ_WRITE));
     ASSERT_EQ(kFileSize, map.length());
     ASSERT_TRUE(map.data() != nullptr);
     EXPECT_TRUE(map.IsValid());
@@ -211,8 +213,8 @@ TEST_F(MemoryMappedFileTest, ExtendableFile) {
               File::FLAG_OPEN | File::FLAG_READ | File::FLAG_WRITE);
     MemoryMappedFile::Region region = {0, kFileSize + kFileExtend};
     MemoryMappedFile map;
-    map.Initialize(std::move(file), region,
-                   MemoryMappedFile::READ_WRITE_EXTEND);
+    ASSERT_TRUE(map.Initialize(std::move(file), region,
+                               MemoryMappedFile::READ_WRITE_EXTEND));
     EXPECT_EQ(kFileSize + kFileExtend, map.length());
     ASSERT_TRUE(map.data() != nullptr);
     EXPECT_TRUE(map.IsValid());

base/metrics/persistent_histogram_allocator.cc

@@ -687,14 +687,15 @@ bool GlobalHistogramAllocator::CreateWithFile(
                  File::FLAG_READ | File::FLAG_WRITE);
 
   std::unique_ptr<MemoryMappedFile> mmfile(new MemoryMappedFile());
+  bool success = false;
   if (exists) {
     size = saturated_cast<size_t>(file.GetLength());
-    mmfile->Initialize(std::move(file), MemoryMappedFile::READ_WRITE);
+    success = mmfile->Initialize(std::move(file), MemoryMappedFile::READ_WRITE);
   } else {
-    mmfile->Initialize(std::move(file), {0, size},
+    success = mmfile->Initialize(std::move(file), {0, size},
                                  MemoryMappedFile::READ_WRITE_EXTEND);
   }
-  if (!mmfile->IsValid() ||
+  if (!success ||
       !FilePersistentMemoryAllocator::IsFileAcceptable(*mmfile, true)) {
     return false;
   }
@@ -844,9 +845,8 @@ bool GlobalHistogramAllocator::CreateSpareFile(const FilePath& spare_path,
       return false;
 
     MemoryMappedFile mmfile;
-    mmfile.Initialize(std::move(spare_file), {0, size},
+    success = mmfile.Initialize(std::move(spare_file), {0, size},
                                 MemoryMappedFile::READ_WRITE_EXTEND);
-    success = mmfile.IsValid();
   }
 
   if (success)

base/metrics/persistent_memory_allocator_unittest.cc

@@ -747,7 +747,7 @@ TEST(FilePersistentMemoryAllocatorTest, CreationTest) {
   }
 
   std::unique_ptr<MemoryMappedFile> mmfile(new MemoryMappedFile());
-  mmfile->Initialize(file_path);
+  ASSERT_TRUE(mmfile->Initialize(file_path));
   EXPECT_TRUE(mmfile->IsValid());
   const size_t mmlength = mmfile->length();
   EXPECT_GE(meminfo1.total, mmlength);
@@ -805,9 +805,9 @@ TEST(FilePersistentMemoryAllocatorTest, ExtendTest) {
   // Map it as an extendable read/write file and append to it.
   {
     std::unique_ptr<MemoryMappedFile> mmfile(new MemoryMappedFile());
-    mmfile->Initialize(
+    ASSERT_TRUE(mmfile->Initialize(
         File(file_path, File::FLAG_OPEN | File::FLAG_READ | File::FLAG_WRITE),
-        region, MemoryMappedFile::READ_WRITE_EXTEND);
+        region, MemoryMappedFile::READ_WRITE_EXTEND));
     FilePersistentMemoryAllocator allocator(std::move(mmfile), region.size, 0,
                                             "", false);
     EXPECT_EQ(static_cast<size_t>(before_size), allocator.used());
@@ -824,9 +824,9 @@ TEST(FilePersistentMemoryAllocatorTest, ExtendTest) {
   // Verify that it's still an acceptable file.
   {
     std::unique_ptr<MemoryMappedFile> mmfile(new MemoryMappedFile());
-    mmfile->Initialize(
+    ASSERT_TRUE(mmfile->Initialize(
         File(file_path, File::FLAG_OPEN | File::FLAG_READ | File::FLAG_WRITE),
-        region, MemoryMappedFile::READ_WRITE_EXTEND);
+        region, MemoryMappedFile::READ_WRITE_EXTEND));
     EXPECT_TRUE(FilePersistentMemoryAllocator::IsFileAcceptable(*mmfile, true));
     EXPECT_TRUE(
         FilePersistentMemoryAllocator::IsFileAcceptable(*mmfile, false));
@@ -869,7 +869,7 @@ TEST(FilePersistentMemoryAllocatorTest, AcceptableTest) {
         read_only ? MemoryMappedFile::READ_ONLY : MemoryMappedFile::READ_WRITE;
 
     mmfile.reset(new MemoryMappedFile());
-    mmfile->Initialize(File(file_path, file_flags), map_access);
+    ASSERT_TRUE(mmfile->Initialize(File(file_path, file_flags), map_access));
     EXPECT_EQ(filesize, mmfile->length());
     if (FilePersistentMemoryAllocator::IsFileAcceptable(*mmfile, read_only)) {
       // Make sure construction doesn't crash. It will, however, cause
@@ -911,7 +911,7 @@ TEST(FilePersistentMemoryAllocatorTest, AcceptableTest) {
     ASSERT_TRUE(PathExists(file_path));
 
     mmfile.reset(new MemoryMappedFile());
-    mmfile->Initialize(File(file_path, file_flags), map_access);
+    ASSERT_TRUE(mmfile->Initialize(File(file_path, file_flags), map_access));
     EXPECT_EQ(filesize, mmfile->length());
     if (FilePersistentMemoryAllocator::IsFileAcceptable(*mmfile, read_only)) {
       // Make sure construction doesn't crash. It will, however, cause
@@ -967,12 +967,12 @@ TEST_F(PersistentMemoryAllocatorTest, TruncateTest) {
       SCOPED_TRACE(StringPrintf("read_only=%s", read_only ? "true" : "false"));
 
       std::unique_ptr<MemoryMappedFile> mmfile(new MemoryMappedFile());
-      mmfile->Initialize(
+      ASSERT_TRUE(mmfile->Initialize(
           File(file_path, File::FLAG_OPEN |
                               (read_only ? File::FLAG_READ
                                          : File::FLAG_READ | File::FLAG_WRITE)),
           read_only ? MemoryMappedFile::READ_ONLY
-                    : MemoryMappedFile::READ_WRITE);
+                    : MemoryMappedFile::READ_WRITE));
       ASSERT_TRUE(
           FilePersistentMemoryAllocator::IsFileAcceptable(*mmfile, read_only));
 

base/trace_event/cfi_backtrace_android_unittest.cc

@@ -88,7 +88,7 @@ TEST(CFIBacktraceAndroidTest, DISABLED_TestFindCFIRow) {
       WriteFile(temp_path, reinterpret_cast<char*>(input), sizeof(input)));
 
   unwinder->cfi_mmap_.reset(new MemoryMappedFile());
-  unwinder->cfi_mmap_->Initialize(temp_path);
+  ASSERT_TRUE(unwinder->cfi_mmap_->Initialize(temp_path));
   unwinder->ParseCFITables();
 
   CFIBacktraceAndroid::CFIRow cfi_row = {0};

chrome/browser/supervised_user/experimental/supervised_user_blacklist.cc

@@ -21,8 +21,7 @@ ReadFromBinaryFileOnFileThread(const base::FilePath& path) {
       new std::vector<SupervisedUserBlacklist::Hash>);
 
   base::MemoryMappedFile file;
-  file.Initialize(path);
-  if (!file.IsValid())
+  if (!file.Initialize(path))
     return host_hashes;
 
   size_t size = file.length();

components/browser_watcher/stability_paths.cc

@@ -46,8 +46,7 @@ bool SetPmaFileDeleted(const base::FilePath& file_path) {
   // Map the file read-write so it can guarantee consistency between
   // the analyzer and any trackers that may still be active.
   std::unique_ptr<MemoryMappedFile> mmfile(new MemoryMappedFile());
-  mmfile->Initialize(file_path, MemoryMappedFile::READ_WRITE);
-  if (!mmfile->IsValid())
+  if (!mmfile->Initialize(file_path, MemoryMappedFile::READ_WRITE))
     return false;
   if (!FilePersistentMemoryAllocator::IsFileAcceptable(*mmfile, true))
     return false;

content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc

@@ -649,8 +649,7 @@ HRESULT FontFileStream::RuntimeClassInitialize(HANDLE handle) {
     return E_FAIL;
   }
 
-  data_.Initialize(base::File(duplicate_handle));
-  if (!data_.IsValid()) {
+  if (!data_.Initialize(base::File(duplicate_handle))) {
     LogFontProxyError(MAPPED_FILE_FAILED);
     return E_FAIL;
   }

media/ffmpeg/ffmpeg_common_unittest.cc

@@ -82,7 +82,7 @@ void TestConfigConvertExtraData(
 TEST_F(FFmpegCommonTest, AVStreamToDecoderConfig) {
   // Open a file to get a real AVStreams from FFmpeg.
   base::MemoryMappedFile file;
-  file.Initialize(GetTestDataFilePath("bear-320x240.webm"));
+  ASSERT_TRUE(file.Initialize(GetTestDataFilePath("bear-320x240.webm")));
   InMemoryUrlProtocol protocol(file.data(), file.length(), false);
   FFmpegGlue glue(&protocol);
   ASSERT_TRUE(glue.OpenContext());
@@ -125,8 +125,8 @@ TEST_F(FFmpegCommonTest, AVStreamToDecoderConfig) {
 
 TEST_F(FFmpegCommonTest, AVStreamToAudioDecoderConfig_OpusAmbisonics_4ch) {
   base::MemoryMappedFile file;
-  file.Initialize(
-      GetTestDataFilePath("bear-opus-end-trimming-4ch-channelmapping2.webm"));
+  ASSERT_TRUE(file.Initialize(
+      GetTestDataFilePath("bear-opus-end-trimming-4ch-channelmapping2.webm")));
   InMemoryUrlProtocol protocol(file.data(), file.length(), false);
   FFmpegGlue glue(&protocol);
   ASSERT_TRUE(glue.OpenContext());
@@ -148,8 +148,8 @@ TEST_F(FFmpegCommonTest, AVStreamToAudioDecoderConfig_OpusAmbisonics_4ch) {
 
 TEST_F(FFmpegCommonTest, AVStreamToAudioDecoderConfig_OpusAmbisonics_11ch) {
   base::MemoryMappedFile file;
-  file.Initialize(
-      GetTestDataFilePath("bear-opus-end-trimming-11ch-channelmapping2.webm"));
+  ASSERT_TRUE(file.Initialize(
+      GetTestDataFilePath("bear-opus-end-trimming-11ch-channelmapping2.webm")));
   InMemoryUrlProtocol protocol(file.data(), file.length(), false);
   FFmpegGlue glue(&protocol);
   ASSERT_TRUE(glue.OpenContext());
@@ -171,7 +171,7 @@ TEST_F(FFmpegCommonTest, AVStreamToAudioDecoderConfig_OpusAmbisonics_11ch) {
 
 TEST_F(FFmpegCommonTest, AVStreamToAudioDecoderConfig_9ch_wav) {
   base::MemoryMappedFile file;
-  file.Initialize(GetTestDataFilePath("9ch.wav"));
+  ASSERT_TRUE(file.Initialize(GetTestDataFilePath("9ch.wav")));
   InMemoryUrlProtocol protocol(file.data(), file.length(), false);
   FFmpegGlue glue(&protocol);
   ASSERT_TRUE(glue.OpenContext());

media/filters/file_data_source.cc

@@ -17,18 +17,16 @@ FileDataSource::FileDataSource()
       bytes_read_(0) {
 }
 
-FileDataSource::FileDataSource(base::File file)
-    : force_read_errors_(false),
-      force_streaming_(false),
-      bytes_read_(0) {
-  file_.Initialize(std::move(file));
-}
-
 bool FileDataSource::Initialize(const base::FilePath& file_path) {
   DCHECK(!file_.IsValid());
   return file_.Initialize(file_path);
 }
 
+bool FileDataSource::Initialize(base::File file) {
+  DCHECK(!file_.IsValid());
+  return file_.Initialize(std::move(file));
+}
+
 void FileDataSource::Stop() {}
 
 void FileDataSource::Abort() {}

media/filters/file_data_source.h

@@ -22,10 +22,10 @@ namespace media {
 class MEDIA_EXPORT FileDataSource : public DataSource {
  public:
   FileDataSource();
-  explicit FileDataSource(base::File file);
   ~FileDataSource() override;
 
-  bool Initialize(const base::FilePath& file_path);
+  WARN_UNUSED_RESULT bool Initialize(const base::FilePath& file_path);
+  WARN_UNUSED_RESULT bool Initialize(base::File file);
 
   // Implementation of DataSource.
   void Stop() override;

media/filters/media_file_checker.cc

@@ -37,7 +37,10 @@ MediaFileChecker::MediaFileChecker(base::File file) : file_(std::move(file)) {}
 MediaFileChecker::~MediaFileChecker() = default;
 
 bool MediaFileChecker::Start(base::TimeDelta check_time) {
-  media::FileDataSource source(std::move(file_));
+  media::FileDataSource source;
+  if (!source.Initialize(std::move(file_)))
+    return false;
+
   bool read_ok = true;
   media::BlockingUrlProtocol protocol(
       &source, base::Bind(&OnMediaFileCheckerError, &read_ok));