diff options
| -rw-r--r-- | chromecast/app/linux/cast_crash_reporter_client_unittest.cc | 158 | ||||
| -rw-r--r-- | chromecast/crash/linux/crash_util.cc | 21 | ||||
| -rw-r--r-- | chromecast/crash/linux/dummy_minidump_generator.cc | 80 | ||||
| -rw-r--r-- | chromecast/crash/linux/dummy_minidump_generator.h | 16 | ||||
| -rw-r--r-- | chromecast/crash/linux/dummy_minidump_generator_unittest.cc | 90 | ||||
| -rw-r--r-- | chromecast/crash/linux/minidump_generator.h | 7 | ||||
| -rw-r--r-- | chromecast/crash/linux/synchronized_minidump_manager.cc | 8 |
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(); |