Revert of Replace system IO calls in chromecast/crash with Chrome utilities. (patchset #9 id:160001 of https://codereview.chromium.org/1227963002/)

Reason for revert:
Breaks CastCrashReporterClientTest.EndToEndTestOnIORestrictedThread:

https://build.chromium.org/p/chromium.linux/builders/Cast%20Linux/builds/2709

Original issue's description:
> Replace system IO calls in chromecast/crash with Chrome utilities.
>
> System IO calls had been used to avoid thread IO restrictions in the
> crash handler. This change replaces each of these calls with the more
> platform-independent, stable Chrome IO utilities from ::base. Thread
> restrictions are disabled in crash_util.cc.
>
> BUG= b/22329428
>
> Committed: https://crrev.com/4927df40d0342522fc268f884835b43f9004549e
> Cr-Commit-Position: refs/heads/master@{#339161}

TBR=wzhong@chromium.org,bcf@google.com,gunsch@chromium.org,gfhuang@chromium.org,slan@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= b/22329428

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

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

7 files changed, 245 insertions, 135 deletions

diff --git a/chromecast/app/linux/cast_crash_reporter_client_unittest.cc b/chromecast/app/linux/cast_crash_reporter_client_unittest.cc
index 481eb30..88cbad9 100644
--- a/chromecast/app/linux/cast_crash_reporter_client_unittest.cc
+++ b/chromecast/app/linux/cast_crash_reporter_client_unittest.cc
@@ -10,7 +10,6 @@
 #include "base/files/file_util.h"
 #include "base/memory/scoped_vector.h"
 #include "base/test/scoped_path_override.h"
-#include "base/threading/thread_restrictions.h"
 #include "chromecast/app/linux/cast_crash_reporter_client.h"
 #include "chromecast/crash/app_state_tracker.h"
 #include "chromecast/crash/linux/crash_util.h"
@@ -47,110 +46,71 @@ ScopedVector<DumpInfo> GetCurrentDumps(const std::string& logfile_path) {
 
 }  // namespace
 
-class CastCrashReporterClientTest : public testing::Test {
- protected:
-  CastCrashReporterClientTest() {}
-  ~CastCrashReporterClientTest() override {}
+TEST(CastCrashReporterClientTest, EndToEnd) {
+  // Set up a temporary directory which will be used as our fake home dir.
+  base::FilePath fake_home_dir;
+  ASSERT_TRUE(base::CreateNewTempDirectory("", &fake_home_dir));
+  base::ScopedPathOverride home(base::DIR_HOME, fake_home_dir);
 
-  static void SetUpTestCase() {
-    // Set a callback to be used in place of the |dumpstate| executable.
-    CrashUtil::SetDumpStateCbForTest(base::Bind(&WriteFakeDumpStateFile));
-  }
-
-  // testing::Test implementation:
-  void SetUp() override {
-    // Set up a temporary directory which will be used as our fake home dir.
-    ASSERT_TRUE(base::CreateNewTempDirectory("", &fake_home_dir_));
-    home_override_.reset(
-        new base::ScopedPathOverride(base::DIR_HOME, fake_home_dir_));
-
-    // "Launch" YouTube.
-    AppStateTracker::SetLastLaunchedApp("youtube");
-    AppStateTracker::SetCurrentApp("youtube");
-
-    // "Launch" and switch to Pandora.
-    AppStateTracker::SetLastLaunchedApp("pandora");
-    AppStateTracker::SetCurrentApp("pandora");
-
-    // "Launch" Netflix.
-    AppStateTracker::SetLastLaunchedApp("netflix");
-    // Netflix crashed.
-
-    // A minidump file is created.
-    base::CreateTemporaryFile(&minidump_path_);
-    base::File minidump(minidump_path_,
-                        base::File::FLAG_OPEN | base::File::FLAG_APPEND);
-    minidump.Write(0, kFakeMinidumpContents, sizeof(kFakeMinidumpContents) - 1);
-    minidump.Close();
-  }
-
-  void TearDown() override {
-    // Remove IO restrictions in order to examine the state of the filesystem.
-    base::ThreadRestrictions::SetIOAllowed(true);
-
-    // Assert that the original file has been moved.
-    ASSERT_FALSE(base::PathExists(minidump_path_));
-
-    // Assert that the file has been moved to "minidumps", with the expected
-    // contents.
-    std::string contents;
-    base::FilePath new_minidump =
-        fake_home_dir_.Append("minidumps").Append(minidump_path_.BaseName());
-    ASSERT_TRUE(base::PathExists(new_minidump));
-    ASSERT_TRUE(base::ReadFileToString(new_minidump, &contents));
-    ASSERT_EQ(kFakeMinidumpContents, contents);
-
-    // Assert that the dumpstate file has been written with the expected
-    // contents.
-    base::FilePath dumpstate = new_minidump.AddExtension(".txt.gz");
-    ASSERT_TRUE(base::PathExists(dumpstate));
-    ASSERT_TRUE(base::ReadFileToString(dumpstate, &contents));
-    ASSERT_EQ(kFakeDumpstateContents, contents);
-
-    // Assert that the lockfile has logged the correct information.
-    base::FilePath lockfile =
-        fake_home_dir_.Append("minidumps").Append("lockfile");
-    ASSERT_TRUE(base::PathExists(lockfile));
-    ScopedVector<DumpInfo> dumps = GetCurrentDumps(lockfile.value());
-    ASSERT_EQ(1u, dumps.size());
-
-    const DumpInfo& dump_info = *(dumps[0]);
-    ASSERT_TRUE(dump_info.valid());
-    EXPECT_EQ(new_minidump.value(), dump_info.crashed_process_dump());
-    EXPECT_EQ(dumpstate.value(), dump_info.logfile());
-    EXPECT_EQ("youtube", dump_info.params().previous_app_name);
-    EXPECT_EQ("pandora", dump_info.params().current_app_name);
-    EXPECT_EQ("netflix", dump_info.params().last_app_name);
-  }
+  // Set a callback to be used in place of the |dumpstate| executable.
+  CrashUtil::SetDumpStateCbForTest(base::Bind(&WriteFakeDumpStateFile));
 
-  const base::FilePath& minidump_path() { return minidump_path_; }
+  // "Launch" YouTube.
+  AppStateTracker::SetLastLaunchedApp("youtube");
+  AppStateTracker::SetCurrentApp("youtube");
 
- private:
-  base::FilePath fake_home_dir_;
-  base::FilePath minidump_path_;
-  scoped_ptr<base::ScopedPathOverride> home_override_;
-};
+  // "Launch" and switch to Pandora.
+  AppStateTracker::SetLastLaunchedApp("pandora");
+  AppStateTracker::SetCurrentApp("pandora");
 
-TEST_F(CastCrashReporterClientTest, EndToEndTestOnIORestrictedThread) {
-  // Handle a "crash" on an IO restricted thread.
-  base::ThreadRestrictions::SetIOAllowed(false);
-  CastCrashReporterClient client;
-  ASSERT_TRUE(client.HandleCrashDump(minidump_path().value().c_str()));
+  // "Launch" Netflix.
+  AppStateTracker::SetLastLaunchedApp("netflix");
+  // Netflix crashed.
 
-  // Assert that the thread is IO restricted when the function exits.
-  // Note that SetIOAllowed returns the previous value.
-  ASSERT_FALSE(base::ThreadRestrictions::SetIOAllowed(true));
-}
+  // A minidump file is created.
+  base::FilePath minidump_path;
+  base::CreateTemporaryFile(&minidump_path);
+  base::File minidump(minidump_path,
+                      base::File::FLAG_OPEN | base::File::FLAG_APPEND);
+  minidump.Write(0, kFakeMinidumpContents, sizeof(kFakeMinidumpContents) - 1);
+  minidump.Close();
 
-TEST_F(CastCrashReporterClientTest, EndToEndTestOnNonIORestrictedThread) {
-  // Handle a crash on a non-IO restricted thread.
-  base::ThreadRestrictions::SetIOAllowed(true);
+  // Handle the crash.
   CastCrashReporterClient client;
-  ASSERT_TRUE(client.HandleCrashDump(minidump_path().value().c_str()));
-
-  // Assert that the thread is not IO restricted when the function exits.
-  // Note that SetIOAllowed returns the previous value.
-  ASSERT_TRUE(base::ThreadRestrictions::SetIOAllowed(true));
+  ASSERT_TRUE(client.HandleCrashDump(minidump_path.value().c_str()));
+
+  // Assert that the original file has been moved.
+  ASSERT_FALSE(base::PathExists(minidump_path));
+
+  // Assert that the file has been moved to "minidumps", with the expected
+  // contents.
+  std::string contents;
+  base::FilePath new_minidump =
+      fake_home_dir.Append("minidumps").Append(minidump_path.BaseName());
+  ASSERT_TRUE(base::PathExists(new_minidump));
+  ASSERT_TRUE(base::ReadFileToString(new_minidump, &contents));
+  ASSERT_EQ(kFakeMinidumpContents, contents);
+
+  // Assert that the dumpstate file has been written with the expected contents.
+  base::FilePath dumpstate = new_minidump.AddExtension(".txt.gz");
+  ASSERT_TRUE(base::PathExists(dumpstate));
+  ASSERT_TRUE(base::ReadFileToString(dumpstate, &contents));
+  ASSERT_EQ(kFakeDumpstateContents, contents);
+
+  // Assert that the lockfile has logged the correct information.
+  base::FilePath lockfile =
+      fake_home_dir.Append("minidumps").Append("lockfile");
+  ASSERT_TRUE(base::PathExists(lockfile));
+  ScopedVector<DumpInfo> dumps = GetCurrentDumps(lockfile.value());
+  ASSERT_EQ(1u, dumps.size());
+
+  const DumpInfo& dump_info = *(dumps[0]);
+  ASSERT_TRUE(dump_info.valid());
+  EXPECT_EQ(new_minidump.value(), dump_info.crashed_process_dump());
+  EXPECT_EQ(dumpstate.value(), dump_info.logfile());
+  EXPECT_EQ("youtube", dump_info.params().previous_app_name);
+  EXPECT_EQ("pandora", dump_info.params().current_app_name);
+  EXPECT_EQ("netflix", dump_info.params().last_app_name);
 }
 
-}  // namespace chromecast
+}  // namespace chromecast
\ No newline at end of file
diff --git a/chromecast/crash/linux/crash_util.cc b/chromecast/crash/linux/crash_util.cc
index 74348f4..e80b179c 100644
--- a/chromecast/crash/linux/crash_util.cc
+++ b/chromecast/crash/linux/crash_util.cc
@@ -4,10 +4,10 @@
 
 #include "chromecast/crash/linux/crash_util.h"
 
+#include <stdlib.h>
+
 #include "base/bind.h"
 #include "base/files/file_path.h"
-#include "base/files/file_util.h"
-#include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
 #include "chromecast/base/path_utils.h"
 #include "chromecast/base/version.h"
@@ -37,13 +37,12 @@ bool CrashUtil::RequestUploadCrashDump(
     const std::string& existing_minidump_path,
     const std::string& crashed_process_name,
     uint64_t crashed_process_start_time_ms) {
-  // Remove IO restrictions from this thread. Chromium IO functions must be used
-  // to access the file system and upload information to the crash server.
-  const bool io_allowed = base::ThreadRestrictions::SetIOAllowed(true);
-
   LOG(INFO) << "Request to upload crash dump " << existing_minidump_path
             << " for process " << crashed_process_name;
 
+  // Note: Do not use Chromium IO methods in this function. When cast_shell
+  // crashes, this function can be called by any thread, which may not allow IO.
+  // Use stdlib system calls for IO instead.
   uint64_t uptime_ms = GetCurrentTimeMs() - crashed_process_start_time_ms;
   MinidumpParams params(crashed_process_name,
                         uptime_ms,
@@ -69,11 +68,10 @@ bool CrashUtil::RequestUploadCrashDump(
   writer->set_non_blocking(false);
   success = (0 == writer->Write());  // error already logged.
 
-  // In case the file is still in $TEMP, remove it. Note that DeleteFile() will
-  // return true if |existing_minidump_path| has already been moved.
-  if (!base::DeleteFile(base::FilePath(existing_minidump_path), false)) {
+  // In case the file is still in $TEMP, remove it.
+  if (remove(existing_minidump_path.c_str()) < 0 && errno != ENOENT) {
     LOG(ERROR) << "Unable to delete temp minidump file "
-               << existing_minidump_path;
+               << existing_minidump_path << ": " << strerror(errno);
     success = false;
   }
 
@@ -81,9 +79,6 @@ bool CrashUtil::RequestUploadCrashDump(
   LOG(INFO) << "Request to upload crash dump finished. "
             << "Exit now if it is main process that crashed." << std::endl;
 
-  // Restore the original IO restrictions on the thread, if there were any.
-  base::ThreadRestrictions::SetIOAllowed(io_allowed);
-
   return success;
 }
 
diff --git a/chromecast/crash/linux/dummy_minidump_generator.cc b/chromecast/crash/linux/dummy_minidump_generator.cc
index a0cea9f..019febf 100644
--- a/chromecast/crash/linux/dummy_minidump_generator.cc
+++ b/chromecast/crash/linux/dummy_minidump_generator.cc
@@ -4,30 +4,96 @@
 
 #include "chromecast/crash/linux/dummy_minidump_generator.h"
 
-#include "base/files/file_util.h"
+#include <stdio.h>
+#include <sys/stat.h>
+
+#include <vector>
+
 #include "base/logging.h"
-#include "base/threading/thread_restrictions.h"
 
 namespace chromecast {
 
+namespace {
+
+const int kBufferSize = 32768;
+
+}  // namespace
+
 DummyMinidumpGenerator::DummyMinidumpGenerator(
     const std::string& existing_minidump_path)
     : existing_minidump_path_(existing_minidump_path) {
 }
 
 bool DummyMinidumpGenerator::Generate(const std::string& minidump_path) {
-  base::ThreadRestrictions::AssertIOAllowed();
+  // Use stdlib calls here to avoid potential IO restrictions on this thread.
 
   // Return false if the file does not exist.
-  if (!base::PathExists(base::FilePath(existing_minidump_path_))) {
-    LOG(ERROR) << existing_minidump_path_ << " does not exist.";
+  struct stat st;
+  if (stat(existing_minidump_path_.c_str(), &st) != 0) {
+    PLOG(ERROR) << existing_minidump_path_.c_str() << " does not exist: ";
     return false;
   }
 
   LOG(INFO) << "Moving minidump from " << existing_minidump_path_ << " to "
             << minidump_path << " for further uploading.";
-  return base::Move(base::FilePath(existing_minidump_path_),
-                    base::FilePath(minidump_path));
+
+  // Attempt to rename(). If this operation fails, the files are on different
+  // volumes. Fall back to a copy and delete.
+  if (rename(existing_minidump_path_.c_str(), minidump_path.c_str()) < 0) {
+    // Any errors will be logged within CopyAndDelete().
+    return CopyAndDelete(minidump_path);
+  }
+
+  return true;
+}
+
+bool DummyMinidumpGenerator::CopyAndDelete(const std::string& dest_path) {
+  FILE* src = fopen(existing_minidump_path_.c_str(), "r");
+  if (!src) {
+    PLOG(ERROR) << existing_minidump_path_ << " failed to open: ";
+    return false;
+  }
+
+  FILE* dest = fopen(dest_path.c_str(), "w");
+  if (!dest) {
+    PLOG(ERROR) << dest_path << " failed to open: ";
+    return false;
+  }
+
+  // Copy all bytes from |src| into |dest|.
+  std::vector<char> buffer(kBufferSize);
+  bool success = false;
+  while (!success) {
+    size_t bytes_read = fread(&buffer[0], 1, buffer.size(), src);
+    if (bytes_read < buffer.size()) {
+      if (feof(src)) {
+        success = true;
+      } else {
+        // An error occurred.
+        PLOG(ERROR) << "Error reading " << existing_minidump_path_ << ": ";
+        break;
+      }
+    }
+
+    size_t bytes_written = fwrite(&buffer[0], 1, bytes_read, dest);
+    if (bytes_written < bytes_read) {
+      // An error occurred.
+      PLOG(ERROR) << "Error writing to " << dest_path << ": ";
+      success = false;
+      break;
+    }
+  }
+
+  // Close both files.
+  fclose(src);
+  fclose(dest);
+
+  // Attempt to delete file at |existing_minidump_path_|. We should log this
+  // error, but the function should not fail if the file is not removed.
+  if (remove(existing_minidump_path_.c_str()) < 0)
+    PLOG(ERROR) << "Could not remove " << existing_minidump_path_ << ": ";
+
+  return success;
 }
 
 }  // namespace chromecast
diff --git a/chromecast/crash/linux/dummy_minidump_generator.h b/chromecast/crash/linux/dummy_minidump_generator.h
index 5eabca8..eb03125 100644
--- a/chromecast/crash/linux/dummy_minidump_generator.h
+++ b/chromecast/crash/linux/dummy_minidump_generator.h
@@ -21,12 +21,22 @@ class DummyMinidumpGenerator : public MinidumpGenerator {
   explicit DummyMinidumpGenerator(const std::string& existing_minidump_path);
 
   // MinidumpGenerator implementation:
-  // Moves the minidump located at |existing_minidump_path_| to |minidump_path|.
-  // Returns true if successful, false otherwise. Note that this function MUST
-  // be called on a thread without IO restrictions, or it will fail fatally.
   bool Generate(const std::string& minidump_path) override;
 
+  // Provide access to internal utility for testing.
+  bool CopyAndDeleteForTest(const std::string& dest_path) {
+    return CopyAndDelete(dest_path);
+  }
+
  private:
+  // Copies the contents of the file at |existing_minidump_path_| to the file at
+  // |dest_path|. If the copy operation succeeds, delete the file at
+  // |existing_minidump_path_|. Returns false if |existing_minidump_path_| can't
+  // be opened, or if the copy operation fails. Ideally, we would use Chrome
+  // utilities for a task like this, but we must ensure that this operation can
+  // occur on any thread (regardless of IO restrictions).
+  bool CopyAndDelete(const std::string& dest_path);
+
   const std::string existing_minidump_path_;
 
   DISALLOW_COPY_AND_ASSIGN(DummyMinidumpGenerator);
diff --git a/chromecast/crash/linux/dummy_minidump_generator_unittest.cc b/chromecast/crash/linux/dummy_minidump_generator_unittest.cc
index d70ea76..c38304f 100644
--- a/chromecast/crash/linux/dummy_minidump_generator_unittest.cc
+++ b/chromecast/crash/linux/dummy_minidump_generator_unittest.cc
@@ -11,6 +11,12 @@
 
 namespace chromecast {
 
+namespace {
+// This value should stay in sync with the internal buffer size used in
+// CopyAndDelete().
+const int kInternalBufferSize = 32768;
+}  // namespace
+
 TEST(DummyMinidumpGeneratorTest, GenerateFailsWithInvalidPath) {
   // Create directory in which to put minidump.
   base::FilePath minidump_dir;
@@ -21,7 +27,7 @@ TEST(DummyMinidumpGeneratorTest, GenerateFailsWithInvalidPath) {
   ASSERT_FALSE(generator.Generate(minidump_dir.Append("minidump.dmp").value()));
 }
 
-TEST(DummyMinidumpGeneratorTest, GenerateSucceedsWithSmallSource) {
+TEST(DummyMinidumpGeneratorTest, GenerateSucceedsWithValidPath) {
   // Create directory in which to put minidump.
   base::FilePath minidump_dir;
   ASSERT_TRUE(base::CreateNewTempDirectory("", &minidump_dir));
@@ -46,24 +52,98 @@ TEST(DummyMinidumpGeneratorTest, GenerateSucceedsWithSmallSource) {
   EXPECT_EQ(data, copied_data);
 }
 
-TEST(DummyMinidumpGeneratorTest, GenerateSucceedsWithLargeSource) {
+TEST(DummyMinidumpGeneratorTest, CopyAndDeleteFailsWithInvalidSource) {
+  // Create directory in which to put minidump.
+  base::FilePath minidump_dir;
+  ASSERT_TRUE(base::CreateNewTempDirectory("", &minidump_dir));
+
+  // Attempt to copy from an invalid path.
+  DummyMinidumpGenerator generator("/path/does/not/exist/minidump.dmp");
+  ASSERT_FALSE(generator.CopyAndDeleteForTest(
+      minidump_dir.Append("minidump.dmp").value()));
+}
+
+TEST(DummyMinidumpGeneratorTest, CopyAndDeleteSucceedsWithSmallSource) {
+  // Create directory in which to put minidump.
+  base::FilePath minidump_dir;
+  ASSERT_TRUE(base::CreateNewTempDirectory("", &minidump_dir));
+
+  // Create a fake minidump file.
+  base::FilePath fake_minidump;
+  ASSERT_TRUE(base::CreateTemporaryFile(&fake_minidump));
+  const std::string data("Test contents of the minidump file.\n");
+  ASSERT_EQ(static_cast<int>(data.size()),
+            base::WriteFile(fake_minidump, data.c_str(), data.size()));
+
+  base::FilePath new_minidump = minidump_dir.Append("minidump.dmp");
+  DummyMinidumpGenerator generator(fake_minidump.value());
+  ASSERT_TRUE(generator.CopyAndDeleteForTest(new_minidump.value()));
+
+  // Original file should not exist, and new file should contain original
+  // contents.
+  std::string copied_data;
+  EXPECT_FALSE(base::PathExists(fake_minidump));
+  ASSERT_TRUE(base::PathExists(new_minidump));
+  EXPECT_TRUE(base::ReadFileToString(new_minidump, &copied_data));
+  EXPECT_EQ(data, copied_data);
+}
+
+TEST(DummyMinidumpGeneratorTest, CopyAndDeleteSucceedsWithLargeSource) {
+  // Create directory in which to put minidump.
+  base::FilePath minidump_dir;
+  ASSERT_TRUE(base::CreateNewTempDirectory("", &minidump_dir));
+
+  // Create a large fake minidump file.
+  // Note: The file must be greater than the size of the buffer used to copy the
+  // file in CopyAndDelete(). Create a big string in memory.
+  base::FilePath fake_minidump;
+  ASSERT_TRUE(base::CreateTemporaryFile(&fake_minidump));
+  size_t str_len = kInternalBufferSize * 10 + 1;
+  const std::string data = base::RandBytesAsString(str_len);
+
+  // Write the string to the file and verify that the file is big enough.
+  ASSERT_EQ(static_cast<int>(data.size()),
+            base::WriteFile(fake_minidump, data.c_str(), data.size()));
+  int64_t filesize = 0;
+  ASSERT_TRUE(base::GetFileSize(fake_minidump, &filesize));
+  ASSERT_GT(filesize, kInternalBufferSize);
+
+  base::FilePath new_minidump = minidump_dir.Append("minidump.dmp");
+  DummyMinidumpGenerator generator(fake_minidump.value());
+  ASSERT_TRUE(generator.CopyAndDeleteForTest(new_minidump.value()));
+
+  // Original file should not exist, and new file should contain original
+  // contents.
+  std::string copied_data;
+  EXPECT_FALSE(base::PathExists(fake_minidump));
+  ASSERT_TRUE(base::PathExists(new_minidump));
+  EXPECT_TRUE(base::ReadFileToString(new_minidump, &copied_data));
+  EXPECT_EQ(data, copied_data);
+}
+
+TEST(DummyMinidumpGeneratorTest, CopyAndDeleteSucceedsWhenEOFAlignsWithBuffer) {
   // Create directory in which to put minidump.
   base::FilePath minidump_dir;
   ASSERT_TRUE(base::CreateNewTempDirectory("", &minidump_dir));
 
   // Create a large fake minidump file.
+  // Note: The file must be greater than the size of the buffer used to copy the
+  // file in CopyAndDelete(). Create a big string in memory.
   base::FilePath fake_minidump;
   ASSERT_TRUE(base::CreateTemporaryFile(&fake_minidump));
-  size_t str_len = 32768 * 10 + 1;
+  size_t str_len = kInternalBufferSize;
   const std::string data = base::RandBytesAsString(str_len);
 
-  // Write the string to the file.
+  // Write the string to the file and verify that the file is big enough.
   ASSERT_EQ(static_cast<int>(data.size()),
             base::WriteFile(fake_minidump, data.c_str(), data.size()));
+  int64_t filesize = 0;
+  ASSERT_TRUE(base::GetFileSize(fake_minidump, &filesize));
+  ASSERT_EQ(kInternalBufferSize, filesize);
 
   base::FilePath new_minidump = minidump_dir.Append("minidump.dmp");
   DummyMinidumpGenerator generator(fake_minidump.value());
-  ASSERT_TRUE(generator.Generate(new_minidump.value()));
+  ASSERT_TRUE(generator.CopyAndDeleteForTest(new_minidump.value()));
 
   // Original file should not exist, and new file should contain original
   // contents.
diff --git a/chromecast/crash/linux/minidump_generator.h b/chromecast/crash/linux/minidump_generator.h
index 0601dea..9780e43 100644
--- a/chromecast/crash/linux/minidump_generator.h
+++ b/chromecast/crash/linux/minidump_generator.h
@@ -9,15 +9,12 @@
 
 namespace chromecast {
 
-// An interface to generate a minidump at a given filepath.
 class MinidumpGenerator {
  public:
   virtual ~MinidumpGenerator() {}
 
-  // Generates a minidump file at |minidump_path|. This method should only be
-  // called on a thread without IO restrictions, as non-trivial implementations
-  // will almost certainly require IO permissions. Returns true if minidump was
-  // successfully generated.
+  // Interface to generate a minidump file in given path.
+  // This is called inside MinidumpWriter::DoWorkLocked().
   virtual bool Generate(const std::string& minidump_path) = 0;
 };
 
diff --git a/chromecast/crash/linux/synchronized_minidump_manager.cc b/chromecast/crash/linux/synchronized_minidump_manager.cc
index c21e625..358427a 100644
--- a/chromecast/crash/linux/synchronized_minidump_manager.cc
+++ b/chromecast/crash/linux/synchronized_minidump_manager.cc
@@ -153,10 +153,12 @@ int SynchronizedMinidumpManager::ParseLockFile() {
   // Grab each entry.
   while (std::getline(in, entry)) {
     scoped_ptr<DumpInfo> info(new DumpInfo(entry));
-    if (info->valid() && info->crashed_process_dump().size() > 0)
+    if (info->valid() && info->crashed_process_dump().size() > 0) {
       dumps->push_back(info.Pass());
-    else
-      LOG(WARNING) << "Ignoring invalid entry: " << entry;
+    } else {
+      LOG(WARNING) << "Entry is not valid: " << entry;
+      return -1;
+    }
   }
 
   dump_metadata_ = dumps.Pass();