crash-reporter: Attach Chrome logs to crash reports.
When Chrome crashes, gather the last 20 lines of each of the
latest two log files in /var/log/chrome and
/home/chronos/user/log. Attach them as gzipped metadata
using a "chrome.txt" key name and rename the GPU error state
key to "i915_error_state.log.xz".
(Re-landing after privacy review; originally reviewed at
https://chromium-review.googlesource.com/216427. Changes
since previous iteration include attaching the Chrome logs
and the GPU state separately and redirecting stderr to
/dev/null when listing user Chrome log files to avoid log
spam in the not-logged-in case.)
BUG=chromium:405732
TEST=triggered crashes and verified that logs were included
in crash reports: b7c4d7f2bee32e2f, 6292f090468fcf28
CQ-DEPEND=CL:242492
Change-Id: I1642a8971f1373726e5b0e3977dbfdbcc2aa6667
Reviewed-on: https://chromium-review.googlesource.com/242457
Commit-Queue: Dan Erat <derat@chromium.org>
Trybot-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>
diff --git a/crash_reporter/chrome_collector.cc b/crash_reporter/chrome_collector.cc
index 7138407..8dd56fb 100644
--- a/crash_reporter/chrome_collector.cc
+++ b/crash_reporter/chrome_collector.cc
@@ -20,16 +20,23 @@
#include <chromeos/process.h>
#include <chromeos/syslog_logging.h>
-const char kDefaultMinidumpName[] = "upload_file_minidump";
-const char kTarPath[] = "/bin/tar";
-// From //net/crash/collector/collector.h
-const int kDefaultMaxUploadBytes = 1024 * 1024;
-
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.
@@ -42,8 +49,25 @@
return true;
}
-bool GetDriErrorState(const chromeos::dbus::Proxy &proxy,
- const FilePath &error_state_path) {
+// 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) {
+ chromeos::dbus::BusConnection dbus = chromeos::dbus::GetSystemBusConnection();
+ if (!dbus.HasConnection()) {
+ LOG(ERROR) << "Error connecting to system D-Bus";
+ return false;
+ }
+
+ chromeos::dbus::Proxy proxy(dbus,
+ debugd::kDebugdServiceName,
+ debugd::kDebugdServicePath,
+ debugd::kDebugdInterface);
+ if (!proxy) {
+ LOG(ERROR) << "Error creating D-Bus proxy to interface "
+ << "'" << debugd::kDebugdServiceName << "'";
+ return false;
+ }
+
chromeos::glib::ScopedError error;
gchar *error_state = nullptr;
if (!dbus_g_proxy_call(proxy.gproxy(), debugd::kGetLog,
@@ -88,44 +112,19 @@
return true;
}
-bool GetAdditionalLogs(const FilePath &log_path) {
- chromeos::dbus::BusConnection dbus = chromeos::dbus::GetSystemBusConnection();
- if (!dbus.HasConnection()) {
- LOG(ERROR) << "Error connecting to system D-Bus";
- return false;
+// 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();
}
-
- chromeos::dbus::Proxy proxy(dbus,
- debugd::kDebugdServiceName,
- debugd::kDebugdServicePath,
- debugd::kDebugdInterface);
- if (!proxy) {
- LOG(ERROR) << "Error creating D-Bus proxy to interface "
- << "'" << debugd::kDebugdServiceName << "'";
- return false;
- }
-
- FilePath error_state_path =
- log_path.DirName().Append("i915_error_state.log.xz");
- if (!GetDriErrorState(proxy, error_state_path))
- return false;
-
- chromeos::ProcessImpl tar_process;
- tar_process.AddArg(kTarPath);
- tar_process.AddArg("cfJ");
- tar_process.AddArg(log_path.value());
- tar_process.AddStringOption("-C", log_path.DirName().value());
- tar_process.AddArg(error_state_path.BaseName().value());
- int res = tar_process.Run();
-
- base::DeleteFile(error_state_path, false);
-
- if (res || !base::PathExists(log_path)) {
- LOG(ERROR) << "Could not tar file " << log_path.value();
- return false;
- }
-
- return true;
+ return path.AddExtension(".gz");
}
} // namespace
@@ -161,7 +160,6 @@
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");
- FilePath log_path = GetCrashPath(dir, dump_basename, "log.tar.xz");
std::string data;
if (!base::ReadFileToString(file_path, &data)) {
@@ -174,19 +172,31 @@
return false;
}
- if (GetAdditionalLogs(log_path)) {
- int64_t minidump_size = 0;
- int64_t log_size = 0;
- if (base::GetFileSize(minidump_path, &minidump_size) &&
- base::GetFileSize(log_path, &log_size) &&
- minidump_size > 0 && log_size > 0 &&
- minidump_size + log_size < kDefaultMaxUploadBytes) {
- AddCrashMetaData("log", log_path.value());
- } else {
- LOG(INFO) << "Skipping logs upload to prevent discarding minidump "
- "because of report size limit < " << minidump_size + log_size;
- base::DeleteFile(log_path, false);
+
+ int64_t report_size = 0;
+ base::GetFileSize(minidump_path, &report_size);
+
+ // Keyed by crash metadata key name.
+ const std::map<std::string, base::FilePath> 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.
@@ -298,5 +308,31 @@
return at == data.size();
}
+std::map<std::string, base::FilePath> ChromeCollector::GetAdditionalLogs(
+ const FilePath &dir,
+ const std::string &basename,
+ const std::string &exe_name) {
+ std::map<std::string, base::FilePath> 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 */);
+ }
+
+ // Now get the GPU state from debugd.
+ const FilePath dri_error_state_path =
+ GetCrashPath(dir, basename, kGpuStateFilename);
+ if (GetDriErrorState(dri_error_state_path))
+ 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
index d8602b9..566b5a8 100644
--- a/crash_reporter/chrome_collector.h
+++ b/crash_reporter/chrome_collector.h
@@ -48,6 +48,14 @@
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<std::string, base::FilePath> GetAdditionalLogs(
+ const base::FilePath &dir,
+ const std::string &basename,
+ const std::string &exe_name);
+
FILE *output_file_ptr_;
DISALLOW_COPY_AND_ASSIGN(ChromeCollector);
diff --git a/crash_reporter/crash_reporter_logs.conf b/crash_reporter/crash_reporter_logs.conf
index e4ad207..3440770 100644
--- a/crash_reporter/crash_reporter_logs.conf
+++ b/crash_reporter/crash_reporter_logs.conf
@@ -20,6 +20,10 @@
# so it is handled in the same way as update_engine.
cros_installer:cat $(ls -1tr /var/log/update_engine | tail -5 | sed s.^./var/log/update_engine/.) | tail -c 50000
+# Dump the last 20 lines of the last two files in Chrome's system and user log
+# directories.
+chrome:for f in $(ls -1rt /var/log/chrome/chrome_[0-9]* | tail -2) $(ls -1rt /home/chronos/u-*/log/chrome_[0-9]* 2>/dev/null | tail -2); do echo "===$f (tail)==="; tail -20 $f; echo EOF; done
+
# The following rule is used for generating additional diagnostics when
# collection of user crashes fails. This output should not be too large
# as it is stored in memory. The output format specified for 'ps' is the
