Stability instrumentation Crashpad integration

components/browser_watcher/stability_report_user_stream_data_source.cc

+// Copyright 2017 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/stability_report_user_stream_data_source.h"
+
+#include <string>
+#include <utility>
+
+#include "base/files/file.h"
+#include "base/files/file_util.h"
+#include "base/memory/ptr_util.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/strings/string16.h"
+#include "base/time/time.h"
+#include "components/browser_watcher/minidump_user_streams.h"
+#include "components/browser_watcher/stability_paths.h"
+#include "components/browser_watcher/stability_report_extractor.h"
+#include "third_party/crashpad/crashpad/minidump/minidump_user_extension_stream_data_source.h"
+#include "third_party/crashpad/crashpad/snapshot/process_snapshot.h"
+
+namespace browser_watcher {
+
+namespace {
+
+void GetStabilityFileName(const base::string16& user_data_dir,

[Sigurður Ásgeirsson, 2017/05/11 19:28:29]

Actually, might as well return the file path?

[grt (UTC plus 2), 2017/05/11 21:08:16]

yes, please. we treat FilePath as a value type -- copy elision and RVO make it
efficient.

[manzagop (departed), 2017/05/12 19:27:32]

Done. 

+                          crashpad::ProcessSnapshot* process_snapshot,
+                          base::FilePath* stability_file) {
+  DCHECK(process_snapshot);
+  DCHECK(stability_file);
+
+  timeval creation_time{};

[grt (UTC plus 2), 2017/05/11 21:08:16]

nit: chromium prefers assignment when possible
(https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...):
  timeval creation_time = {};

[manzagop (departed), 2017/05/12 19:27:32]

Done.

+  process_snapshot->ProcessStartTime(&creation_time);
+
+  *stability_file =
+      GetStabilityFileForProcess(process_snapshot->ProcessID(), creation_time,
+                                 base::FilePath(user_data_dir));
+}
+
+class BufferExtensionStreamDataSource final

[grt (UTC plus 2), 2017/05/11 21:08:16]

i see precious few classes marked "final" in chromium and don't recall seeing a
discussion about it. would you mind removing this and starting a thread on
cxx@chromium.org about it?

[manzagop (departed), 2017/05/12 19:27:32]

Validating before sending the email. It seems to be be on the chromium c++ 11
list.
https://chromium-cpp.appspot.com/
The thread includes classes but discussion mostly focuses on functions.

Shall I double check with an email?

[grt (UTC plus 2), 2017/05/15 13:03:18]

Yes. I read through some of the discussions, and everyone seemed to be talking
exclusively about final methods rather than final classes. Thanks.

[manzagop (departed), 2017/05/15 13:43:45]

Sent the email. Done.

+    : public crashpad::MinidumpUserExtensionStreamDataSource {
+ public:
+  BufferExtensionStreamDataSource(uint32_t stream_type,
+                                  const StabilityReport& report);
+
+  bool Init() { return is_init_; }

[Sigurður Ásgeirsson, 2017/05/11 19:28:29]

Hmmm, this seems contrived and/or not super well named. 
I'd have gone with an Init(const StabilityReport& report) method, which can
return failure.

If you want to assert on a precondition, you can test !data_.empty(), as
presumably that will never happen on successful serialization.

[manzagop (departed), 2017/05/12 19:27:32]

Done.

+
+  // Precondition: Init() returns true.
+  size_t StreamDataSize() override;
+  bool ReadStreamData(Delegate* delegate) override;
+
+ private:
+  bool is_init_;
+  std::string data_;
+
+  DISALLOW_COPY_AND_ASSIGN(BufferExtensionStreamDataSource);
+};
+
+BufferExtensionStreamDataSource::BufferExtensionStreamDataSource(
+    uint32_t stream_type,
+    const StabilityReport& report)
+    : crashpad::MinidumpUserExtensionStreamDataSource(stream_type),
+      is_init_(true) {
+  if (!report.SerializeToString(&data_)) {
+    data_.clear();
+    is_init_ = false;
+  }
+}
+
+size_t BufferExtensionStreamDataSource::StreamDataSize() {
+  DCHECK(Init());
+  return data_.size();
+}
+
+bool BufferExtensionStreamDataSource::ReadStreamData(Delegate* delegate) {
+  DCHECK(Init());
+  return delegate->ExtensionStreamDataSourceRead(
+      data_.size() ? data_.data() : nullptr, data_.size());
+}
+
+std::unique_ptr<BufferExtensionStreamDataSource> CollectReport(
+    const base::FilePath& path) {
+  StabilityReport report;
+  CollectionStatus status = Extract(path, &report);
+  UMA_HISTOGRAM_ENUMERATION("ActivityTracker.CollectCrash.Status", status,
+                            COLLECTION_STATUS_MAX);
+  if (status != SUCCESS)
+    return nullptr;
+
+  // Open (with delete) and then immediately close the file by going out of
+  // scope. This should cause the stability debugging file to be deleted prior
+  // to the next execution.
+  // TODO(manzagop): set the persistent allocator file's state to deleted in
+  //     case the file can't be deleted.
+  base::File file(path, base::File::FLAG_OPEN | base::File::FLAG_READ |
+                            base::File::FLAG_DELETE_ON_CLOSE);
+  UMA_HISTOGRAM_BOOLEAN("ActivityTracker.CollectCrash.OpenForDeleteSuccess",
+                        file.IsValid());
+
+  std::unique_ptr<BufferExtensionStreamDataSource> source(
+      new BufferExtensionStreamDataSource(kStabilityReportStreamType, report));
+  return source->Init() ? std::move(source) : nullptr;
+}
+
+}  // namespace
+
+StabilityReportUserStreamDataSource::StabilityReportUserStreamDataSource(
+    const base::string16& user_data_dir)
+    : user_data_dir_(user_data_dir) {}
+
+std::unique_ptr<crashpad::MinidumpUserExtensionStreamDataSource>
+StabilityReportUserStreamDataSource::ProduceStreamData(
+    crashpad::ProcessSnapshot* process_snapshot) {
+  DCHECK(process_snapshot);
+
+  if (user_data_dir_.empty())
+    return nullptr;
+
+  base::FilePath stability_file;
+  GetStabilityFileName(user_data_dir_, process_snapshot, &stability_file);
+  if (!PathExists(stability_file)) {
+    // Either this is not an instrumented process (currenlty only browser

[Sigurður Ásgeirsson, 2017/05/11 19:28:29]

nit: speling [sic]

[manzagop (departed), 2017/05/12 19:27:32]

Done.

+    // processes can be instrumented), or the stability file cannot be found.
+    return nullptr;
+  }
+
+  return CollectReport(stability_file);
+}
+
+}  // namespace browser_watcher