Switch stability reports to use the crashed version's details

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
index 4a61f1efb2f831e48ffb62d7e313939d8d589a2d..29e281105acee6363ca69b34edd0deaf3ee2fdb5 100644
--- a/components/browser_watcher/postmortem_minidump_writer_win.cc
+++ b/components/browser_watcher/postmortem_minidump_writer_win.cc
@@ -19,8 +19,10 @@
 
 #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 {
@@ -34,6 +36,49 @@ namespace {
 // 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;

[bcwhite, 2017/02/13 13:16:59]

Is it possible to fetch only partial data?  Would that be a "failure"?

[manzagop (departed), 2017/02/13 17:07:45]

Missing fields are unlikely (all 4 are recorded as soon as we enable tracking)
but still possible. I'm thinking we can start by considering it a failure
because it's likely not worth the effort of handling unless the UMA metric says
otherwise.

+  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);
@@ -64,8 +109,9 @@ class PostmortemMinidumpWriter {
   // 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
@@ -76,7 +122,9 @@ class PostmortemMinidumpWriter {
   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,
@@ -137,14 +185,32 @@ PostmortemMinidumpWriter::~PostmortemMinidumpWriter() {
 
 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_COUNTS_100("ActivityTracker.Collect.MissingProductDetails",
+                             1);
+    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;
@@ -160,7 +226,7 @@ bool PostmortemMinidumpWriter::WriteDump(
   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;
@@ -170,7 +236,9 @@ bool PostmortemMinidumpWriter::WriteDump(
 
 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))
@@ -197,12 +265,11 @@ bool PostmortemMinidumpWriter::WriteDumpImpl(
   // 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.
@@ -405,15 +472,14 @@ void PostmortemMinidumpWriter::RegisterDirectoryEntry(uint32_t stream_type,
 
 }  // namespace
 
-MinidumpInfo::MinidumpInfo() {}
-
-MinidumpInfo::~MinidumpInfo() {}
-
 bool WritePostmortemDump(base::PlatformFile minidump_file,
-                         const StabilityReport& report,
-                         const MinidumpInfo& minidump_info) {
+                         const crashpad::UUID& client_id,
+                         const crashpad::UUID& report_id,
+                         StabilityReport* report) {
+  DCHECK(report);
+
   PostmortemMinidumpWriter writer;
-  return writer.WriteDump(minidump_file, report, minidump_info);
+  return writer.WriteDump(minidump_file, client_id, report_id, report);
 }
 
 }  // namespace browser_watcher