A collector for postmortem reports

components/browser_watcher/postmortem_report_collector.cc

Index: components/browser_watcher/postmortem_report_collector.cc
diff --git a/components/browser_watcher/postmortem_report_collector.cc b/components/browser_watcher/postmortem_report_collector.cc
new file mode 100644
index 0000000000000000000000000000000000000000..783abb626b11d00b41f5c8402fa043de7ee495be
--- /dev/null
+++ b/components/browser_watcher/postmortem_report_collector.cc
@@ -0,0 +1,181 @@
+// Copyright 2016 The Chromium 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 "components/browser_watcher/postmortem_report_collector.h"
+
+#include <utility>
+
+#include "base/debug/activity_analyzer.h"
+#include "base/files/file_enumerator.h"
+#include "base/files/file_util.h"
+#include "base/logging.h"
+#include "base/path_service.h"
+#include "components/browser_watcher/postmortem_minidump_writer.h"
+#include "components/version_info/version_info.h"
+#include "third_party/crashpad/crashpad/client/settings.h"
+#include "third_party/crashpad/crashpad/util/misc/uuid.h"
+
+using base::FilePath;
+
+namespace browser_watcher {
+
+using base::debug::GlobalActivityAnalyzer;
+using base::debug::ThreadActivityAnalyzer;
+using crashpad::CrashReportDatabase;
+
+// TODO(manzagop): throttling and graceful handling of too much data.

[Sigurður Ásgeirsson, 2016/09/15 18:44:10]

I think it makes sense for throttling to be centralized in the uploader?

[manzagop (departed), 2016/09/16 16:37:30]

I've rewritten the TODO to more clearly express the concern is about the
collection taking too long on a critical path (e.g. starting chrome). I've also
moved the TODO to the .h.

+int PostmortemReportCollector::CollectAndSubmitForUpload(
+    const base::FilePath& debug_info_dir,
+    const base::FilePath::StringType& debug_file_pattern,
+    const std::set<base::FilePath>& excluded_debug_files,
+    crashpad::CrashReportDatabase* report_database) {
+  DCHECK_NE(true, debug_info_dir.empty());
+  DCHECK_NE(true, debug_file_pattern.empty());
+  DCHECK_NE(nullptr, report_database);
+
+  // Collect the list of files to harvest.
+  std::vector<FilePath> debug_files = GetDebugStateFilePaths(
+      debug_info_dir, debug_file_pattern, excluded_debug_files);
+
+  // Determine the crashpad client id.
+  crashpad::UUID client_id;
+  crashpad::Settings* settings = report_database->GetSettings();
+  if (settings) {
+    // If GetSettings() or GetClientID() fails client_id will be left at its
+    // default value, all zeroes, which is appropriate.
+    settings->GetClientID(&client_id);
+  }
+
+  // The number of postmortem reports recovered irrespective of whether they
+  // were successfully wrapped in a minidump and registered with the reporter.
+  int postmortem_cnt = 0;
+
+  // Note: the code below involves two notions of report:
+  //   - crashpad reports
+  //   - chrome internal state reports, which are reported wrapped inside a
+  //     crashpad report
+  for (const FilePath& file : debug_files) {
+    // Collect the data from the debug file to a proto.
+    std::unique_ptr<StabilityReport> report_proto = Collect(file);

[Sigurður Ásgeirsson, 2016/09/15 18:44:10]

it would IMHO be much more readable to scoop the body of this loop into a
function.
Early return is somehow much more readable (in my opinion) than continue...

[manzagop (departed), 2016/09/16 16:37:30]

+1, and it's a nicely self contained unit. Done.

+
+    // The file was empty, or there was an error collecting the data. Detailed
+    // logging happens within the Collect function.
+    if (!report_proto) {
+      if (!base::DeleteFile(file, false))
+        LOG(ERROR) << "Failed to delete " << file.value();
+      continue;
+    }
+
+    ++postmortem_cnt;
+
+    // Prepare a crashpad report.
+    CrashReportDatabase::NewReport* new_report = nullptr;
+    CrashReportDatabase::OperationStatus database_status =
+        report_database->PrepareNewCrashReport(&new_report);
+    if (database_status != CrashReportDatabase::kNoError) {
+      LOG(ERROR) << "PrepareNewCrashReport failed";
+      continue;
+    }
+    CrashReportDatabase::CallErrorWritingCrashReport
+        call_error_writing_crash_report(report_database, new_report);
+
+    // Write the report to a minidump.
+    if (!WriteReportToMinidump(*report_proto, client_id, new_report->uuid,
+                               reinterpret_cast<FILE*>(new_report->handle))) {
+      continue;
+    }
+
+    // If the file cannot be deleted, do not report its contents. Note this can
+    // lead to under reporting and retries. However, under reporting is
+    // preferable to the over reporting that would happen with a file that
+    // cannot be deleted.
+    // TODO(manzagop): metrics for the number of non-deletable files.

[Sigurður Ásgeirsson, 2016/09/15 18:44:10]

is there any reason you can't collect these metrics here for reporting by the
user, and/or just UMA them directly?

[manzagop (departed), 2016/09/16 16:37:30]

Done.

+    if (!base::DeleteFile(file, false)) {
+      LOG(ERROR) << "Failed to delete " << file.value();
+      continue;
+    }
+
+    // Finalize the report wrt the report database.
+    call_error_writing_crash_report.Disarm();
+    crashpad::UUID unused_report_id;
+    database_status = report_database->FinishedWritingCrashReport(
+        new_report, &unused_report_id);
+    if (database_status != CrashReportDatabase::kNoError) {
+      LOG(ERROR) << "FinishedWritingCrashReport failed";
+      continue;
+    }
+  }
+
+  return postmortem_cnt;
+}
+
+std::vector<FilePath> PostmortemReportCollector::GetDebugStateFilePaths(
+    const base::FilePath& debug_info_dir,
+    const base::FilePath::StringType& debug_file_pattern,
+    const std::set<FilePath>& excluded_debug_files) {
+  DCHECK_NE(true, debug_info_dir.empty());
+  DCHECK_NE(true, debug_file_pattern.empty());
+
+  std::vector<FilePath> paths;
+  base::FileEnumerator enumerator(debug_info_dir, false /* recursive */,
+                                  base::FileEnumerator::FILES,
+                                  debug_file_pattern);
+  FilePath path;
+  for (path = enumerator.Next(); !path.empty(); path = enumerator.Next()) {
+    if (excluded_debug_files.find(path) == excluded_debug_files.end())
+      paths.push_back(path);
+  }
+  return paths;
+}
+
+std::unique_ptr<StabilityReport> PostmortemReportCollector::Collect(
+    const base::FilePath& debug_state_file) {
+  // Create a global analyzer.
+  std::unique_ptr<GlobalActivityAnalyzer> global_analyzer =
+      GlobalActivityAnalyzer::CreateWithFile(debug_state_file);
+  if (!global_analyzer)
+    return nullptr;
+
+  // Early exit if there is no data.
+  ThreadActivityAnalyzer* thread_analyzer = global_analyzer->GetFirstAnalyzer();
+  if (!thread_analyzer) {
+    // No data. This case happens in the case of a clean exit.
+    return nullptr;
+  }
+
+  // Iterate through the thread analyzers, fleshing out the report.
+  std::unique_ptr<StabilityReport> report(new StabilityReport());
+  ProcessState* process_state = report->add_process_states();
+
+  while (thread_analyzer) {

[Sigurður Ásgeirsson, 2016/09/15 18:44:10]

could write this as a for(;;)?

[manzagop (departed), 2016/09/16 16:37:30]

Done, but note I've left the init statement empty as I think it better
highlights it's common to get no analyzer (in the clean exit case).

[Sigurður Ásgeirsson, 2016/09/16 17:01:02]

I like it - I often use the same idiom, mainly because multi-line for(;;)
clauses annoy me :).

[manzagop (departed), 2016/09/16 18:06:41]

Acknowledged.

+    // Only valid analyzers are expected per contract of GetFirstAnalyzer /
+    // GetNextAnalyzer.
+    DCHECK(thread_analyzer->IsValid());
+
+    ThreadState* thread_state = process_state->add_threads();
+    thread_state->set_thread_name(thread_analyzer->GetThreadName());
+
+    // TODO(manzagop): flesh this out.
+
+    thread_analyzer = global_analyzer->GetNextAnalyzer();
+  }
+
+  return report;
+}
+
+bool PostmortemReportCollector::WriteReportToMinidump(
+    const StabilityReport& report,
+    const crashpad::UUID& client_id,
+    const crashpad::UUID& report_id,
+    base::PlatformFile minidump_file) {
+  MinidumpInfo mindump_info;
+  mindump_info.client_id = client_id;
+  mindump_info.report_id = report_id;
+  mindump_info.product_name = version_info::GetProductName();
+  mindump_info.version_number = version_info::GetVersionNumber();
+
+  return WritePostmortemDump(minidump_file, report, mindump_info);
+}
+
+}  // namespace browser_watcher