crash-reporter: Write out a magic string for Chrome crashes. Also do some much needed code cleanup. BUG=chromium:363660 TEST=emerge platform2 Change-Id: Ica9abfd854e2c77d970851805989c86a6a45fdee Reviewed-on: https://chromium-review.googlesource.com/196764 Commit-Queue: Lei Zhang <thestig@chromium.org> Tested-by: Lei Zhang <thestig@chromium.org> Reviewed-by: Mike Frysinger <vapier@chromium.org>
diff --git a/chrome_collector.cc b/chrome_collector.cc index 896d0c3..9798ec5 100644 --- a/chrome_collector.cc +++ b/chrome_collector.cc
@@ -128,11 +128,11 @@ } //namespace -ChromeCollector::ChromeCollector() {} +ChromeCollector::ChromeCollector() : output_file_ptr_(stdout) {} ChromeCollector::~ChromeCollector() {} -bool ChromeCollector::HandleCrash(const std::string &file_path, +bool ChromeCollector::HandleCrash(const FilePath &file_path, const std::string &pid_string, const std::string &uid_string, const std::string &exe_name) { @@ -158,8 +158,8 @@ FilePath log_path = GetCrashPath(dir, dump_basename, "log.tar.xz"); std::string data; - if (!base::ReadFileToString(FilePath(file_path), &data)) { - LOG(ERROR) << "Can't read crash log: " << file_path.c_str(); + if (!base::ReadFileToString(file_path, &data)) { + LOG(ERROR) << "Can't read crash log: " << file_path.value(); return false; } @@ -186,6 +186,9 @@ // We're done. WriteCrashMetaData(meta_path, exe_name, minidump_path.value()); + fprintf(output_file_ptr_, "%s", kSuccessMagic); + fflush(output_file_ptr_); + return true; } @@ -288,3 +291,6 @@ return at == data.size(); } + +// static +const char ChromeCollector::kSuccessMagic[] = "_sys_cr_finished";
diff --git a/chrome_collector.h b/chrome_collector.h index ab59407..c6dbfc9 100644 --- a/chrome_collector.h +++ b/chrome_collector.h
@@ -19,9 +19,14 @@ ChromeCollector(); virtual ~ChromeCollector(); + // Magic string to let Chrome know the crash report succeeded. + static const char kSuccessMagic[]; + // Handle a specific chrome crash. Returns true on success. - bool HandleCrash(const std::string &file_path, const std::string &pid_string, - const std::string &uid_string, const std::string &exe_name); + bool HandleCrash(const base::FilePath &file_path, + const std::string &pid_string, + const std::string &uid_string, + const std::string &exe_name); private: friend class ChromeCollectorTest; @@ -29,6 +34,7 @@ FRIEND_TEST(ChromeCollectorTest, BadValues); FRIEND_TEST(ChromeCollectorTest, Newlines); FRIEND_TEST(ChromeCollectorTest, File); + FRIEND_TEST(ChromeCollectorTest, HandleCrash); // Crashes are expected to be in a TLV-style format of: // <name>:<length>:<value> @@ -39,6 +45,8 @@ bool ParseCrashLog(const std::string &data, const base::FilePath &dir, const base::FilePath &minidump, const std::string &basename); + + FILE *output_file_ptr_; }; #endif
diff --git a/chrome_collector_test.cc b/chrome_collector_test.cc index 8a82cb2..37a180b 100644 --- a/chrome_collector_test.cc +++ b/chrome_collector_test.cc
@@ -2,13 +2,13 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include <bits/wordsize.h> -#include <elf.h> -#include <unistd.h> +#include <stdio.h> +#include <dbus/dbus-glib-lowlevel.h> + +#include "base/auto_reset.h" #include "base/file_util.h" #include "base/files/scoped_temp_dir.h" -#include "base/strings/string_split.h" #include "chromeos/syslog_logging.h" #include "chromeos/test_helpers.h" #include "crash-reporter/chrome_collector.h" @@ -30,37 +30,30 @@ "value3:2:ok"; void CountCrash() { - static int s_crashes = 0; - ++s_crashes; } +static bool s_allow_crash = false; + bool IsMetrics() { - return false; + return s_allow_crash; } class ChromeCollectorTest : public ::testing::Test { - void SetUp() { - collector_.Initialize(CountCrash, IsMetrics); - pid_ = getpid(); - chromeos::ClearLog(); - } - protected: void ExpectFileEquals(const char *golden, - const char *file_path) { + const FilePath &file_path) { std::string contents; - EXPECT_TRUE(base::ReadFileToString(FilePath(file_path), &contents)); + EXPECT_TRUE(base::ReadFileToString(file_path, &contents)); EXPECT_EQ(golden, contents); } - std::vector<std::string> SplitLines(const std::string &lines) const { - std::vector<std::string> result; - base::SplitString(lines, '\n', &result); - return result; - } - ChromeCollector collector_; - pid_t pid_; + + private: + void SetUp() OVERRIDE { + collector_.Initialize(CountCrash, IsMetrics); + chromeos::ClearLog(); + } }; TEST_F(ChromeCollectorTest, GoodValues) { @@ -107,7 +100,9 @@ } TEST_F(ChromeCollectorTest, File) { - FilePath dir("."); + base::ScopedTempDir scoped_temp_dir; + ASSERT_TRUE(scoped_temp_dir.CreateUniqueTempDir()); + const FilePath& dir = scoped_temp_dir.path(); EXPECT_TRUE(collector_.ParseCrashLog(kCrashFormatWithFile, dir, dir.Append("minidump.dmp"), "base")); @@ -118,12 +113,34 @@ EXPECT_TRUE(meta.find("value1=abcdefghij") != std::string::npos); EXPECT_TRUE(meta.find("value2=12345") != std::string::npos); EXPECT_TRUE(meta.find("value3=ok") != std::string::npos); - ExpectFileEquals("12345\n789\n12345", - dir.Append("base-foo.txt.other").value().c_str()); - base::DeleteFile(dir.Append("base-foo.txt.other"), false); + ExpectFileEquals("12345\n789\n12345", dir.Append("base-foo.txt.other")); +} + +TEST_F(ChromeCollectorTest, HandleCrash) { + base::AutoReset<bool> auto_reset(&s_allow_crash, true); + base::ScopedTempDir scoped_temp_dir; + ASSERT_TRUE(scoped_temp_dir.CreateUniqueTempDir()); + const FilePath& dir = scoped_temp_dir.path(); + FilePath dump_file = dir.Append("test.dmp"); + ASSERT_EQ(strlen(kCrashFormatWithFile), + file_util::WriteFile(dump_file, kCrashFormatWithFile, + strlen(kCrashFormatWithFile))); + collector_.ForceCrashDirectory(dir); + + FilePath log_file; + { + file_util::ScopedFILE output( + base::CreateAndOpenTemporaryFileInDir(dir, &log_file)); + ASSERT_TRUE(output.get()); + base::AutoReset<FILE*> auto_reset_file_ptr(&collector_.output_file_ptr_, + output.get()); + EXPECT_TRUE(collector_.HandleCrash(dump_file, "123", "456", "chrome_test")); + } + ExpectFileEquals(ChromeCollector::kSuccessMagic, log_file); } int main(int argc, char **argv) { + ::g_type_init(); SetUpTests(&argc, argv, false); return RUN_ALL_TESTS(); }
diff --git a/crash_collector.cc b/crash_collector.cc index 0521a06..1a0898b 100644 --- a/crash_collector.cc +++ b/crash_collector.cc
@@ -76,8 +76,7 @@ using base::StringPrintf; CrashCollector::CrashCollector() - : forced_crash_directory_(NULL), - lsb_release_(kLsbRelease), + : lsb_release_(kLsbRelease), log_config_path_(kDefaultLogConfig) { } @@ -262,8 +261,8 @@ if (out_of_capacity != NULL) *out_of_capacity = false; // For testing. - if (forced_crash_directory_ != NULL) { - *crash_directory = FilePath(forced_crash_directory_); + if (!forced_crash_directory_.empty()) { + *crash_directory = forced_crash_directory_; return true; } @@ -498,7 +497,7 @@ const std::string &exec_name, const std::string &payload_path) { std::map<std::string, std::string> contents; - if (!ReadKeyValueFile(FilePath(std::string(lsb_release_)), '=', &contents)) { + if (!ReadKeyValueFile(FilePath(lsb_release_), '=', &contents)) { LOG(ERROR) << "Problem parsing " << lsb_release_; // Even though there was some failure, take as much as we could read. }
diff --git a/crash_collector.h b/crash_collector.h index f1cf32e..67ff870 100644 --- a/crash_collector.h +++ b/crash_collector.h
@@ -32,6 +32,7 @@ protected: friend class CrashCollectorTest; + FRIEND_TEST(ChromeCollectorTest, HandleCrash); FRIEND_TEST(CrashCollectorTest, CheckHasCapacityCorrectBasename); FRIEND_TEST(CrashCollectorTest, CheckHasCapacityStrangeNames); FRIEND_TEST(CrashCollectorTest, CheckHasCapacityUsual); @@ -71,7 +72,7 @@ // For testing, set the directory always returned by // GetCreatedCrashDirectoryByEuid. - void ForceCrashDirectory(const char *forced_directory) { + void ForceCrashDirectory(const base::FilePath &forced_directory) { forced_crash_directory_ = forced_directory; } @@ -86,7 +87,8 @@ bool GetUserInfoFromName(const std::string &name, uid_t *uid, gid_t *gid); - // Determines the crash directory for given eud, and creates the + + // Determines the crash directory for given euid, and creates the // directory if necessary with appropriate permissions. If // |out_of_capacity| is not NULL, it is set to indicate if the call // failed due to not having capacity in the crash directory. Returns @@ -165,8 +167,8 @@ CountCrashFunction count_crash_function_; IsFeedbackAllowedFunction is_feedback_allowed_function_; std::string extra_metadata_; - const char *forced_crash_directory_; - const char *lsb_release_; + base::FilePath forced_crash_directory_; + std::string lsb_release_; base::FilePath log_config_path_; };
diff --git a/crash_collector_test.cc b/crash_collector_test.cc index 7d3c1fe..863fed3 100644 --- a/crash_collector_test.cc +++ b/crash_collector_test.cc
@@ -34,8 +34,7 @@ class CrashCollectorTest : public ::testing::Test { public: void SetUp() { - collector_.Initialize(CountCrash, - IsMetrics); + collector_.Initialize(CountCrash, IsMetrics); test_dir_ = FilePath("test"); base::CreateDirectory(test_dir_); chromeos::ClearLog(); @@ -281,7 +280,7 @@ FilePath lsb_release = test_dir_.Append("lsb-release"); FilePath payload_file = test_dir_.Append("payload-file"); std::string contents; - collector_.lsb_release_ = lsb_release.value().c_str(); + collector_.lsb_release_ = lsb_release.value(); const char kLsbContents[] = "CHROMEOS_RELEASE_VERSION=version\n"; ASSERT_TRUE( file_util::WriteFile(lsb_release,
diff --git a/crash_reporter.cc b/crash_reporter.cc index 1da1d87..828ec91 100644 --- a/crash_reporter.cc +++ b/crash_reporter.cc
@@ -186,8 +186,8 @@ CHECK(!FLAGS_exe.empty()) << "--exe= must be set"; chromeos::LogToString(true); - bool handled = chrome_collector->HandleCrash(FLAGS_chrome, FLAGS_pid, - FLAGS_uid, FLAGS_exe); + bool handled = chrome_collector->HandleCrash(FilePath(FLAGS_chrome), + FLAGS_pid, FLAGS_uid, FLAGS_exe); chromeos::LogToString(false); if (!handled) return 1; @@ -269,8 +269,7 @@ ::g_type_init(); KernelCollector kernel_collector; - kernel_collector.Initialize(CountKernelCrash, - IsFeedbackAllowed); + kernel_collector.Initialize(CountKernelCrash, IsFeedbackAllowed); UserCollector user_collector; user_collector.Initialize(CountUserCrash, my_path.value(),
diff --git a/kernel_collector_test.cc b/kernel_collector_test.cc index dd6e5ba..14b8357 100644 --- a/kernel_collector_test.cc +++ b/kernel_collector_test.cc
@@ -5,6 +5,7 @@ #include <unistd.h> #include "base/file_util.h" +#include "base/files/scoped_temp_dir.h" #include "base/strings/string_util.h" #include "base/strings/stringprintf.h" #include "chromeos/syslog_logging.h" @@ -15,9 +16,6 @@ static int s_crashes = 0; static bool s_metrics = false; -static const char kTestKCrash[] = "test/kcrash"; -static const char kTestCrashDirectory[] = "test/crash_directory"; - using base::FilePath; using base::StringPrintf; using chromeos::FindLog; @@ -32,24 +30,6 @@ } class KernelCollectorTest : public ::testing::Test { - void SetUp() { - s_crashes = 0; - s_metrics = true; - collector_.Initialize(CountCrash, - IsMetrics); - mkdir("test", 0777); - mkdir(kTestKCrash, 0777); - test_kcrash_ = FilePath(kTestKCrash); - collector_.OverridePreservedDumpPath(test_kcrash_); - test_kcrash_ = test_kcrash_.Append("dmesg-ramoops-0"); - unlink(test_kcrash_.value().c_str()); - if (mkdir(kTestCrashDirectory, 0777)) { - ASSERT_EQ(EEXIST, errno) - << "Error while creating directory '" << kTestCrashDirectory - << "': " << strerror(errno); - } - chromeos::ClearLog(); - } protected: void WriteStringToFile(const FilePath &file_path, const char *data) { @@ -60,8 +40,32 @@ void SetUpSuccessfulCollect(); void ComputeKernelStackSignatureCommon(); + const FilePath &kcrash_file() const { return test_kcrash_; } + const FilePath &test_crash_directory() const { return test_crash_directory_; } + KernelCollector collector_; + + private: + void SetUp() OVERRIDE { + s_crashes = 0; + s_metrics = true; + collector_.Initialize(CountCrash, IsMetrics); + ASSERT_TRUE(scoped_temp_dir_.CreateUniqueTempDir()); + test_kcrash_ = scoped_temp_dir_.path().Append("kcrash"); + ASSERT_TRUE(base::CreateDirectory(test_kcrash_)); + collector_.OverridePreservedDumpPath(test_kcrash_); + + test_kcrash_ = test_kcrash_.Append("dmesg-ramoops-0"); + ASSERT_FALSE(base::PathExists(test_kcrash_)); + + test_crash_directory_ = scoped_temp_dir_.path().Append("crash_directory"); + ASSERT_TRUE(base::CreateDirectory(test_crash_directory_)); + chromeos::ClearLog(); + } + FilePath test_kcrash_; + FilePath test_crash_directory_; + base::ScopedTempDir scoped_temp_dir_; }; TEST_F(KernelCollectorTest, ComputeKernelStackSignatureBase) { @@ -70,16 +74,16 @@ } TEST_F(KernelCollectorTest, LoadPreservedDump) { - ASSERT_FALSE(base::PathExists(test_kcrash_)); + ASSERT_FALSE(base::PathExists(kcrash_file())); std::string dump; dump.clear(); - WriteStringToFile(test_kcrash_, "emptydata"); + WriteStringToFile(kcrash_file(), "emptydata"); ASSERT_TRUE(collector_.LoadParameters()); ASSERT_FALSE(collector_.LoadPreservedDump(&dump)); ASSERT_EQ("", dump); - WriteStringToFile(test_kcrash_, "====1.1\nsomething"); + WriteStringToFile(kcrash_file(), "====1.1\nsomething"); ASSERT_TRUE(collector_.LoadParameters()); ASSERT_TRUE(collector_.LoadPreservedDump(&dump)); ASSERT_EQ("something", dump); @@ -94,7 +98,7 @@ } TEST_F(KernelCollectorTest, EnableOK) { - WriteStringToFile(test_kcrash_, ""); + WriteStringToFile(kcrash_file(), ""); ASSERT_TRUE(collector_.Enable()); ASSERT_TRUE(collector_.IsEnabled()); ASSERT_TRUE(FindLog("Enabling kernel crash handling")); @@ -225,7 +229,7 @@ } TEST_F(KernelCollectorTest, CollectNoCrash) { - WriteStringToFile(test_kcrash_, ""); + WriteStringToFile(kcrash_file(), ""); ASSERT_FALSE(collector_.Collect()); ASSERT_TRUE(FindLog("No valid records found")); ASSERT_FALSE(FindLog("Stored kcrash to ")); @@ -233,7 +237,7 @@ } TEST_F(KernelCollectorTest, CollectBadDirectory) { - WriteStringToFile(test_kcrash_, "====1.1\nsomething"); + WriteStringToFile(kcrash_file(), "====1.1\nsomething"); ASSERT_TRUE(collector_.Collect()); ASSERT_TRUE(FindLog("Unable to create appropriate crash directory")) << "Did not find expected error string in log: {\n" @@ -242,8 +246,8 @@ } void KernelCollectorTest::SetUpSuccessfulCollect() { - collector_.ForceCrashDirectory(kTestCrashDirectory); - WriteStringToFile(test_kcrash_, "====1.1\nsomething"); + collector_.ForceCrashDirectory(test_crash_directory()); + WriteStringToFile(kcrash_file(), "====1.1\nsomething"); ASSERT_EQ(0, s_crashes); } @@ -272,7 +276,7 @@ size_t end_pos = filename.find_first_of("\n"); ASSERT_NE(std::string::npos, end_pos); filename = filename.substr(0, end_pos); - ASSERT_EQ(0, filename.find(kTestCrashDirectory)); + ASSERT_EQ(0, filename.find(test_crash_directory().value())); ASSERT_TRUE(base::PathExists(FilePath(filename))); std::string contents; ASSERT_TRUE(base::ReadFileToString(FilePath(filename), &contents));
diff --git a/udev_collector_test.cc b/udev_collector_test.cc index 6a5c856..db1550f 100644 --- a/udev_collector_test.cc +++ b/udev_collector_test.cc
@@ -5,7 +5,6 @@ #include "base/file_util.h" #include "base/files/file_enumerator.h" #include "base/files/scoped_temp_dir.h" -#include "base/memory/scoped_ptr.h" #include "chromeos/test_helpers.h" #include "crash-reporter/udev_collector.h" #include "gtest/gtest.h" @@ -47,21 +46,25 @@ } // namespace class UdevCollectorTest : public ::testing::Test { - void SetUp() { + protected: + base::ScopedTempDir temp_dir_generator_; + + void HandleCrash(const std::string &udev_event) { + collector_.HandleCrash(udev_event); + } + + private: + void SetUp() OVERRIDE { s_consent_given = true; - collector_.reset(new UdevCollector); - collector_->Initialize(CountCrash, IsMetrics); + collector_.Initialize(CountCrash, IsMetrics); - temp_dir_generator_.reset(new base::ScopedTempDir()); - ASSERT_TRUE(temp_dir_generator_->CreateUniqueTempDir()); - EXPECT_TRUE(temp_dir_generator_->IsValid()); + ASSERT_TRUE(temp_dir_generator_.CreateUniqueTempDir()); FilePath log_config_path = - temp_dir_generator_->path().Append(kLogConfigFileName); - collector_->log_config_path_ = log_config_path; - collector_->ForceCrashDirectory( - temp_dir_generator_->path().value().c_str()); + temp_dir_generator_.path().Append(kLogConfigFileName); + collector_.log_config_path_ = log_config_path; + collector_.ForceCrashDirectory(temp_dir_generator_.path()); // Write to a dummy log config file. ASSERT_EQ(strlen(kLogConfigFileContents), @@ -72,30 +75,28 @@ chromeos::ClearLog(); } - protected: - scoped_ptr<UdevCollector> collector_; - scoped_ptr<base::ScopedTempDir> temp_dir_generator_; + UdevCollector collector_; }; TEST_F(UdevCollectorTest, TestNoConsent) { s_consent_given = false; - collector_->HandleCrash("ACTION=change:KERNEL=card0:SUBSYSTEM=drm"); - EXPECT_EQ(0, GetNumLogFiles(temp_dir_generator_->path())); + HandleCrash("ACTION=change:KERNEL=card0:SUBSYSTEM=drm"); + EXPECT_EQ(0, GetNumLogFiles(temp_dir_generator_.path())); } TEST_F(UdevCollectorTest, TestNoMatch) { // No rule should match this. - collector_->HandleCrash("ACTION=change:KERNEL=foo:SUBSYSTEM=bar"); - EXPECT_EQ(0, GetNumLogFiles(temp_dir_generator_->path())); + HandleCrash("ACTION=change:KERNEL=foo:SUBSYSTEM=bar"); + EXPECT_EQ(0, GetNumLogFiles(temp_dir_generator_.path())); } TEST_F(UdevCollectorTest, TestMatches) { // Try multiple udev events in sequence. The number of log files generated // should increase. - collector_->HandleCrash("ACTION=change:KERNEL=card0:SUBSYSTEM=drm"); - EXPECT_EQ(1, GetNumLogFiles(temp_dir_generator_->path())); - collector_->HandleCrash("ACTION=add:KERNEL=state0:SUBSYSTEM=cpu"); - EXPECT_EQ(2, GetNumLogFiles(temp_dir_generator_->path())); + HandleCrash("ACTION=change:KERNEL=card0:SUBSYSTEM=drm"); + EXPECT_EQ(1, GetNumLogFiles(temp_dir_generator_.path())); + HandleCrash("ACTION=add:KERNEL=state0:SUBSYSTEM=cpu"); + EXPECT_EQ(2, GetNumLogFiles(temp_dir_generator_.path())); } // TODO(sque, crosbug.com/32238) - test wildcard cases, multiple identical udev
diff --git a/user_collector_test.cc b/user_collector_test.cc index eed6daf..0dd35e0 100644 --- a/user_collector_test.cc +++ b/user_collector_test.cc
@@ -52,10 +52,9 @@ protected: void ExpectFileEquals(const char *golden, - const char *file_path) { + const FilePath &file_path) { std::string contents; - EXPECT_TRUE(base::ReadFileToString(FilePath(file_path), - &contents)); + EXPECT_TRUE(base::ReadFileToString(file_path, &contents)); EXPECT_EQ(golden, contents); } @@ -71,8 +70,9 @@ TEST_F(UserCollectorTest, EnableOK) { ASSERT_TRUE(collector_.Enable()); - ExpectFileEquals("|/my/path --user=%P:%s:%u:%e", "test/core_pattern"); - ExpectFileEquals("4", "test/core_pipe_limit"); + ExpectFileEquals("|/my/path --user=%P:%s:%u:%e", + FilePath("test/core_pattern")); + ExpectFileEquals("4", FilePath("test/core_pipe_limit")); ASSERT_EQ(s_crashes, 0); EXPECT_TRUE(FindLog("Enabling user crash handling")); } @@ -98,7 +98,7 @@ TEST_F(UserCollectorTest, DisableOK) { ASSERT_TRUE(collector_.Disable()); - ExpectFileEquals("core", "test/core_pattern"); + ExpectFileEquals("core", FilePath("test/core_pattern")); ASSERT_EQ(s_crashes, 0); EXPECT_TRUE(FindLog("Disabling user crash handling")); }