Carry the client ID from the database all the way through upload.

The client ID is added to a new field, MinidumpCrashpadInfo::client_id,
in each minidump file that is written. The ProcessSnapshot::ClientID()
gives access to value at the snapshot level. In the upload thread,
client IDs are retrieved from minidump files and used to populate the
“guid” HTTP form parameter.

The Breakpad client supplies these values at upload without hyphens and
with all capital letters. Currently, the Crashpad client uses hyphens
and lowercase letters when communicating with a Breakpad server.

TEST=crashpad_minidump_test MinidumpCrashpadInfoWriter.*,
     crashpad_snapshot_test ProcessSnapshotMinidump.*,
     run_with_crashpad --handler crashpad_handler \
         -a --database=/tmp/crashpad_db \
         -a --url=https://clients2.google.com/cr/staging_report \
         -a --annotation=prod=crashpad \
         -a --annotation=ver=0.7.0 \
         crashy_program

R=rsesek@chromium.org

Review URL: https://codereview.chromium.org/998033002

handler/mac/crash_report_exception_handler.cc

@@ -21,6 +21,7 @@
 #include "base/logging.h"
 #include "base/mac/mach_logging.h"
 #include "base/strings/stringprintf.h"
+#include "client/settings.h"
 #include "minidump/minidump_file_writer.h"
 #include "snapshot/mac/crashpad_info_client_options.h"
 #include "snapshot/mac/process_snapshot_mac.h"
@@ -132,6 +133,16 @@ kern_return_t CrashReportExceptionHandler::CatchMachException(
       return KERN_FAILURE;
     }
 
+    UUID client_id;
+    Settings* const settings = database_->GetSettings();
+    if (settings) {
+      // If GetSettings() or GetClientID() fails, something else will log a
+      // message and client_id will be left at its default value, all zeroes,
+      // which is appropriate.
+      settings->GetClientID(&client_id);
+    }
+
+    process_snapshot.SetClientID(client_id);
     process_snapshot.SetAnnotationsSimpleMap(*process_annotations_);
 
     CrashReportDatabase::NewReport* new_report;

handler/mac/crash_report_upload_thread.cc

@@ -27,6 +27,7 @@
 #include "snapshot/minidump/process_snapshot_minidump.h"
 #include "snapshot/module_snapshot.h"
 #include "util/file/file_reader.h"
+#include "util/misc/uuid.h"
 #include "util/net/http_body.h"
 #include "util/net/http_multipart_builder.h"
 #include "util/net/http_transport.h"
