From c8b741428caeb01f7aca3c4c199e5f6894d335f8 Mon Sep 17 00:00:00 2001 From: Steve Fung Date: Wed, 5 Aug 2015 15:45:20 -0700 Subject: [PATCH] crash_reporter: Remove Chrome collector, CrOS build files, DBus Removed the ChromeOS specific code so that the code compiles. The code removed is not needed when running on Android, and includes: * Chrome collector * ChromeOS build's gyp file * ChromeOS-specific DBus calls * ChromeOS-specific collector path logic * Chrome bypass logic in user collector Bug: 22672752 Change-Id: I3c7d87c971a181d8f73293519318e3602d142927 --- crash_reporter/chrome_collector.cc | 335 ------------------------ crash_reporter/chrome_collector.h | 72 ----- crash_reporter/chrome_collector_test.cc | 150 ----------- crash_reporter/crash-reporter.gyp | 147 ----------- crash_reporter/crash_collector.cc | 80 +----- crash_reporter/crash_collector.h | 20 -- crash_reporter/crash_collector_test.cc | 14 +- crash_reporter/crash_reporter.cc | 37 --- crash_reporter/user_collector.cc | 94 ------- crash_reporter/user_collector.h | 2 - crash_reporter/user_collector_test.cc | 69 +---- 11 files changed, 7 insertions(+), 1013 deletions(-) delete mode 100644 crash_reporter/chrome_collector.cc delete mode 100644 crash_reporter/chrome_collector.h delete mode 100644 crash_reporter/chrome_collector_test.cc delete mode 100644 crash_reporter/crash-reporter.gyp diff --git a/crash_reporter/chrome_collector.cc b/crash_reporter/chrome_collector.cc deleted file mode 100644 index ac44e4b36..000000000 --- a/crash_reporter/chrome_collector.cc +++ /dev/null @@ -1,335 +0,0 @@ -// Copyright (c) 2013 The Chromium OS Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "chrome_collector.h" - -#include -#include - -#include -#include -#include - -#include -#include -#include -#include -#include -#include -#include -#include - -using base::FilePath; -using base::StringPrintf; - -namespace { - -const char kDefaultMinidumpName[] = "upload_file_minidump"; - -// Path to the gzip binary. -const char kGzipPath[] = "/bin/gzip"; - -// Filenames for logs attached to crash reports. Also used as metadata keys. -const char kChromeLogFilename[] = "chrome.txt"; -const char kGpuStateFilename[] = "i915_error_state.log.xz"; - -// From //net/crash/collector/collector.h -const int kDefaultMaxUploadBytes = 1024 * 1024; - -// Extract a string delimited by the given character, from the given offset -// into a source string. Returns false if the string is zero-sized or no -// delimiter was found. -bool GetDelimitedString(const std::string &str, char ch, size_t offset, - std::string *substr) { - size_t at = str.find_first_of(ch, offset); - if (at == std::string::npos || at == offset) - return false; - *substr = str.substr(offset, at - offset); - return true; -} - -// Gets the GPU's error state from debugd and writes it to |error_state_path|. -// Returns true on success. -bool GetDriErrorState(const FilePath &error_state_path, - org::chromium::debugdProxy *proxy) { - chromeos::ErrorPtr error; - std::string error_state_str; - - proxy->GetLog("i915_error_state", &error_state_str, &error); - - if (error) { - LOG(ERROR) << "Error calling D-Bus proxy call to interface " - << "'" << proxy->GetObjectPath().value() << "':" - << error->GetMessage(); - return false; - } - - if (error_state_str == "") - return false; - - const char kBase64Header[] = ": "; - const size_t kBase64HeaderLength = sizeof(kBase64Header) - 1; - if (error_state_str.compare(0, kBase64HeaderLength, kBase64Header)) { - LOG(ERROR) << "i915_error_state is missing base64 header"; - return false; - } - - std::string decoded_error_state; - - if (!chromeos::data_encoding::Base64Decode( - error_state_str.c_str() + kBase64HeaderLength, - &decoded_error_state)) { - LOG(ERROR) << "Could not decode i915_error_state"; - return false; - } - - int written = base::WriteFile(error_state_path, - decoded_error_state.c_str(), - decoded_error_state.length()); - if (written < 0 || - static_cast(written) != decoded_error_state.length()) { - LOG(ERROR) << "Could not write file " << error_state_path.value() - << " Written: " << written << " Len: " - << decoded_error_state.length(); - base::DeleteFile(error_state_path, false); - return false; - } - - return true; -} - -// Gzip-compresses |path|, removes the original file, and returns the path of -// the new file. On failure, the original file is left alone and an empty path -// is returned. -FilePath GzipFile(const FilePath& path) { - chromeos::ProcessImpl proc; - proc.AddArg(kGzipPath); - proc.AddArg(path.value()); - const int res = proc.Run(); - if (res != 0) { - LOG(ERROR) << "Failed to gzip " << path.value(); - return FilePath(); - } - return path.AddExtension(".gz"); -} - -} // namespace - - -ChromeCollector::ChromeCollector() : output_file_ptr_(stdout) {} - -ChromeCollector::~ChromeCollector() {} - -bool ChromeCollector::HandleCrash(const FilePath &file_path, - const std::string &pid_string, - const std::string &uid_string, - const std::string &exe_name) { - if (!is_feedback_allowed_function_()) - return true; - - LOG(WARNING) << "Received crash notification for " << exe_name << "[" - << pid_string << "] user " << uid_string << " (called directly)"; - - if (exe_name.find('/') != std::string::npos) { - LOG(ERROR) << "exe_name contains illegal characters: " << exe_name; - return false; - } - - FilePath dir; - uid_t uid = atoi(uid_string.c_str()); - pid_t pid = atoi(pid_string.c_str()); - if (!GetCreatedCrashDirectoryByEuid(uid, &dir, nullptr)) { - LOG(ERROR) << "Can't create crash directory for uid " << uid; - return false; - } - - std::string dump_basename = FormatDumpBasename(exe_name, time(nullptr), pid); - FilePath meta_path = GetCrashPath(dir, dump_basename, "meta"); - FilePath minidump_path = GetCrashPath(dir, dump_basename, "dmp"); - - std::string data; - if (!base::ReadFileToString(file_path, &data)) { - LOG(ERROR) << "Can't read crash log: " << file_path.value(); - return false; - } - - if (!ParseCrashLog(data, dir, minidump_path, dump_basename)) { - LOG(ERROR) << "Failed to parse Chrome's crash log"; - return false; - } - - - int64_t report_size = 0; - base::GetFileSize(minidump_path, &report_size); - - // Keyed by crash metadata key name. - const std::map additional_logs = - GetAdditionalLogs(dir, dump_basename, exe_name); - for (auto it : additional_logs) { - int64_t file_size = 0; - if (!base::GetFileSize(it.second, &file_size)) { - PLOG(WARNING) << "Unable to get size of " << it.second.value(); - continue; - } - if (report_size + file_size > kDefaultMaxUploadBytes) { - LOG(INFO) << "Skipping upload of " << it.second.value() << "(" - << file_size << "B) because report size would exceed limit (" - << kDefaultMaxUploadBytes << "B)"; - continue; - } - VLOG(1) << "Adding metadata: " << it.first << " -> " << it.second.value(); - // Call AddCrashMetaUploadFile() rather than AddCrashMetaData() here. The - // former adds a prefix to the key name; without the prefix, only the key - // "logs" appears to be displayed on the crash server. - AddCrashMetaUploadFile(it.first, it.second.value()); - report_size += file_size; - } - - // We're done. - WriteCrashMetaData(meta_path, exe_name, minidump_path.value()); - - fprintf(output_file_ptr_, "%s", kSuccessMagic); - fflush(output_file_ptr_); - - return true; -} - -void ChromeCollector::SetUpDBus() { - CrashCollector::SetUpDBus(); - - debugd_proxy_.reset( - new org::chromium::debugdProxy(bus_, debugd::kDebugdServiceName)); -} - -bool ChromeCollector::ParseCrashLog(const std::string &data, - const FilePath &dir, - const FilePath &minidump, - const std::string &basename) { - size_t at = 0; - while (at < data.size()) { - // Look for a : followed by a decimal number, followed by another : - // followed by N bytes of data. - std::string name, size_string; - if (!GetDelimitedString(data, ':', at, &name)) { - LOG(ERROR) << "Can't find : after name @ offset " << at; - break; - } - at += name.size() + 1; // Skip the name & : delimiter. - - if (!GetDelimitedString(data, ':', at, &size_string)) { - LOG(ERROR) << "Can't find : after size @ offset " << at; - break; - } - at += size_string.size() + 1; // Skip the size & : delimiter. - - size_t size; - if (!base::StringToSizeT(size_string, &size)) { - LOG(ERROR) << "String not convertible to integer: " << size_string; - break; - } - - // Data would run past the end, did we get a truncated file? - if (at + size > data.size()) { - LOG(ERROR) << "Overrun, expected " << size << " bytes of data, got " - << (data.size() - at); - break; - } - - if (name.find("filename") != std::string::npos) { - // File. - // Name will be in a semi-MIME format of - // "; filename="" - // Descriptive name will be upload_file_minidump for the dump. - std::string desc, filename; - pcrecpp::RE re("(.*)\" *; *filename=\"(.*)\""); - if (!re.FullMatch(name.c_str(), &desc, &filename)) { - LOG(ERROR) << "Filename was not in expected format: " << name; - break; - } - - if (desc.compare(kDefaultMinidumpName) == 0) { - // The minidump. - WriteNewFile(minidump, data.c_str() + at, size); - } else { - // Some other file. - FilePath path = GetCrashPath(dir, basename + "-" + filename, "other"); - if (WriteNewFile(path, data.c_str() + at, size) >= 0) { - AddCrashMetaUploadFile(desc, path.value()); - } - } - } else { - // Other attribute. - std::string value_str; - value_str.reserve(size); - - // Since metadata is one line/value the values must be escaped properly. - for (size_t i = at; i < at + size; i++) { - switch (data[i]) { - case '"': - case '\\': - value_str.push_back('\\'); - value_str.push_back(data[i]); - break; - - case '\r': - value_str += "\\r"; - break; - - case '\n': - value_str += "\\n"; - break; - - case '\t': - value_str += "\\t"; - break; - - case '\0': - value_str += "\\0"; - break; - - default: - value_str.push_back(data[i]); - break; - } - } - AddCrashMetaUploadData(name, value_str); - } - - at += size; - } - - return at == data.size(); -} - -std::map ChromeCollector::GetAdditionalLogs( - const FilePath &dir, - const std::string &basename, - const std::string &exe_name) { - std::map logs; - - // Run the command specified by the config file to gather logs. - const FilePath chrome_log_path = - GetCrashPath(dir, basename, kChromeLogFilename); - if (GetLogContents(log_config_path_, exe_name, chrome_log_path)) { - const FilePath compressed_path = GzipFile(chrome_log_path); - if (!compressed_path.empty()) - logs[kChromeLogFilename] = compressed_path; - else - base::DeleteFile(chrome_log_path, false /* recursive */); - } - - // For unit testing, debugd_proxy_ isn't initialized, so skip attempting to - // get the GPU error state from debugd. - if (debugd_proxy_) { - const FilePath dri_error_state_path = - GetCrashPath(dir, basename, kGpuStateFilename); - if (GetDriErrorState(dri_error_state_path, debugd_proxy_.get())) - logs[kGpuStateFilename] = dri_error_state_path; - } - - return logs; -} - -// static -const char ChromeCollector::kSuccessMagic[] = "_sys_cr_finished"; diff --git a/crash_reporter/chrome_collector.h b/crash_reporter/chrome_collector.h deleted file mode 100644 index 43aaf2ba0..000000000 --- a/crash_reporter/chrome_collector.h +++ /dev/null @@ -1,72 +0,0 @@ -// Copyright (c) 2013 The Chromium OS Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef CRASH_REPORTER_CHROME_COLLECTOR_H_ -#define CRASH_REPORTER_CHROME_COLLECTOR_H_ - -#include -#include - -#include -#include -#include // for FRIEND_TEST - -#include "crash_collector.h" -#include "debugd/dbus-proxies.h" - -class SystemLogging; - -// Chrome crash collector. -class ChromeCollector : public CrashCollector { - public: - ChromeCollector(); - ~ChromeCollector() override; - - // 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 base::FilePath &file_path, - const std::string &pid_string, - const std::string &uid_string, - const std::string &exe_name); - - protected: - void SetUpDBus() override; - - private: - friend class ChromeCollectorTest; - FRIEND_TEST(ChromeCollectorTest, GoodValues); - 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: - // :: - // Length is encoded as a decimal number. It can be zero, but must consist of - // at least one character - // For file values, name actually contains both a description and a filename, - // in a fixed format of: "; filename="" - bool ParseCrashLog(const std::string &data, const base::FilePath &dir, - const base::FilePath &minidump, - const std::string &basename); - - // Writes additional logs for |exe_name| to files based on |basename| within - // |dir|. Crash report metadata key names and the corresponding file paths are - // returned. - std::map GetAdditionalLogs( - const base::FilePath &dir, - const std::string &basename, - const std::string &exe_name); - - FILE *output_file_ptr_; - - // D-Bus proxy for debugd interface. Unset in unit tests. - std::unique_ptr debugd_proxy_; - - DISALLOW_COPY_AND_ASSIGN(ChromeCollector); -}; - -#endif // CRASH_REPORTER_CHROME_COLLECTOR_H_ diff --git a/crash_reporter/chrome_collector_test.cc b/crash_reporter/chrome_collector_test.cc deleted file mode 100644 index f13883ae7..000000000 --- a/crash_reporter/chrome_collector_test.cc +++ /dev/null @@ -1,150 +0,0 @@ -// Copyright (c) 2013 The Chromium OS Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "chrome_collector.h" - -#include - -#include -#include -#include -#include -#include -#include - -using base::FilePath; - -namespace { - -const char kCrashFormatGood[] = "value1:10:abcdefghijvalue2:5:12345"; -const char kCrashFormatEmbeddedNewline[] = - "value1:10:abcd\r\nghijvalue2:5:12\n34"; -const char kCrashFormatBad1[] = "value1:10:abcdefghijvalue2:6=12345"; -const char kCrashFormatBad2[] = "value1:10:abcdefghijvalue2:512345"; -const char kCrashFormatBad3[] = "value1:10::abcdefghijvalue2:5=12345"; -const char kCrashFormatBad4[] = "value1:10:abcdefghijvalue2:4=12345"; - -const char kCrashFormatWithFile[] = - "value1:10:abcdefghijvalue2:5:12345" - "some_file\"; filename=\"foo.txt\":15:12345\n789\n12345" - "value3:2:ok"; - -void CountCrash() { -} - -bool s_allow_crash = false; - -bool IsMetrics() { - return s_allow_crash; -} - -} // namespace - -class ChromeCollectorMock : public ChromeCollector { - public: - MOCK_METHOD0(SetUpDBus, void()); -}; - -class ChromeCollectorTest : public ::testing::Test { - protected: - void ExpectFileEquals(const char *golden, - const FilePath &file_path) { - std::string contents; - EXPECT_TRUE(base::ReadFileToString(file_path, &contents)); - EXPECT_EQ(golden, contents); - } - - ChromeCollectorMock collector_; - - private: - void SetUp() override { - EXPECT_CALL(collector_, SetUpDBus()).WillRepeatedly(testing::Return()); - - collector_.Initialize(CountCrash, IsMetrics); - chromeos::ClearLog(); - } -}; - -TEST_F(ChromeCollectorTest, GoodValues) { - FilePath dir("."); - EXPECT_TRUE(collector_.ParseCrashLog(kCrashFormatGood, - dir, dir.Append("minidump.dmp"), - "base")); - - // Check to see if the values made it in properly. - std::string meta = collector_.extra_metadata_; - EXPECT_TRUE(meta.find("value1=abcdefghij") != std::string::npos); - EXPECT_TRUE(meta.find("value2=12345") != std::string::npos); -} - -TEST_F(ChromeCollectorTest, Newlines) { - FilePath dir("."); - EXPECT_TRUE(collector_.ParseCrashLog(kCrashFormatEmbeddedNewline, - dir, dir.Append("minidump.dmp"), - "base")); - - // Check to see if the values were escaped. - std::string meta = collector_.extra_metadata_; - EXPECT_TRUE(meta.find("value1=abcd\\r\\nghij") != std::string::npos); - EXPECT_TRUE(meta.find("value2=12\\n34") != std::string::npos); -} - -TEST_F(ChromeCollectorTest, BadValues) { - FilePath dir("."); - const struct { - const char *data; - } list[] = { - {kCrashFormatBad1, }, - {kCrashFormatBad2, }, - {kCrashFormatBad3, }, - {kCrashFormatBad4, }, - }; - - for (size_t i = 0; i < sizeof(list) / sizeof(list[0]); i++) { - chromeos::ClearLog(); - EXPECT_FALSE(collector_.ParseCrashLog(list[i].data, - dir, dir.Append("minidump.dmp"), - "base")); - } -} - -TEST_F(ChromeCollectorTest, File) { - 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")); - - // Check to see if the values are still correct and that the file was - // written with the right data. - std::string meta = collector_.extra_metadata_; - 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")); -} - -TEST_F(ChromeCollectorTest, HandleCrash) { - base::AutoReset 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), - base::WriteFile(dump_file, kCrashFormatWithFile, - strlen(kCrashFormatWithFile))); - collector_.ForceCrashDirectory(dir); - - FilePath log_file; - { - base::ScopedFILE output( - base::CreateAndOpenTemporaryFileInDir(dir, &log_file)); - ASSERT_TRUE(output.get()); - base::AutoReset 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); -} diff --git a/crash_reporter/crash-reporter.gyp b/crash_reporter/crash-reporter.gyp deleted file mode 100644 index a7f0e7ecc..000000000 --- a/crash_reporter/crash-reporter.gyp +++ /dev/null @@ -1,147 +0,0 @@ -{ - # Shouldn't need this, but doesn't work otherwise. - # http://crbug.com/340086 and http://crbug.com/385186 - # Note: the unused dependencies are optimized out by the compiler. - 'target_defaults': { - 'variables': { - 'deps': [ - 'libchromeos-<(libbase_ver)', - ], - }, - }, - 'targets': [ - { - 'target_name': 'libcrash', - 'type': 'static_library', - 'variables': { - 'exported_deps': [ - 'libchrome-<(libbase_ver)', - 'libpcrecpp', - ], - 'deps': ['<@(exported_deps)'], - }, - 'all_dependent_settings': { - 'variables': { - 'deps': [ - '<@(exported_deps)', - ], - }, - }, - 'sources': [ - 'chrome_collector.cc', - 'crash_collector.cc', - 'kernel_collector.cc', - 'kernel_warning_collector.cc', - 'udev_collector.cc', - 'unclean_shutdown_collector.cc', - 'user_collector.cc', - ], - 'actions': [ - { - 'action_name': 'generate-session-manager-proxies', - 'variables': { - 'proxy_output_file': 'include/session_manager/dbus-proxies.h' - }, - 'sources': [ - '../login_manager/org.chromium.SessionManagerInterface.xml', - ], - 'includes': ['../common-mk/generate-dbus-proxies.gypi'], - }, - { - 'action_name': 'generate-debugd-proxies', - 'variables': { - 'proxy_output_file': 'include/debugd/dbus-proxies.h' - }, - 'sources': [ - '../debugd/share/org.chromium.debugd.xml', - ], - 'includes': ['../common-mk/generate-dbus-proxies.gypi'], - }, - ], - }, - { - 'target_name': 'crash_reporter', - 'type': 'executable', - 'variables': { - 'deps': [ - 'dbus-1', - 'libmetrics-<(libbase_ver)', - ], - }, - 'dependencies': [ - 'libcrash', - ], - 'sources': [ - 'crash_reporter.cc', - ], - }, - { - 'target_name': 'list_proxies', - 'type': 'executable', - 'variables': { - 'deps': [ - 'dbus-1', - 'libchrome-<(libbase_ver)', - ], - }, - 'sources': [ - 'list_proxies.cc', - ], - 'actions': [ - { - 'action_name': 'generate-lib-cros-service-proxies', - 'variables': { - 'proxy_output_file': 'include/libcrosservice/dbus-proxies.h' - }, - 'sources': [ - './dbus_bindings/org.chromium.LibCrosService.xml', - ], - 'includes': ['../common-mk/generate-dbus-proxies.gypi'], - }, - ], - }, - { - 'target_name': 'warn_collector', - 'type': 'executable', - 'variables': { - 'lexer_out_dir': 'crash-reporter', - 'deps': [ - 'libmetrics-<(libbase_ver)', - ], - }, - 'link_settings': { - 'libraries': [ - '-lfl', - ], - }, - 'sources': [ - 'warn_collector.l', - ], - 'includes': ['../common-mk/lex.gypi'], - }, - ], - 'conditions': [ - ['USE_test == 1', { - 'targets': [ - { - 'target_name': 'crash_reporter_test', - 'type': 'executable', - 'includes': ['../common-mk/common_test.gypi'], - 'dependencies': ['libcrash'], - 'sources': [ - 'chrome_collector_test.cc', - 'crash_collector_test.cc', - 'crash_collector_test.h', - 'crash_reporter_logs_test.cc', - 'kernel_collector_test.cc', - 'kernel_collector_test.h', - 'testrunner.cc', - 'udev_collector_test.cc', - 'unclean_shutdown_collector_test.cc', - 'user_collector_test.cc', - ], - }, - ], - }], - ], -} diff --git a/crash_reporter/crash_collector.cc b/crash_reporter/crash_collector.cc index 29a2b7286..69fe9392f 100644 --- a/crash_reporter/crash_collector.cc +++ b/crash_reporter/crash_collector.cc @@ -23,8 +23,6 @@ #include #include #include -#include -#include #include #include @@ -87,8 +85,6 @@ CrashCollector::CrashCollector() } CrashCollector::~CrashCollector() { - if (bus_) - bus_->ShutdownAndBlock(); } void CrashCollector::Initialize( @@ -99,21 +95,6 @@ void CrashCollector::Initialize( count_crash_function_ = count_crash_function; is_feedback_allowed_function_ = is_feedback_allowed_function; - - SetUpDBus(); -} - -void CrashCollector::SetUpDBus() { - dbus::Bus::Options options; - options.bus_type = dbus::Bus::SYSTEM; - - bus_ = new dbus::Bus(options); - CHECK(bus_->Connect()); - - session_manager_proxy_.reset( - new org::chromium::SessionManagerInterfaceProxy( - bus_, - login_manager::kSessionManagerServiceName)); } int CrashCollector::WriteNewFile(const FilePath &filename, @@ -166,38 +147,6 @@ FilePath CrashCollector::GetCrashPath(const FilePath &crash_directory, extension.c_str())); } -bool CrashCollector::GetActiveUserSessions( - std::map *sessions) { - chromeos::ErrorPtr error; - session_manager_proxy_->RetrieveActiveSessions(sessions, &error); - - if (error) { - LOG(ERROR) << "Error calling D-Bus proxy call to interface " - << "'" << session_manager_proxy_->GetObjectPath().value() << "':" - << error->GetMessage(); - return false; - } - - return true; -} - -FilePath CrashCollector::GetUserCrashPath() { - // In this multiprofile world, there is no one-specific user dir anymore. - // Ask the session manager for the active ones, then just run with the - // first result we get back. - FilePath user_path = FilePath(kFallbackUserCrashPath); - std::map active_sessions; - if (!GetActiveUserSessions(&active_sessions) || active_sessions.empty()) { - LOG(ERROR) << "Could not get active user sessions, using default."; - return user_path; - } - - user_path = chromeos::cryptohome::home::GetHashedUserPath( - active_sessions.begin()->second).Append("crash"); - - return user_path; -} - FilePath CrashCollector::GetCrashDirectoryInfo( uid_t process_euid, uid_t default_user_id, @@ -205,17 +154,11 @@ FilePath CrashCollector::GetCrashDirectoryInfo( mode_t *mode, uid_t *directory_owner, gid_t *directory_group) { - // TODO(mkrebs): This can go away once Chrome crashes are handled - // normally (see crosbug.com/5872). - // Check if the user crash directory should be used. If we are - // collecting chrome crashes during autotesting, we want to put them in - // the system crash directory so they are outside the cryptohome -- in - // case we are being run during logout (see crosbug.com/18637). - if (process_euid == default_user_id && IsUserSpecificDirectoryEnabled()) { + if (process_euid == default_user_id) { *mode = kUserCrashPathMode; *directory_owner = default_user_id; *directory_group = default_user_group; - return GetUserCrashPath(); + return FilePath(kFallbackUserCrashPath); } else { *mode = kSystemCrashPathMode; *directory_owner = kRootOwner; @@ -491,22 +434,3 @@ bool CrashCollector::IsDeveloperImage() { return false; return base::PathExists(FilePath(kLeaveCoreFile)); } - -bool CrashCollector::ShouldHandleChromeCrashes() { - // If we're testing crash reporter itself, we don't want to allow an - // override for chrome crashes. And, let's be conservative and only - // allow an override for developer images. - if (!IsCrashTestInProgress() && IsDeveloperImage()) { - // Check if there's an override to indicate we should indeed collect - // chrome crashes. This allows the crashes to still be tracked when - // they occur in autotests. See "crosbug.com/17987". - if (base::PathExists(FilePath(kCollectChromeFile))) - return true; - } - // We default to ignoring chrome crashes. - return false; -} - -bool CrashCollector::IsUserSpecificDirectoryEnabled() { - return !ShouldHandleChromeCrashes(); -} diff --git a/crash_reporter/crash_collector.h b/crash_reporter/crash_collector.h index ef443d3e3..0d335cdda 100644 --- a/crash_reporter/crash_collector.h +++ b/crash_reporter/crash_collector.h @@ -12,11 +12,8 @@ #include #include -#include #include // for FRIEND_TEST -#include "session_manager/dbus-proxies.h" - // User crash collector. class CrashCollector { public: @@ -44,7 +41,6 @@ class CrashCollector { FRIEND_TEST(CrashCollectorTest, ForkExecAndPipe); FRIEND_TEST(CrashCollectorTest, FormatDumpBasename); FRIEND_TEST(CrashCollectorTest, Initialize); - FRIEND_TEST(CrashCollectorTest, IsUserSpecificDirectoryEnabled); FRIEND_TEST(CrashCollectorTest, MetaData); FRIEND_TEST(CrashCollectorTest, Sanitize); FRIEND_TEST(CrashCollectorTest, WriteNewFile); @@ -61,9 +57,6 @@ class CrashCollector { // Set maximum enqueued crashes in a crash directory. static const int kMaxCrashDirectorySize; - // Set up D-Bus. - virtual void SetUpDBus(); - // Writes |data| of |size| to |filename|, which must be a new file. // If the file already exists or writing fails, return a negative value. // Otherwise returns the number of bytes written. @@ -79,9 +72,6 @@ class CrashCollector { forced_crash_directory_ = forced_directory; } - virtual bool GetActiveUserSessions( - std::map *sessions); - base::FilePath GetUserCrashPath(); base::FilePath GetCrashDirectoryInfo(uid_t process_euid, uid_t default_user_id, gid_t default_user_group, @@ -154,10 +144,6 @@ class CrashCollector { // Returns true if we should consider ourselves to be running on a // developer image. bool IsDeveloperImage(); - // Returns true if chrome crashes should be handled. - bool ShouldHandleChromeCrashes(); - // Returns true if user crash directory may be used. - bool IsUserSpecificDirectoryEnabled(); CountCrashFunction count_crash_function_; IsFeedbackAllowedFunction is_feedback_allowed_function_; @@ -166,13 +152,7 @@ class CrashCollector { std::string lsb_release_; base::FilePath log_config_path_; - scoped_refptr bus_; - private: - // D-Bus proxy for session manager interface. - std::unique_ptr - session_manager_proxy_; - DISALLOW_COPY_AND_ASSIGN(CrashCollector); }; diff --git a/crash_reporter/crash_collector_test.cc b/crash_reporter/crash_collector_test.cc index ecb2ba659..13fb76a6d 100644 --- a/crash_reporter/crash_collector_test.cc +++ b/crash_reporter/crash_collector_test.cc @@ -32,13 +32,6 @@ bool IsMetrics() { return false; } -bool GetActiveUserSessionsImpl(std::map *sessions) { - char kUser[] = "chicken@butt.com"; - char kHash[] = "hashcakes"; - sessions->insert(std::pair(kUser, kHash)); - return true; -} - } // namespace class CrashCollectorTest : public ::testing::Test { @@ -126,18 +119,13 @@ TEST_F(CrashCollectorTest, GetCrashDirectoryInfo) { EXPECT_EQ(kRootUid, directory_owner); EXPECT_EQ(kRootGid, directory_group); - EXPECT_CALL(collector_, GetActiveUserSessions(testing::_)) - .WillOnce(Invoke(&GetActiveUserSessionsImpl)); - - EXPECT_EQ(collector_.IsUserSpecificDirectoryEnabled(), true); - path = collector_.GetCrashDirectoryInfo(kChronosUid, kChronosUid, kChronosGid, &directory_mode, &directory_owner, &directory_group); - EXPECT_EQ("/home/user/hashcakes/crash", path.value()); + EXPECT_EQ("/var/spool/crash", path.value()); EXPECT_EQ(kExpectedUserMode, directory_mode); EXPECT_EQ(kChronosUid, directory_owner); EXPECT_EQ(kChronosGid, directory_group); diff --git a/crash_reporter/crash_reporter.cc b/crash_reporter/crash_reporter.cc index 9fa0d07e1..0e45992e3 100644 --- a/crash_reporter/crash_reporter.cc +++ b/crash_reporter/crash_reporter.cc @@ -16,7 +16,6 @@ #include #include -#include "chrome_collector.h" #include "kernel_collector.h" #include "kernel_warning_collector.h" #include "udev_collector.h" @@ -97,12 +96,6 @@ static void CountUserCrash() { LOG_IF(WARNING, status != 0) << "dbus-send running failed"; } -static void CountChromeCrash() { - // For now, consider chrome crashes the same as user crashes for reporting - // purposes. - CountUserCrash(); -} - static int Initialize(KernelCollector *kernel_collector, UserCollector *user_collector, @@ -164,25 +157,6 @@ static int HandleUserCrash(UserCollector *user_collector, return 0; } -static int HandleChromeCrash(ChromeCollector *chrome_collector, - const std::string& chrome_dump_file, - const std::string& pid, - const std::string& uid, - const std::string& exe) { - CHECK(!chrome_dump_file.empty()) << "--chrome= must be set"; - CHECK(!pid.empty()) << "--pid= must be set"; - CHECK(!uid.empty()) << "--uid= must be set"; - CHECK(!exe.empty()) << "--exe= must be set"; - - chromeos::LogToString(true); - bool handled = chrome_collector->HandleCrash(FilePath(chrome_dump_file), - pid, uid, exe); - chromeos::LogToString(false); - if (!handled) - return 1; - return 0; -} - static int HandleUdevCrash(UdevCollector *udev_collector, const std::string& udev_event) { // Handle a crash indicated by a udev event. @@ -258,7 +232,6 @@ int main(int argc, char *argv[]) { DEFINE_bool(unclean_check, true, "Check for unclean shutdown"); DEFINE_string(udev, "", "Udev event description (type:device:subsystem)"); DEFINE_bool(kernel_warning, false, "Report collected kernel warning"); - DEFINE_string(chrome, "", "Chrome crash dump file"); DEFINE_string(pid, "", "PID of crashing process"); DEFINE_string(uid, "", "UID of crashing process"); DEFINE_string(exe, "", "Executable name of crashing process"); @@ -289,8 +262,6 @@ int main(int argc, char *argv[]) { IsFeedbackAllowed); UdevCollector udev_collector; udev_collector.Initialize(CountUdevCrash, IsFeedbackAllowed); - ChromeCollector chrome_collector; - chrome_collector.Initialize(CountChromeCrash, IsFeedbackAllowed); KernelWarningCollector kernel_warning_collector; kernel_warning_collector.Initialize(CountUdevCrash, IsFeedbackAllowed); @@ -322,13 +293,5 @@ int main(int argc, char *argv[]) { return HandleKernelWarning(&kernel_warning_collector); } - if (!FLAGS_chrome.empty()) { - return HandleChromeCrash(&chrome_collector, - FLAGS_chrome, - FLAGS_pid, - FLAGS_uid, - FLAGS_exe); - } - return HandleUserCrash(&user_collector, FLAGS_user, FLAGS_crash_test); } diff --git a/crash_reporter/user_collector.cc b/crash_reporter/user_collector.cc index 039718573..ac2f90ef2 100644 --- a/crash_reporter/user_collector.cc +++ b/crash_reporter/user_collector.cc @@ -13,14 +13,12 @@ #include #include // For getpwuid_r, getgrnam_r, WEXITSTATUS. -#include #include #include #include #include #include -#include #include #include #include @@ -495,101 +493,11 @@ bool UserCollector::ParseCrashAttributes(const std::string &crash_attributes, kernel_supplied_name); } -// Returns true if the given executable name matches that of Chrome. This -// includes checks for threads that Chrome has renamed. -static bool IsChromeExecName(const std::string &exec) { - static const char *kChromeNames[] = { - "chrome", - // These are additional thread names seen in http://crash/ - "MediaPipeline", - // These come from the use of base::PlatformThread::SetName() directly - "CrBrowserMain", "CrRendererMain", "CrUtilityMain", "CrPPAPIMain", - "CrPPAPIBrokerMain", "CrPluginMain", "CrWorkerMain", "CrGpuMain", - "BrokerEvent", "CrVideoRenderer", "CrShutdownDetector", - "UsbEventHandler", "CrNaClMain", "CrServiceMain", - // These thread names come from the use of base::Thread - "Gamepad polling thread", "Chrome_InProcGpuThread", - "Chrome_DragDropThread", "Renderer::FILE", "VC manager", - "VideoCaptureModuleImpl", "JavaBridge", "VideoCaptureManagerThread", - "Geolocation", "Geolocation_wifi_provider", - "Device orientation polling thread", "Chrome_InProcRendererThread", - "NetworkChangeNotifier", "Watchdog", "inotify_reader", - "cf_iexplore_background_thread", "BrowserWatchdog", - "Chrome_HistoryThread", "Chrome_SyncThread", "Chrome_ShellDialogThread", - "Printing_Worker", "Chrome_SafeBrowsingThread", "SimpleDBThread", - "D-Bus thread", "AudioThread", "NullAudioThread", "V4L2Thread", - "ChromotingClientDecodeThread", "Profiling_Flush", - "worker_thread_ticker", "AudioMixerAlsa", "AudioMixerCras", - "FakeAudioRecordingThread", "CaptureThread", - "Chrome_WebSocketproxyThread", "ProcessWatcherThread", - "Chrome_CameraThread", "import_thread", "NaCl_IOThread", - "Chrome_CloudPrintJobPrintThread", "Chrome_CloudPrintProxyCoreThread", - "DaemonControllerFileIO", "ChromotingMainThread", - "ChromotingEncodeThread", "ChromotingDesktopThread", - "ChromotingIOThread", "ChromotingFileIOThread", - "Chrome_libJingle_WorkerThread", "Chrome_ChildIOThread", - "GLHelperThread", "RemotingHostPlugin", - // "PAC thread #%d", // not easy to check because of "%d" - "Chrome_DBThread", "Chrome_WebKitThread", "Chrome_FileThread", - "Chrome_FileUserBlockingThread", "Chrome_ProcessLauncherThread", - "Chrome_CacheThread", "Chrome_IOThread", "Cache Thread", "File Thread", - "ServiceProcess_IO", "ServiceProcess_File", - "extension_crash_uploader", "gpu-process_crash_uploader", - "plugin_crash_uploader", "renderer_crash_uploader", - // These come from the use of webkit_glue::WebThreadImpl - "Compositor", "Browser Compositor", - // "WorkerPool/%d", // not easy to check because of "%d" - // These come from the use of base::Watchdog - "Startup watchdog thread Watchdog", "Shutdown watchdog thread Watchdog", - // These come from the use of AudioDeviceThread::Start - "AudioDevice", "AudioInputDevice", "AudioOutputDevice", - // These come from the use of MessageLoopFactory::GetMessageLoop - "GpuVideoDecoder", "RtcVideoDecoderThread", "PipelineThread", - "AudioDecoderThread", "VideoDecoderThread", - // These come from the use of MessageLoopFactory::GetMessageLoopProxy - "CaptureVideoDecoderThread", "CaptureVideoDecoder", - // These come from the use of base::SimpleThread - "LocalInputMonitor/%d", // "%d" gets lopped off for kernel-supplied - // These come from the use of base::DelegateSimpleThread - "ipc_channel_nacl reader thread/%d", "plugin_audio_input_thread/%d", - "plugin_audio_thread/%d", - // These come from the use of base::SequencedWorkerPool - "BrowserBlockingWorker%d/%d", // "%d" gets lopped off for kernel-supplied - }; - static std::set chrome_names; - - // Initialize a set of chrome names, for efficient lookup - if (chrome_names.empty()) { - for (size_t i = 0; i < arraysize(kChromeNames); i++) { - std::string check_name(kChromeNames[i]); - chrome_names.insert(check_name); - // When checking a kernel-supplied name, it should be truncated to 15 - // chars. See PR_SET_NAME in - // http://www.kernel.org/doc/man-pages/online/pages/man2/prctl.2.html, - // although that page misleads by saying "16 bytes". - chrome_names.insert("supplied_" + std::string(check_name, 0, 15)); - } - } - - return ContainsKey(chrome_names, exec); -} - bool UserCollector::ShouldDump(bool has_owner_consent, bool is_developer, - bool handle_chrome_crashes, - const std::string &exec, std::string *reason) { reason->clear(); - // Treat Chrome crashes as if the user opted-out. We stop counting Chrome - // crashes towards user crashes, so user crashes really mean non-Chrome - // user-space crashes. - if (!handle_chrome_crashes && IsChromeExecName(exec)) { - *reason = "ignoring call by kernel - chrome crash; " - "waiting for chrome to call us directly"; - return false; - } - // For developer builds, we always want to keep the crash reports unless // we're testing the crash facilities themselves. This overrides // feedback. Crash sending still obeys consent. @@ -646,8 +554,6 @@ bool UserCollector::HandleCrash(const std::string &crash_attributes, std::string reason; bool dump = ShouldDump(is_feedback_allowed_function_(), IsDeveloperImage(), - ShouldHandleChromeCrashes(), - exec, &reason); LOG(WARNING) << "Received crash notification for " << exec << "[" << pid diff --git a/crash_reporter/user_collector.h b/crash_reporter/user_collector.h index 77024e13e..7b356eed7 100644 --- a/crash_reporter/user_collector.h +++ b/crash_reporter/user_collector.h @@ -169,8 +169,6 @@ class UserCollector : public CrashCollector { bool ShouldDump(bool has_owner_consent, bool is_developer, - bool handle_chrome_crashes, - const std::string &exec, std::string *reason); bool generate_diagnostics_; diff --git a/crash_reporter/user_collector_test.cc b/crash_reporter/user_collector_test.cc index b80b93c81..a0ada0657 100644 --- a/crash_reporter/user_collector_test.cc +++ b/crash_reporter/user_collector_test.cc @@ -164,81 +164,20 @@ TEST_F(UserCollectorTest, ParseCrashAttributes) { TEST_F(UserCollectorTest, ShouldDumpDeveloperImageOverridesConsent) { std::string reason; - EXPECT_TRUE(collector_.ShouldDump(false, true, false, - "chrome-wm", &reason)); + EXPECT_TRUE(collector_.ShouldDump(false, true, &reason)); EXPECT_EQ("developer build - not testing - always dumping", reason); // When running a crash test, behave as normal. - EXPECT_FALSE(collector_.ShouldDump(false, false, false, - "chrome-wm", &reason)); + EXPECT_FALSE(collector_.ShouldDump(false, false, &reason)); EXPECT_EQ("ignoring - no consent", reason); } -TEST_F(UserCollectorTest, ShouldDumpChromeOverridesDeveloperImage) { - std::string reason; - // When running a crash test, behave as normal. - EXPECT_FALSE(collector_.ShouldDump(false, false, false, - "chrome", &reason)); - EXPECT_EQ(kChromeIgnoreMsg, reason); - EXPECT_FALSE(collector_.ShouldDump(false, false, false, - "supplied_Compositor", &reason)); - EXPECT_EQ(kChromeIgnoreMsg, reason); - EXPECT_FALSE(collector_.ShouldDump(false, false, false, - "supplied_PipelineThread", &reason)); - EXPECT_EQ(kChromeIgnoreMsg, reason); - EXPECT_FALSE(collector_.ShouldDump(false, false, false, - "Chrome_ChildIOThread", &reason)); - EXPECT_EQ(kChromeIgnoreMsg, reason); - EXPECT_FALSE(collector_.ShouldDump(false, false, false, - "supplied_Chrome_ChildIOT", &reason)); - EXPECT_EQ(kChromeIgnoreMsg, reason); - EXPECT_FALSE(collector_.ShouldDump(false, false, false, - "supplied_ChromotingClien", &reason)); - EXPECT_EQ(kChromeIgnoreMsg, reason); - EXPECT_FALSE(collector_.ShouldDump(false, false, false, - "supplied_LocalInputMonit", &reason)); - EXPECT_EQ(kChromeIgnoreMsg, reason); - - // When running a developer image, test that chrome crashes are handled - // when the "handle_chrome_crashes" flag is set. - EXPECT_TRUE(collector_.ShouldDump(false, true, true, - "chrome", &reason)); - EXPECT_EQ("developer build - not testing - always dumping", - reason); - EXPECT_TRUE(collector_.ShouldDump(false, true, true, - "supplied_Compositor", &reason)); - EXPECT_EQ("developer build - not testing - always dumping", - reason); - EXPECT_TRUE(collector_.ShouldDump(false, true, true, - "supplied_PipelineThread", &reason)); - EXPECT_EQ("developer build - not testing - always dumping", - reason); - EXPECT_TRUE(collector_.ShouldDump(false, true, true, - "Chrome_ChildIOThread", &reason)); - EXPECT_EQ("developer build - not testing - always dumping", - reason); - EXPECT_TRUE(collector_.ShouldDump(false, true, true, - "supplied_Chrome_ChildIOT", &reason)); - EXPECT_EQ("developer build - not testing - always dumping", - reason); - EXPECT_TRUE(collector_.ShouldDump(false, true, true, - "supplied_ChromotingClien", &reason)); - EXPECT_EQ("developer build - not testing - always dumping", - reason); - EXPECT_TRUE(collector_.ShouldDump(false, true, true, - "supplied_LocalInputMonit", &reason)); - EXPECT_EQ("developer build - not testing - always dumping", - reason); -} - TEST_F(UserCollectorTest, ShouldDumpUseConsentProductionImage) { std::string result; - EXPECT_FALSE(collector_.ShouldDump(false, false, false, - "chrome-wm", &result)); + EXPECT_FALSE(collector_.ShouldDump(false, false, &result)); EXPECT_EQ("ignoring - no consent", result); - EXPECT_TRUE(collector_.ShouldDump(true, false, false, - "chrome-wm", &result)); + EXPECT_TRUE(collector_.ShouldDump(true, false, &result)); EXPECT_EQ("handling", result); }