Switch stability reports to use the crashed version's details

components/browser_watcher/postmortem_minidump_writer_win.cc

 // 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 <map>
 #include <type_traits>
 #include <vector>
 
 #include "base/files/file_util.h"
 #include "base/macros.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/numerics/safe_math.h"
 #include "base/strings/string_piece.h"
+#include "components/browser_watcher/stability_data_names.h"
 #include "third_party/crashpad/crashpad/minidump/minidump_extensions.h"
 
 namespace browser_watcher {
 
 namespace {
 
 // 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;
 
+struct ProductDetails {
+  std::string product;
+  std::string channel;
+  std::string platform;
+  std::string version;
+};
+
+bool GetStringFromTypedValueMap(
+    const google::protobuf::Map<std::string, TypedValue>& map,
+    const std::string& key,
+    std::string* out) {
+  DCHECK(out);
+
+  auto it = map.find(key);
+  if (it == map.end())
+    return false;
+
+  const TypedValue& value = it->second;
+  if (value.value_case() != TypedValue::kStringValue)
+    return false;
+
+  *out = value.string_value();
+  return true;
+}
+
+bool GetProductDetails(
+    const google::protobuf::Map<std::string, TypedValue>& global_data,
+    ProductDetails* product_details) {
+  DCHECK(product_details);
+
+  if (!GetStringFromTypedValueMap(global_data, kStabilityProduct,
+                                  &(product_details->product)))
+    return false;
+  if (!GetStringFromTypedValueMap(global_data, kStabilityChannel,
+                                  &(product_details->channel)))
+    return false;
+  if (!GetStringFromTypedValueMap(global_data, kStabilityPlatform,
+                                  &(product_details->platform)))
+    return false;
+  return GetStringFromTypedValueMap(global_data, kStabilityVersion,
+                                    &(product_details->version));
+}
+
 int64_t GetFileOffset(base::File* file) {
   DCHECK(file);
   return file->Seek(base::File::FROM_CURRENT, 0LL);
 }
 
 // Returns true if the file is empty, and false if the file is not empty or if
 // there is an error.
 bool IsFileEmpty(base::File* file) {
   DCHECK(file);
   int64_t end = file->Seek(base::File::FROM_END, 0LL);
   return end == 0LL;
 }
 
 // A class with functionality for writing minimal minidump containers to wrap
 // postmortem stability reports.
 // TODO(manzagop): remove this class once Crashpad takes over writing postmortem
 //     minidumps.
 // TODO(manzagop): revisit where the module information should be transported,
 //     in the protocol buffer or in a module stream.
 class PostmortemMinidumpWriter {
  public:
+  // DO NOT CHANGE VALUES. This is logged persistently in a histogram.
+  enum WriteStatus {
+    NONE = 0,

[bcwhite, 2017/02/13 18:35:07]

Omit NONE if you never send it.

[manzagop (departed), 2017/02/13 19:22:36]

Done.

+    SUCCESS = 1,
+    FAILED = 2,
+    FAILED_MISSING_PRODUCT_DETAILS = 3,
+    WRITE_STATUS_MAX = 4
+  };
+
   PostmortemMinidumpWriter();
   ~PostmortemMinidumpWriter();
 
   // Write to |minidump_file| a minimal minidump that wraps |report|. Returns
   // true on success, false otherwise.
   // Note: the caller owns |minidump_file| and is responsible for keeping it
   // valid for this object's lifetime. |minidump_file| is expected to be empty
   // and a binary stream.
   bool WriteDump(base::PlatformFile minidump_file,
-                 const StabilityReport& report,
-                 const MinidumpInfo& minidump_info);
+                 const crashpad::UUID& client_id,
+                 const crashpad::UUID& report_id,
+                 StabilityReport* report);
 
  private:
   // An offset within a minidump file. Note: using this type to avoid including
   // windows.h and relying on the RVA type.
   using FilePosition = uint32_t;
 
   // The minidump header is always located at the head.
   static const FilePosition kHeaderPos = 0U;
 
   bool WriteDumpImpl(const StabilityReport& report,
-                     const MinidumpInfo& minidump_info);
+                     const crashpad::UUID& client_id,
+                     const crashpad::UUID& report_id,
+                     const ProductDetails& product_details);
 
   bool AppendCrashpadInfo(const crashpad::UUID& client_id,
                           const crashpad::UUID& report_id,
                           const std::map<std::string, std::string>& crash_keys);
 
   bool AppendCrashpadDictionaryEntry(
       const std::string& key,
       const std::string& value,
       std::vector<crashpad::MinidumpSimpleStringDictionaryEntry>* entries);
 
@@ skipping 40 matching lines @@
 
 PostmortemMinidumpWriter::PostmortemMinidumpWriter()
     : next_available_byte_(0U), minidump_file_(nullptr) {}
 
 PostmortemMinidumpWriter::~PostmortemMinidumpWriter() {
   DCHECK_EQ(nullptr, minidump_file_);
 }
 
 bool PostmortemMinidumpWriter::WriteDump(
     base::PlatformFile minidump_platform_file,
-    const StabilityReport& report,
-    const MinidumpInfo& minidump_info) {
+    const crashpad::UUID& client_id,
+    const crashpad::UUID& report_id,
+    StabilityReport* report) {
   DCHECK_NE(base::kInvalidPlatformFile, minidump_platform_file);
+  DCHECK(report);
 
   DCHECK_EQ(0U, next_available_byte_);
   DCHECK(directory_.empty());
   DCHECK_EQ(nullptr, minidump_file_);
 
+  // Ensure the report contains the crasher's product details.
+  ProductDetails product_details = {};
+  if (!GetProductDetails(report->global_data(), &product_details)) {
+    // The report is missing the basic information to determine the affected
+    // version. Ignore the report.
+    UMA_HISTOGRAM_ENUMERATION("ActivityTracker.Collect.WriteDumpStatus",
+                              FAILED_MISSING_PRODUCT_DETAILS, WRITE_STATUS_MAX);
+    return false;
+  }
+
+  // No need to keep the version details inside the report.
+  report->mutable_global_data()->erase(kStabilityProduct);
+  report->mutable_global_data()->erase(kStabilityChannel);
+  report->mutable_global_data()->erase(kStabilityPlatform);
+  report->mutable_global_data()->erase(kStabilityVersion);
+
   // We do not own |minidump_platform_file|, but we want to rely on base::File's
   // API, and so we need to duplicate it.
   HANDLE duplicated_handle;
   BOOL duplicate_success = ::DuplicateHandle(
       ::GetCurrentProcess(), minidump_platform_file, ::GetCurrentProcess(),
       &duplicated_handle, 0, FALSE, DUPLICATE_SAME_ACCESS);
-  if (!duplicate_success)
+  if (!duplicate_success) {
+    UMA_HISTOGRAM_ENUMERATION("ActivityTracker.Collect.WriteDumpStatus", FAILED,

[bcwhite, 2017/02/13 18:35:07]

To avoid duplicate histogram instantiation code, rearrange this so into "if
(success) { ... }" code and have a common "status" variable that gets set
appropriately.  Then update the histogram once below.

Alternatively, you can create a separate method (UpdateDumpStatusHistogram?) and
call that with the desired code.

[manzagop (departed), 2017/02/13 19:22:36]

Ah right! Went with the latter, to limit indentations.

+                              WRITE_STATUS_MAX);
     return false;
+  }
   base::File minidump_file(duplicated_handle);
   DCHECK(minidump_file.IsValid());
   minidump_file_ = &minidump_file;
   DCHECK_EQ(0LL, GetFileOffset(minidump_file_));
   DCHECK(IsFileEmpty(minidump_file_));
 
   // Write the minidump, then reset members.
-  bool success = WriteDumpImpl(report, minidump_info);
+  bool success = WriteDumpImpl(*report, client_id, report_id, product_details);
   next_available_byte_ = 0U;
   directory_.clear();
   minidump_file_ = nullptr;
 
+  WriteStatus status = success ? SUCCESS : FAILED;

[bcwhite, 2017/02/13 18:35:07]

Move this inline below.

[manzagop (departed), 2017/02/13 19:22:36]

Done.

+  UMA_HISTOGRAM_ENUMERATION("ActivityTracker.Collect.WriteDumpStatus", status,
+                            WRITE_STATUS_MAX);
   return success;
 }
 
 bool PostmortemMinidumpWriter::WriteDumpImpl(
     const StabilityReport& report,
-    const MinidumpInfo& minidump_info) {
+    const crashpad::UUID& client_id,
+    const crashpad::UUID& report_id,
+    const ProductDetails& product_details) {
   // Allocate space for the header and seek the cursor.
   FilePosition pos = 0U;
   if (!Allocate(sizeof(MINIDUMP_HEADER), &pos))
     return false;
   if (!SeekCursor(sizeof(MINIDUMP_HEADER)))
     return false;
   DCHECK_EQ(kHeaderPos, pos);
 
   // Write the proto to the file.
   std::string serialized_report;
   if (!report.SerializeToString(&serialized_report))
     return false;
   FilePosition report_pos = 0U;
   if (!AppendBytes(serialized_report, &report_pos))
     return false;
 
   // The directory entry for the stability report's stream.
   RegisterDirectoryEntry(kStabilityReportStreamType, report_pos,
                          serialized_report.length());
 
   // 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
   // current executable's values are an (imperfect) proxy.
   std::map<std::string, std::string> crash_keys = {
-      {"prod", minidump_info.product_name + "_Postmortem"},
-      {"ver", minidump_info.version_number},
-      {"channel", minidump_info.channel_name},
-      {"plat", minidump_info.platform}};
-  if (!AppendCrashpadInfo(minidump_info.client_id, minidump_info.report_id,
-                          crash_keys))
+      {"prod", product_details.product + "_Postmortem"},
+      {"ver", product_details.version},
+      {"channel", product_details.channel},
+      {"plat", product_details.platform}};
+  if (!AppendCrashpadInfo(client_id, report_id, crash_keys))
     return false;
 
   // Write the directory.
   FilePosition directory_pos = 0U;
   if (!AppendVec(directory_, &directory_pos))
     return false;
 
   // Write the header.
   MINIDUMP_HEADER header;
   header.Signature = MINIDUMP_SIGNATURE;
@@ skipping 182 matching lines @@
                                                       uint32_t size) {
   MINIDUMP_DIRECTORY entry = {0};
   entry.StreamType = stream_type;
   entry.Location.Rva = pos;
   entry.Location.DataSize = size;
   directory_.push_back(entry);
 }
 
 }  // namespace
 
-MinidumpInfo::MinidumpInfo() {}
+bool WritePostmortemDump(base::PlatformFile minidump_file,
+                         const crashpad::UUID& client_id,
+                         const crashpad::UUID& report_id,
+                         StabilityReport* report) {
+  DCHECK(report);
 
-MinidumpInfo::~MinidumpInfo() {}
-
-bool WritePostmortemDump(base::PlatformFile minidump_file,
-                         const StabilityReport& report,
-                         const MinidumpInfo& minidump_info) {
   PostmortemMinidumpWriter writer;
-  return writer.WriteDump(minidump_file, report, minidump_info);
+  return writer.WriteDump(minidump_file, client_id, report_id, report);
 }
 
 }  // namespace browser_watcher