Stability instrumentation Crashpad integration

components/browser_watcher/stability_report_user_stream_data_source.cc

Index: components/browser_watcher/stability_report_user_stream_data_source.cc
diff --git a/components/browser_watcher/stability_report_user_stream_data_source.cc b/components/browser_watcher/stability_report_user_stream_data_source.cc
new file mode 100644
index 0000000000000000000000000000000000000000..caad17853e7e517cb3a8988ddb6faaf0f1d25f16
--- /dev/null
+++ b/components/browser_watcher/stability_report_user_stream_data_source.cc
@@ -0,0 +1,83 @@
+// 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 "base/files/file.h"
+#include "base/files/file_util.h"
+#include "base/memory/ptr_util.h"
+#include "base/strings/string16.h"
+#include "base/time/time.h"
+// DO NOT SUBMIT: is this ok?
+#include "chrome/install_static/user_data_dir.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"
+// DO NOT SUBMIT: move out of test folder.
+#include "third_party/crashpad/crashpad/minidump/test/minidump_user_extension_stream_util.h"
+#include "third_party/crashpad/crashpad/snapshot/process_snapshot.h"
+
+namespace browser_watcher {
+
+namespace {
+
+bool GetStabilityFileName(crashpad::ProcessSnapshot* process_snapshot,

[Sigurður Ásgeirsson, 2017/05/10 17:46:23]

I'd suggest you grab the user data dir from the user data dir argument, and pass
into your instance at creation.

[manzagop (departed), 2017/05/10 23:06:11]

Done.

+                          base::FilePath* stability_file) {
+  DCHECK(process_snapshot);
+  DCHECK(stability_file);
+
+  base::string16 user_data_dir;
+  if (!install_static::GetUserDataDirectory(&user_data_dir, nullptr))
+    return false;
+
+  timeval creation_time{};
+  process_snapshot->ProcessStartTime(&creation_time);
+
+  *stability_file =
+      GetStabilityFileForProcess(process_snapshot->ProcessID(), creation_time,
+                                 base::FilePath(user_data_dir));
+  return true;
+}
+
+}  // namespace
+
+std::unique_ptr<crashpad::MinidumpUserExtensionStreamDataSource>
+StabilityReportUserStreamDataSource::ProduceStreamData(
+    crashpad::ProcessSnapshot* process_snapshot) {
+  DCHECK(process_snapshot);
+
+  base::FilePath stability_file;
+  GetStabilityFileName(process_snapshot, &stability_file);
+  if (!PathExists(stability_file)) {
+    // Either this is not an instrumented process, or the stability file cannot
+    // be found.
+    return nullptr;
+  }
+
+  StabilityReport report;

[Sigurður Ásgeirsson, 2017/05/10 17:46:23]

make sure this report is deallocated as soon as you're done with it.

As-is, you'll have three copies of this data in memory at once.

[manzagop (departed), 2017/05/10 23:06:11]

Done.

+  CollectionStatus status = Extract(stability_file, &report);
+  if (status != SUCCESS)
+    return nullptr;
+  std::string serialized_report;
+  if (!report.SerializeToString(&serialized_report))
+    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(stability_file, base::File::FLAG_OPEN |

[Sigurður Ásgeirsson, 2017/05/10 17:46:23]

You probably want to capture a metric for how often this fails (cause it's going
to fail).

[manzagop (departed), 2017/05/10 23:06:11]

My initial thought was we can't do metrics, but I guess they might end up in the
Crashpad metrics file? I'll check with Scott and Brian.

That said, how would I check this? The file will only be deleted after the
crashing process gets cleaned up? I think the way to go here is to update the
file's persitent allocator's state - the TODO above.

+                                      base::File::FLAG_READ |
+                                      base::File::FLAG_DELETE_ON_CLOSE);
+
+  return base::WrapUnique(new crashpad::test::BufferExtensionStreamDataSource(

[Sigurður Ásgeirsson, 2017/05/10 17:46:23]

just re-implement the damn thing, and make sure you get rid of this copying.
This code is going to be running under VERY dire memory restrictions on OOM
crashes and duplicating the stream here the wrong thing to do.

[manzagop (departed), 2017/05/10 23:06:11]

Done.

+      kStabilityReportStreamType, serialized_report.data(),
+      serialized_report.size()));
+}
+
+}  // namespace browser_watcher