Collect unclean shutdown debug information

components/browser_watcher/postmortem_minidump_writer_win.cc

Index: components/browser_watcher/postmortem_minidump_writer_win.cc
diff --git a/components/browser_watcher/postmortem_minidump_writer_win.cc b/components/browser_watcher/postmortem_minidump_writer_win.cc
new file mode 100644
index 0000000000000000000000000000000000000000..e940f908937e1621ec037c86229f374e212ea8a4
--- /dev/null
+++ b/components/browser_watcher/postmortem_minidump_writer_win.cc
@@ -0,0 +1,218 @@
+// 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.
+//
+// Note: aside from using windows headers to obtain the definitions of minidump
+//     structures, nothing here is windows specific. This seems like the best
+//     approach given this code is for temporary experimentation on Windows.
+//     Longer term, CrashPad will take over the minidump writing in this case as
+//     well.
+
+#include "components/browser_watcher/postmortem_minidump_writer.h"
+
+#include <windows.h>  // NOLINT
+#include <dbghelp.h>
+
+#include <string>
+
+#include "base/files/file_util.h"
+#include "base/numerics/safe_math.h"
+#include "components/version_info/version_info.h"
+#include "third_party/crashpad/crashpad/minidump/minidump_extensions.h"
+
+namespace browser_watcher {
+
+// The stream type assigned to the minidump stream that holds the serialized
+// stability report.
+// Note: the value was obtained by adding 1 to the stream type used for holding
+// the SyzyAsan proto.
+// TODO(manzagop): centralize the stream type definitions to avoid issues.
+const uint32_t kStabilityReportStreamType = 0x4B6B0002;
+
+PostmortemMinidumpWriter::PostmortemMinidumpWriter(FILE* minidump_file)
+    : cursor_(0U), minidump_file_(minidump_file) {}

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

DCHECK or even CHECK minidump_file here?

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

Done.

+
+bool PostmortemMinidumpWriter::WriteDump(const StabilityReport& report,
+                                         const crashpad::UUID& client_id,
+                                         const crashpad::UUID& report_id) {
+  DCHECK_NE(nullptr, minidump_file_);
+  DCHECK_EQ(0U, cursor_);
+  DCHECK(directory_.empty());
+
+  // Allocate space for the header.
+  Position pos = 0U;
+  if (!Allocate(sizeof(MINIDUMP_HEADER), &pos))
+    return false;
+  DCHECK_EQ(kHeaderPos, pos);
+
+  // Write the proto to the file.
+  std::string serialized_report;
+  report.SerializeToString(&serialized_report);

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

Looks like this can fail - check the return value.

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

Huh. Looks like it's when messages are too large. Done.

+  Position report_pos = 0U;
+  if (!AppendBytes(serialized_report, &report_pos))
+    return false;
+
+  // The directory entry for the stability report's stream.
+  {
+    MINIDUMP_DIRECTORY report_directory = {0};
+    report_directory.StreamType = kStabilityReportStreamType;
+    report_directory.Location.Rva = report_pos;
+    report_directory.Location.DataSize = serialized_report.length();
+    directory_.push_back(report_directory);
+  }
+
+  // Write mandatory crash keys. These will be read by crashpad and used as
+  // http request parameters for the upload. Keys and values should match
+  // server side configuration.
+  // TODO(manzagop): use product and version from the stability report. The

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

I think you want to pass the product and version into this function, and you'd
want to use the base product from crash_reporter::CrashReporterClient, I think?

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

Now passing in product and version.

Is the version_info component not correct? crash_reporter::CrashReporterClient
seems to have different signatures depending on the platform and requires a
::GetModuleFileName?

+  // current executable's values are an (imperfect) proxy.
+  std::map<std::string, std::string> crash_keys = {
+      {"product", version_info::GetProductName() + "_Postmortem"},

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

Is this crash team's recommended way to disambiguate? Note that you'll need to
register any new product with the crash backend before you can collect anything.

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

Yes, this is what we discussed.

+      {"version", version_info::GetVersionNumber()}};
+  if (!AppendCrashpadInfo(client_id, report_id, crash_keys))
+    return false;
+
+  // Write the directory.
+  Position directory_pos = 0U;
+  if (!AppendVec(directory_, &directory_pos))
+    return false;
+
+  // Write the header.
+  MINIDUMP_HEADER header;

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

so the module information is solely in the protobuf? I guess this implies
there'll be some duplication for "real" crash reports? Will this make it harder
to reuse existing symbolization code, over recording the module information in
the minidump module information stream?

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

I've placed a TODO in the .h to revisit where this information should be kept.

+  header.Signature = MINIDUMP_SIGNATURE;
+  header.Version = MINIDUMP_VERSION;
+  header.NumberOfStreams = directory_.size();
+  header.StreamDirectoryRva = directory_pos;
+  return Write(kHeaderPos, header);
+}
+
+bool PostmortemMinidumpWriter::AppendCrashpadInfo(
+    const crashpad::UUID& client_id,
+    const crashpad::UUID& report_id,
+    const std::map<std::string, std::string>& crash_keys) {
+  // Write the crash keys as the contents of a crashpad dictionary.
+  std::vector<crashpad::MinidumpSimpleStringDictionaryEntry> entries;
+  for (const auto& crash_key : crash_keys) {
+    if (!AppendCrashpadDictionaryEntry(crash_key.first, crash_key.second,
+                                       &entries)) {
+      return false;
+    }
+  }
+
+  // Write the dictionary's index.
+  Position dict_pos = 0U;
+  uint32_t entry_count = entries.size();
+  if (entry_count > 0) {
+    if (!Append(entry_count, &dict_pos))
+      return false;
+    Position unused_pos = 0U;
+    if (!AppendVec(entries, &unused_pos))
+      return false;
+  }
+
+  MINIDUMP_LOCATION_DESCRIPTOR simple_annotations;

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

a simpler way to do this would be to subtract dict_pos from cursor_?

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

Totally! Done.

+  simple_annotations.DataSize = 0U;
+  if (entry_count > 0) {
+    simple_annotations.DataSize =
+        sizeof(crashpad::MinidumpSimpleStringDictionary) +
+        entry_count * sizeof(crashpad::MinidumpSimpleStringDictionaryEntry);
+  }
+  simple_annotations.Rva = dict_pos;

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

do you want a non-null RVA for an empty list?

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

An RVA of 0 signals the absence of a dictionary. Adding a comment.

+
+  // Write the crashpad info.
+  crashpad::MinidumpCrashpadInfo crashpad_info;
+  crashpad_info.version = crashpad::MinidumpCrashpadInfo::kVersion;
+  crashpad_info.report_id = report_id;
+  crashpad_info.client_id = client_id;
+  crashpad_info.simple_annotations = simple_annotations;
+  // Note: module_list is left at 0, which means none.
+
+  Position crashpad_pos = 0U;
+  if (!Append(crashpad_info, &crashpad_pos))
+    return false;
+
+  // Append a directory entry for the crashpad info stream.
+  MINIDUMP_DIRECTORY crashpad_directory = {0};

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

maybe pull the directory extension code out to a utility function?

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

Done.

+  crashpad_directory.StreamType = crashpad::kMinidumpStreamTypeCrashpadInfo;
+  crashpad_directory.Location.Rva = crashpad_pos;
+  crashpad_directory.Location.DataSize = sizeof(crashpad::MinidumpCrashpadInfo);
+  directory_.push_back(crashpad_directory);
+
+  return true;
+}
+
+bool PostmortemMinidumpWriter::AppendCrashpadDictionaryEntry(
+    const std::string& key,
+    const std::string& value,
+    std::vector<crashpad::MinidumpSimpleStringDictionaryEntry>* entries) {
+  DCHECK_NE(nullptr, entries);
+
+  Position key_pos = 0U;
+  if (!AppendUtf8String(key, &key_pos))
+    return false;
+  Position value_pos = 0U;
+  if (!AppendUtf8String(value, &value_pos))
+    return false;
+
+  crashpad::MinidumpSimpleStringDictionaryEntry entry = {0};
+  entry.key = key_pos;
+  entry.value = value_pos;
+  entries->push_back(entry);
+
+  return true;
+}
+
+bool PostmortemMinidumpWriter::Allocate(size_t size_bytes, Position* pos) {
+  DCHECK(pos);
+  *pos = cursor_;
+  return IncrementCursor(size_bytes);
+}
+
+bool PostmortemMinidumpWriter::WriteBytes(Position pos,
+                                          size_t size_bytes,
+                                          const void* data) {
+  DCHECK(data);
+  DCHECK_NE(base::kInvalidPlatformFile, minidump_file_);

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

I don't think this is right.

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

Done.

+
+  // Validate the write does not extend past the cursor.
+  base::CheckedNumeric<Position> pos_end = pos;
+  pos_end += size_bytes;
+  if (!pos_end.IsValid() || pos_end.ValueOrDie() > cursor_)
+    return false;
+
+  // Seek and write.

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

ISTR there's a very non-trivial performance hit to seeking in the MSVC CRT, even
when the seek is a noop. This adds up, and this may be running at a ...
difficult ... time.
Maybe you can maintain the invariant that the file position is always at the end
of file, except for the directory write. This is probably best done with a
DCHECK(IsPositionAtEnd()) or some such, wherever you write to the file.

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

Good to know. I've made seeking explicit. Done.

+  if (fseek(minidump_file_, pos, SEEK_SET) != 0)
+    return false;
+  return fwrite(data, sizeof(char), size_bytes, minidump_file_) == size_bytes;
+}
+
+bool PostmortemMinidumpWriter::AppendUtf8String(const std::string data,

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

string&?

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

Changed to StringPiece.

+                                                Position* pos) {
+  DCHECK(pos);
+  uint32_t string_size = data.size();
+  if (!Append(string_size, pos))
+    return false;
+
+  Position unused_pos = 0U;
+  return AppendBytes(data, &unused_pos);
+}
+
+bool PostmortemMinidumpWriter::AppendBytes(base::StringPiece data,
+                                           Position* pos) {
+  DCHECK(pos);
+  if (!Allocate(data.length(), pos))
+    return false;
+  return WriteBytes(*pos, data.length(), data.data());
+}
+
+bool PostmortemMinidumpWriter::IncrementCursor(size_t size_bytes) {
+  base::CheckedNumeric<Position> cur = cursor_;
+  cur += size_bytes;
+  if (!cur.IsValid())
+    return false;
+
+  cursor_ += size_bytes;
+  return true;
+}
+
+}  // namespace browser_watcher