Collect unclean shutdown debug information

components/browser_watcher/postmortem_report_collector.h

+// 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.
+//
+// Following an unclean shutdown, a stability report can be collected and
+// submitted for upload to a reporter.
+
+#ifndef COMPONENTS_BROWSER_WATCHER_POSTMORTEM_REPORT_COLLECTOR_H_
+#define COMPONENTS_BROWSER_WATCHER_POSTMORTEM_REPORT_COLLECTOR_H_
+
+#include <stdio.h>
+
+#include <memory>
+#include <set>
+#include <vector>
+
+#include "base/files/file.h"
+#include "base/files/file_path.h"
+#include "base/gtest_prod_util.h"
+#include "base/macros.h"
+#include "components/browser_watcher/stability_report.pb.h"
+#include "third_party/crashpad/crashpad/client/crash_report_database.h"
+
+namespace browser_watcher {
+
+// Collects unclean shutdown information, builds reports out of it and submits
+// the reports for upload to a delegate.
+// TODO(manzagop): throttling, graceful handling of accumulating data.
+// TODO(manzagop): UMA metrics and some error logging.

[Sigurður Ásgeirsson, 2016/08/11 17:44:15]

as we discussed earlier today, it'd be better^Weasier to do this from inside the
browser process for now. That way you have the benefit of UMA and finch without
trouble, and if you do this from e.g. within the metrics provider, you'll be
running fairly early in the process lifetime, so stability should be fine.

[manzagop (departed), 2016/08/12 21:23:22]

Sgtm. Let's include moving the code and this TODO as part of this CL.

+class PostmortemReportCollector {
+ public:
+  // Creates a postmortem report collector that will look for unclean shutdown
+  // data in |debug_dir| using |debug_file_pattern|. If data is found,
+  // |delegate| will be used for reporting.
+  PostmortemReportCollector(
+      const base::FilePath& debug_dir,
+      const base::FilePath::StringType& debug_file_pattern,
+      std::unique_ptr<crashpad::CrashReportDatabase> report_database);
+  virtual ~PostmortemReportCollector() = default;
+
+  // Collects reports about unclean shutdowns and submits them for upload.
+  // |excluded_debug_files| is a set of files to exclude, such as files
+  // pertaining to a running instance.
+  void CollectAndSubmitForUpload(
+      const std::set<base::FilePath>& excluded_debug_files);
+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(PostmortemReportCollectorTest,
+                           GetDebugStateFilePaths);
+  FRIEND_TEST_ALL_PREFIXES(PostmortemReportCollectorTest, CollectEmptyFile);
+  FRIEND_TEST_ALL_PREFIXES(PostmortemReportCollectorTest, CollectRandomFile);
+  FRIEND_TEST_ALL_PREFIXES(PostmortemReportCollectorCollectionTest,
+                           CollectSuccess);
+
+  PostmortemReportCollector();
+
+  // Virtual for unittesting.

[Sigurður Ásgeirsson, 2016/08/11 17:44:15]

I do hope there's a test for this method, as it generates file names that are
later deleted. It'd be ... awkward .. if there's an error in this method that
fingers the wrong files :O.

[manzagop (departed), 2016/08/12 21:23:22]

Well... there is a test!

The reason: I was thinking of using unique debug state files (e.g.
DebugState-<PID>-<TID>-<creation-time>.pma). 

Brian suggests it would be simpler/sufficient to work with only 2 file names:
"current" and "previous". This would overwrite any debug state that failed to be
registered to crashpad by the subsequent browser run. I'm coming to agree. WDYT?

WDYT?

+  virtual std::vector<base::FilePath> GetDebugStateFilePaths(
+      const std::set<base::FilePath>& excluded_debug_files);
+
+  // Virtual for unittesting.
+  // TODO(manzagop): move this for reuse in live scenario.
+  virtual std::unique_ptr<StabilityReport> Collect(
+      const base::FilePath& debug_state_file);
+
+  virtual bool WriteReportToMinidump(const StabilityReport& report,
+                                     const crashpad::UUID& client_id,
+                                     const crashpad::UUID& report_id,
+                                     FILE* minidump_file);
+
+  base::FilePath debug_state_dir_;

[Sigurður Ásgeirsson, 2016/08/11 17:44:15]

there doesn't seem to be any benefit to the debug_* member variables that I can
see?

[manzagop (departed), 2016/08/12 21:23:22]

Agreed, removed. The class is now an empty husk. I've kept the functions
non-static as it enables use of gmock.

+  base::FilePath::StringType debug_file_pattern_;
+  std::unique_ptr<crashpad::CrashReportDatabase> report_database_;
+
+  DISALLOW_COPY_AND_ASSIGN(PostmortemReportCollector);
+};
+
+}  // namespace browser_watcher
+
+#endif  // COMPONENTS_BROWSER_WATCHER_POSTMORTEM_REPORT_COLLECTOR_H_