@@ -35,6 +36,19 @@ namespace crashpad {
 
 namespace {
 
+void InsertOrReplaceMapEntry(std::map<std::string, std::string>* map,
+                             const std::string& key,
+                             const std::string& value) {
+  auto it = map->find(key);
+  if (it != map->end()) {
+    LOG(WARNING) << "duplicate key " << key << ", discarding value "
+                 << it->second;
+    it->second = value;
+  } else {
+    map->insert(std::make_pair(key, value));
+  }
+}
+
 // Given a minidump file readable by |minidump_file_reader|, returns a map of
 // key-value pairs to use as HTTP form parameters for upload to a Breakpad
 // server. The map is built by combining the process simple annotations map with
@@ -43,6 +57,8 @@ namespace {
 // discarded values. Each module’s annotations vector is also examined and built
 // into a single string value, with distinct elements separated by newlines, and
 // stored at the key named “list_annotations”, which supersedes any other key
+// found by that name. The client ID stored in the minidump is converted to
+// a string and stored at the key named “guid”, which supersedes any other key
 // found by that name.
 //
 // In the event of an error reading the minidump file, a message will be logged.
@@ -77,17 +93,13 @@ std::map<std::string, std::string> BreakpadHTTPFormParametersFromMinidump(
     // Remove the final newline character.
     list_annotations.resize(list_annotations.size() - 1);
 
-    const char kListAnnotationsKey[] = "list_annotations";
+    InsertOrReplaceMapEntry(&parameters, "list_annotations", list_annotations);
-    auto it = parameters.find(kListAnnotationsKey);
-    if (it != parameters.end()) {
-      LOG(WARNING) << "duplicate key " << kListAnnotationsKey
-                   << ", discarding value " << it->second;
-      it->second = list_annotations;
-    } else {
-      parameters.insert(std::make_pair(kListAnnotationsKey, list_annotations));
-    }
   }
 
+  UUID client_id;
+  minidump_process_snapshot.ClientID(&client_id);
+  InsertOrReplaceMapEntry(&parameters, "guid", client_id.ToString());
+
   return parameters;
 }
 

minidump/minidump_crashpad_info_writer.cc

@@ -38,6 +38,10 @@ void MinidumpCrashpadInfoWriter::InitializeFromSnapshot(
   DCHECK_EQ(state(), kStateMutable);
   DCHECK(!module_list_);
 
+  UUID client_id;
+  process_snapshot->ClientID(&client_id);
+  SetClientID(client_id);
+
   auto simple_annotations =
       make_scoped_ptr(new MinidumpSimpleStringDictionaryWriter());
   simple_annotations->InitializeFromMap(
@@ -54,6 +58,12 @@ void MinidumpCrashpadInfoWriter::InitializeFromSnapshot(
   }
 }
 
+void MinidumpCrashpadInfoWriter::SetClientID(const UUID& client_id) {
+  DCHECK_EQ(state(), kStateMutable);
+
+  crashpad_info_.client_id = client_id;
+}
+
 void MinidumpCrashpadInfoWriter::SetSimpleAnnotations(
     scoped_ptr<MinidumpSimpleStringDictionaryWriter> simple_annotations) {
   DCHECK_EQ(state(), kStateMutable);
@@ -118,7 +128,9 @@ MinidumpStreamType MinidumpCrashpadInfoWriter::StreamType() const {
 }
 
 bool MinidumpCrashpadInfoWriter::IsUseful() const {
-  return simple_annotations_ || module_list_;
+  return crashpad_info_.client_id != UUID() ||
+         simple_annotations_ ||
+         module_list_;
 }
 
 }  // namespace crashpad

minidump/minidump_crashpad_info_writer.h

@@ -21,6 +21,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "minidump/minidump_extensions.h"
 #include "minidump/minidump_stream_writer.h"
+#include "util/misc/uuid.h"
 
 namespace crashpad {
 
@@ -51,6 +52,9 @@ class MinidumpCrashpadInfoWriter final : public internal::MinidumpStreamWriter {
   //!     methods after this method.
   void InitializeFromSnapshot(const ProcessSnapshot* process_snapshot);
 
+  //! \brief Sets MinidumpCrashpadInfo::client_id.
+  void SetClientID(const UUID& client_id);
+
   //! \brief Arranges for MinidumpCrashpadInfo::simple_annotations to point to
   //!     the MinidumpSimpleStringDictionaryWriter object to be written by \a
   //!     simple_annotations.

minidump/minidump_crashpad_info_writer_test.cc

@@ -80,6 +80,36 @@ TEST(MinidumpCrashpadInfoWriter, Empty) {
       string_file.string(), &crashpad_info, &simple_annotations, &module_list));
 
   EXPECT_EQ(MinidumpCrashpadInfo::kVersion, crashpad_info->version);
+  EXPECT_EQ(UUID(), crashpad_info->client_id);
+  EXPECT_FALSE(simple_annotations);
+  EXPECT_FALSE(module_list);
+}
+
+TEST(MinidumpCrashpadInfoWriter, ClientID) {
+  MinidumpFileWriter minidump_file_writer;
+  auto crashpad_info_writer = make_scoped_ptr(new MinidumpCrashpadInfoWriter());
+
+  UUID client_id;
+  ASSERT_TRUE(
+      client_id.InitializeFromString("00112233-4455-6677-8899-aabbccddeeff"));
+  crashpad_info_writer->SetClientID(client_id);
+
+  EXPECT_TRUE(crashpad_info_writer->IsUseful());
+
+  minidump_file_writer.AddStream(crashpad_info_writer.Pass());
+
+  StringFile string_file;
+  ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file));
+
+  const MinidumpCrashpadInfo* crashpad_info = nullptr;
+  const MinidumpSimpleStringDictionary* simple_annotations = nullptr;
+  const MinidumpModuleCrashpadInfoList* module_list = nullptr;
+
+  ASSERT_NO_FATAL_FAILURE(GetCrashpadInfoStream(
+      string_file.string(), &crashpad_info, &simple_annotations, &module_list));
+
+  EXPECT_EQ(MinidumpCrashpadInfo::kVersion, crashpad_info->version);
+  EXPECT_EQ(client_id, crashpad_info->client_id);
   EXPECT_FALSE(simple_annotations);
   EXPECT_FALSE(module_list);
 }
@@ -178,6 +208,10 @@ TEST(MinidumpCrashpadInfoWriter, CrashpadModuleList) {
 }
 
 TEST(MinidumpCrashpadInfoWriter, InitializeFromSnapshot) {
+  UUID client_id;
+  ASSERT_TRUE(
+      client_id.InitializeFromString("fedcba98-7654-3210-0123-456789abcdef"));
+
   const char kKey[] = "version";
   const char kValue[] = "40.0.2214.111";
   const char kEntry[] = "This is a simple annotation in a list.";
@@ -196,6 +230,8 @@ TEST(MinidumpCrashpadInfoWriter, InitializeFromSnapshot) {
   // Try again with a useful module.
   process_snapshot.reset(new TestProcessSnapshot());
 
+  process_snapshot->SetClientID(client_id);
+
   std::map<std::string, std::string> annotations_simple_map;
   annotations_simple_map[kKey] = kValue;
   process_snapshot->SetAnnotationsSimpleMap(annotations_simple_map);
@@ -223,6 +259,8 @@ TEST(MinidumpCrashpadInfoWriter, InitializeFromSnapshot) {
 
   EXPECT_EQ(MinidumpCrashpadInfo::kVersion, info->version);
 
+  EXPECT_EQ(client_id, info->client_id);
+
   ASSERT_TRUE(simple_annotations);
   ASSERT_EQ(1u, simple_annotations->count);
   EXPECT_EQ(kKey,

minidump/minidump_extensions.h

@@ -460,6 +460,18 @@ struct ALIGNAS(4) PACKED MinidumpCrashpadInfo {
   //! no need for any fields present in later versions.
   uint32_t version;
 
+  //! \brief A %UUID identifying the client that crashed.
+  //!
+  //! Client identification is within the scope of the application, but it is
+  //! expected that the identifier will be unique for an instance of Crashpad
+  //! monitoring an application or set of applications for a user. The
+  //! identifier shall remain stable over time.
+  //!
+  //! If no identifier is available, this field will contain zeroes.
+  //!
+  //! This field is present when #version is at least `1`.
+  UUID client_id;
+
   //! \brief A MinidumpSimpleStringDictionary pointing to strings interpreted as
   //!     key-value pairs.
   //!

snapshot/mac/process_snapshot_mac.cc

@@ -25,6 +25,7 @@ ProcessSnapshotMac::ProcessSnapshotMac()
       modules_(),
       exception_(),
       process_reader_(),
+      client_id_(),
       annotations_simple_map_(),
       snapshot_time_(),
       initialized_() {
@@ -137,6 +138,11 @@ void ProcessSnapshotMac::ProcessCPUTimes(timeval* user_time,
   process_reader_.CPUTimes(user_time, system_time);
 }
 
+void ProcessSnapshotMac::ClientID(UUID* client_id) const {
+  INITIALIZATION_STATE_DCHECK_VALID(initialized_);
+  *client_id = client_id_;
+}
+
 const std::map<std::string, std::string>&
 ProcessSnapshotMac::AnnotationsSimpleMap() const {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);

snapshot/mac/process_snapshot_mac.h

@@ -38,6 +38,7 @@
 #include "snapshot/system_snapshot.h"
 #include "snapshot/thread_snapshot.h"
 #include "util/misc/initialization_state_dcheck.h"
+#include "util/misc/uuid.h"
 #include "util/stdlib/pointer_container.h"
 
 namespace crashpad {
@@ -76,6 +77,13 @@ class ProcessSnapshotMac final : public ProcessSnapshot {
                            const natural_t* state,
                            mach_msg_type_number_t state_count);
 
+  //! \brief Sets the value to be returned by ClientID().
+  //!
+  //! On Mac OS X, the client ID is under the control of the snapshot producer,
+  //! which may call this method to set the client ID. If this is not done,
+  //! ClientID() will return an identifier consisting entirely of zeroes.
+  void SetClientID(const UUID& client_id) { client_id_ = client_id; }
+
   //! \brief Sets the value to be returned by AnnotationsSimpleMap().
   //!
   //! On Mac OS X, all process annotations are under the control of the snapshot
@@ -101,6 +109,7 @@ class ProcessSnapshotMac final : public ProcessSnapshot {
   void SnapshotTime(timeval* snapshot_time) const override;
   void ProcessStartTime(timeval* start_time) const override;
   void ProcessCPUTimes(timeval* user_time, timeval* system_time) const override;
+  void ClientID(UUID* client_id) const override;
   const std::map<std::string, std::string>& AnnotationsSimpleMap()
       const override;
   const SystemSnapshot* System() const override;
@@ -120,6 +129,7 @@ class ProcessSnapshotMac final : public ProcessSnapshot {
   PointerVector<internal::ModuleSnapshotMac> modules_;
   scoped_ptr<internal::ExceptionSnapshotMac> exception_;
   ProcessReader process_reader_;
+  UUID client_id_;
   std::map<std::string, std::string> annotations_simple_map_;
   timeval snapshot_time_;
   InitializationStateDcheck initialized_;

snapshot/minidump/process_snapshot_minidump.cc

@@ -127,6 +127,11 @@ void ProcessSnapshotMinidump::ProcessCPUTimes(timeval* user_time,
   system_time->tv_usec = 0;
 }
 
+void ProcessSnapshotMinidump::ClientID(UUID* client_id) const {
+  INITIALIZATION_STATE_DCHECK_VALID(initialized_);
+  *client_id = crashpad_info_.client_id;
+}
+
 const std::map<std::string, std::string>&
 ProcessSnapshotMinidump::AnnotationsSimpleMap() const {
   // TODO(mark): This method should not be const, although the interface

snapshot/minidump/process_snapshot_minidump.h

@@ -33,6 +33,7 @@
 #include "snapshot/thread_snapshot.h"
 #include "util/file/file_reader.h"
 #include "util/misc/initialization_state_dcheck.h"
+#include "util/misc/uuid.h"
 #include "util/stdlib/pointer_container.h"
 
 namespace crashpad {
@@ -59,6 +60,7 @@ class ProcessSnapshotMinidump final : public ProcessSnapshot {
   void SnapshotTime(timeval* snapshot_time) const override;
   void ProcessStartTime(timeval* start_time) const override;
   void ProcessCPUTimes(timeval* user_time, timeval* system_time) const override;
+  void ClientID(UUID* client_id) const override;
   const std::map<std::string, std::string>& AnnotationsSimpleMap()
       const override;
   const SystemSnapshot* System() const override;

snapshot/minidump/process_snapshot_minidump_test.cc

@@ -56,6 +56,12 @@ TEST(ProcessSnapshotMinidump, Empty) {
 
   ProcessSnapshotMinidump process_snapshot;
   EXPECT_TRUE(process_snapshot.Initialize(&string_file));
+
+  UUID client_id;
+  process_snapshot.ClientID(&client_id);
+  EXPECT_EQ(UUID(), client_id);
+
+  EXPECT_TRUE(process_snapshot.AnnotationsSimpleMap().empty());
 }
 
 // Writes |string| to |writer| as a MinidumpUTF8String, and returns the file
@@ -124,6 +130,47 @@ void WriteMinidumpStringList(MINIDUMP_LOCATION_DESCRIPTOR* location,
                                              rvas.size() * sizeof(RVA));
 }
 
+TEST(ProcessSnapshotMinidump, ClientID) {
+  StringFile string_file;
+
+  MINIDUMP_HEADER header = {};
+  EXPECT_TRUE(string_file.Write(&header, sizeof(header)));
+
+  UUID client_id;
+  ASSERT_TRUE(
+      client_id.InitializeFromString("0001f4a9-d00d-5155-0a55-c0ffeec0ffee"));
+
+  MinidumpCrashpadInfo crashpad_info = {};
+  crashpad_info.version = MinidumpCrashpadInfo::kVersion;
+  crashpad_info.client_id = client_id;
+
+  MINIDUMP_DIRECTORY crashpad_info_directory = {};
+  crashpad_info_directory.StreamType = kMinidumpStreamTypeCrashpadInfo;
+  crashpad_info_directory.Location.Rva =
+      static_cast<RVA>(string_file.SeekGet());
+  EXPECT_TRUE(string_file.Write(&crashpad_info, sizeof(crashpad_info)));
+  crashpad_info_directory.Location.DataSize = sizeof(crashpad_info);
+
+  header.StreamDirectoryRva = static_cast<RVA>(string_file.SeekGet());
+  EXPECT_TRUE(string_file.Write(&crashpad_info_directory,
+                                sizeof(crashpad_info_directory)));
+
+  header.Signature = MINIDUMP_SIGNATURE;
+  header.Version = MINIDUMP_VERSION;
+  header.NumberOfStreams = 1;
+  EXPECT_TRUE(string_file.SeekSet(0));
+  EXPECT_TRUE(string_file.Write(&header, sizeof(header)));
+
+  ProcessSnapshotMinidump process_snapshot;
+  EXPECT_TRUE(process_snapshot.Initialize(&string_file));
+
+  UUID actual_client_id;
+  process_snapshot.ClientID(&actual_client_id);
+  EXPECT_EQ(client_id, actual_client_id);
+
+  EXPECT_TRUE(process_snapshot.AnnotationsSimpleMap().empty());
+}
+
 TEST(ProcessSnapshotMinidump, AnnotationsSimpleMap) {
   StringFile string_file;
 
@@ -159,6 +206,10 @@ TEST(ProcessSnapshotMinidump, AnnotationsSimpleMap) {
   ProcessSnapshotMinidump process_snapshot;
   EXPECT_TRUE(process_snapshot.Initialize(&string_file));
 
+  UUID client_id;
+  process_snapshot.ClientID(&client_id);
+  EXPECT_EQ(UUID(), client_id);
+
   const auto annotations_simple_map = process_snapshot.AnnotationsSimpleMap();
   EXPECT_EQ(dictionary, annotations_simple_map);
 }

snapshot/process_snapshot.h

@@ -22,6 +22,8 @@
 #include <string>
 #include <vector>
 
+#include "util/misc/uuid.h"
+
 namespace crashpad {
 
 class ExceptionSnapshot;
@@ -77,6 +79,17 @@ class ProcessSnapshot {
   virtual void ProcessCPUTimes(timeval* user_time,
                                timeval* system_time) const = 0;
 
+  //! \brief Returns a %UUID identifying the client that the snapshot
+  //!     represents.
+  //!
+  //! Client identification is within the scope of the application, but it is
+  //! expected that the identifier will be unique for an instance of Crashpad
+  //! monitoring an application or set of applications for a user. The
+  //! identifier shall remain stable over time.
+  //!
+  //! If no identifier is available, this field will contain zeroes.
+  virtual void ClientID(UUID* client_id) const = 0;
+
   //! \brief Returns key-value string annotations recorded for the process,
   //!     system, or snapshot producer.
   //!

snapshot/test/test_process_snapshot.cc

@@ -26,6 +26,7 @@ TestProcessSnapshot::TestProcessSnapshot()
       process_start_time_(),
       process_cpu_user_time_(),
       process_cpu_system_time_(),
+      client_id_(),
       annotations_simple_map_(),
       system_(),
       threads_(),
@@ -58,6 +59,10 @@ void TestProcessSnapshot::ProcessCPUTimes(timeval* user_time,
   *system_time = process_cpu_system_time_;
 }
 
+void TestProcessSnapshot::ClientID(UUID* client_id) const {
+  *client_id = client_id_;
+}
+
 const std::map<std::string, std::string>&
 TestProcessSnapshot::AnnotationsSimpleMap() const {
   return annotations_simple_map_;

snapshot/test/test_process_snapshot.h

@@ -30,6 +30,7 @@
 #include "snapshot/process_snapshot.h"
 #include "snapshot/system_snapshot.h"
 #include "snapshot/thread_snapshot.h"
+#include "util/misc/uuid.h"
 #include "util/stdlib/pointer_container.h"
 
 namespace crashpad {
@@ -57,6 +58,7 @@ class TestProcessSnapshot final : public ProcessSnapshot {
     process_cpu_user_time_ = user_time;
     process_cpu_system_time_ = system_time;
   }
+  void SetClientID(const UUID& client_id) { client_id_ = client_id; }
   void SetAnnotationsSimpleMap(
       const std::map<std::string, std::string>& annotations_simple_map) {
     annotations_simple_map_ = annotations_simple_map;
@@ -99,6 +101,7 @@ class TestProcessSnapshot final : public ProcessSnapshot {
   void SnapshotTime(timeval* snapshot_time) const override;
   void ProcessStartTime(timeval* start_time) const override;
   void ProcessCPUTimes(timeval* user_time, timeval* system_time) const override;
+  void ClientID(UUID* client_id) const override;
   const std::map<std::string, std::string>& AnnotationsSimpleMap()
       const override;
   const SystemSnapshot* System() const override;
@@ -113,6 +116,7 @@ class TestProcessSnapshot final : public ProcessSnapshot {
   timeval process_start_time_;
   timeval process_cpu_user_time_;
   timeval process_cpu_system_time_;
+  UUID client_id_;
   std::map<std::string, std::string> annotations_simple_map_;
   scoped_ptr<SystemSnapshot> system_;
   PointerVector<ThreadSnapshot> threads_;