Add MinidumpCrashpadInfo::report_id.

Now that Chrome’s about:crashes displays the crash report UUID, I wanted
to add it to the minidump. In the future, we may be able to index these
on the server. This will also help identify dumps that correspond to the
same event once we’re equipped to convert between different formats.

Ideally, this new field is populated with the same UUID used locally in
the crash report database. To make this work,
CrashReportDatabase::NewReport must carry the UUID. This was actually
part of CrashReportDatabaseWin’s private extension to NewReport, so that
extension subclass can now be cleaned up.

TEST=crashpad_minidump_test MinidumpCrashpadInfoWriter.*,
     crashpad_client_test CrashReportDatabaseTest.NewCrashReport

R=rsesek@chromium.org

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

client/crash_report_database.h

@@ -98,6 +98,10 @@ class CrashReportDatabase {
     //! The file handle to which the report should be written.
     FileHandle handle;
 
+    //! A unique identifier by which this report will always be known to the
+    //! database.
+    UUID uuid;
+
     //! The path to the crash report being written.
     base::FilePath path;
   };

client/crash_report_database_mac.mm

@@ -238,15 +238,15 @@ CrashReportDatabase::OperationStatus
 CrashReportDatabaseMac::PrepareNewCrashReport(NewReport** out_report) {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
 
+  scoped_ptr<NewReport> report(new NewReport());
+
   uuid_t uuid_gen;
   uuid_generate(uuid_gen);
-  UUID uuid(uuid_gen);
+  report->uuid.InitializeFromBytes(uuid_gen);
-
-  scoped_ptr<NewReport> report(new NewReport());
 
   report->path =
       base_dir_.Append(kWriteDirectory)
-               .Append(uuid.ToString() + "." + kCrashReportFileExtension);
+          .Append(report->uuid.ToString() + "." + kCrashReportFileExtension);
 
   report->handle = HANDLE_EINTR(open(report->path.value().c_str(),
                                      O_CREAT | O_WRONLY | O_EXCL | O_EXLOCK,
@@ -257,7 +257,8 @@ CrashReportDatabaseMac::PrepareNewCrashReport(NewReport** out_report) {
   }
 
   // TODO(rsesek): Potentially use an fsetxattr() here instead.
-  if (!WriteXattr(report->path, XattrName(kXattrUUID), uuid.ToString())) {
+  if (!WriteXattr(
+          report->path, XattrName(kXattrUUID), report->uuid.ToString())) {
     PLOG_IF(ERROR, IGNORE_EINTR(close(report->handle)) != 0) << "close";
     return kDatabaseError;
   }
@@ -288,6 +289,11 @@ CrashReportDatabaseMac::FinishedWritingCrashReport(NewReport* report,
     return kDatabaseError;
   }
 
+  if (*uuid != report->uuid) {
+    LOG(ERROR) << "UUID mismatch for crash report " << report->path.value();
+    return kDatabaseError;
+  }
+
   // Record the creation time of this report.
   if (!WriteXattrTimeT(report->path, XattrName(kXattrCreationTime),
                        time(nullptr))) {

client/crash_report_database_test.cc

@@ -179,10 +179,12 @@ TEST_F(CrashReportDatabaseTest, NewCrashReport) {
   CrashReportDatabase::NewReport* new_report;
   EXPECT_EQ(CrashReportDatabase::kNoError,
             db()->PrepareNewCrashReport(&new_report));
+  UUID expect_uuid = new_report->uuid;
   EXPECT_TRUE(FileExistsAtPath(new_report->path)) << new_report->path.value();
   UUID uuid;
   EXPECT_EQ(CrashReportDatabase::kNoError,
             db()->FinishedWritingCrashReport(new_report, &uuid));
+  EXPECT_EQ(expect_uuid, uuid);
 
   CrashReportDatabase::Report report;
   EXPECT_EQ(CrashReportDatabase::kNoError,

client/crash_report_database_win.cc

@@ -174,14 +174,6 @@ ReportDisk::ReportDisk(const UUID& uuid,
   this->state = state;
 }
 
-//! \brief A private extension of the NewReport class to hold the UUID during
-//!     initial write. We don't store metadata in dump's file attributes, so we
-//!     use the UUID to identify the dump on write completion.
-struct NewReportDisk : public CrashReportDatabase::NewReport {
-  //! \brief The UUID for this crash report.
-  UUID uuid;
-};
-
 // Metadata --------------------------------------------------------------------
 
 //! \brief Manages the metadata for the set of reports, handling serialization
@@ -586,13 +578,13 @@ OperationStatus CrashReportDatabaseWin::PrepareNewCrashReport(
     return kFileSystemError;
   static_assert(sizeof(system_uuid) == 16, "unexpected system uuid size");
   static_assert(offsetof(::UUID, Data1) == 0, "unexpected uuid layout");
-  UUID uuid(reinterpret_cast<const uint8_t*>(&system_uuid.Data1));
 
-  scoped_ptr<NewReportDisk> new_report(new NewReportDisk());
+  scoped_ptr<NewReport> new_report(new NewReport());
-  new_report->uuid = uuid;
+  new_report->uuid.InitializeFromBytes(
-  new_report->path =
+      reinterpret_cast<const uint8_t*>(&system_uuid.Data1));
-      base_dir_.Append(kReportsDirectory)
+  new_report->path = base_dir_.Append(kReportsDirectory)
-          .Append(uuid.ToString16() + L"." + kCrashReportFileExtension);
+                         .Append(new_report->uuid.ToString16() + L"." +
+                                 kCrashReportFileExtension);
   new_report->handle = LoggingOpenFileForWrite(new_report->path,
                                                FileWriteMode::kCreateOrFail,
                                                FilePermissions::kOwnerOnly);
@@ -608,8 +600,8 @@ OperationStatus CrashReportDatabaseWin::FinishedWritingCrashReport(
     UUID* uuid) {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
 
-  // Take ownership of the report, and cast to our private version with UUID.
+  // Take ownership of the report.
-  scoped_ptr<NewReportDisk> scoped_report(static_cast<NewReportDisk*>(report));
+  scoped_ptr<NewReport> scoped_report(report);
   // Take ownership of the file handle.
   ScopedFileHandle handle(report->handle);
 
@@ -628,8 +620,8 @@ OperationStatus CrashReportDatabaseWin::ErrorWritingCrashReport(
     NewReport* report) {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
 
-  // Take ownership of the report, and cast to our private version with UUID.
+  // Take ownership of the report.
-  scoped_ptr<NewReportDisk> scoped_report(static_cast<NewReportDisk*>(report));
+  scoped_ptr<NewReport> scoped_report(report);
 
   // Close the outstanding handle.
   LoggingCloseFile(report->handle);

handler/mac/crash_report_exception_handler.cc

@@ -169,6 +169,8 @@ kern_return_t CrashReportExceptionHandler::CatchMachException(
       return KERN_FAILURE;
     }
 
+    process_snapshot.SetReportID(new_report->uuid);
+
     CallErrorWritingCrashReport call_error_writing_crash_report(database_,
                                                                 new_report);
 

minidump/minidump_crashpad_info_writer.cc

@@ -38,6 +38,10 @@ void MinidumpCrashpadInfoWriter::InitializeFromSnapshot(
   DCHECK_EQ(state(), kStateMutable);
   DCHECK(!module_list_);
 
+  UUID report_id;
+  process_snapshot->ReportID(&report_id);
+  SetReportID(report_id);
+
   UUID client_id;
   process_snapshot->ClientID(&client_id);
   SetClientID(client_id);
@@ -58,6 +62,12 @@ void MinidumpCrashpadInfoWriter::InitializeFromSnapshot(
   }
 }
 
+void MinidumpCrashpadInfoWriter::SetReportID(const UUID& report_id) {
+  DCHECK_EQ(state(), kStateMutable);
+
+  crashpad_info_.report_id = report_id;
+}
+
 void MinidumpCrashpadInfoWriter::SetClientID(const UUID& client_id) {
   DCHECK_EQ(state(), kStateMutable);
 
@@ -128,7 +138,8 @@ MinidumpStreamType MinidumpCrashpadInfoWriter::StreamType() const {
 }
 
 bool MinidumpCrashpadInfoWriter::IsUseful() const {
-  return crashpad_info_.client_id != UUID() ||
+  return crashpad_info_.report_id != UUID() ||
+         crashpad_info_.client_id != UUID() ||
          simple_annotations_ ||
          module_list_;
 }

minidump/minidump_crashpad_info_writer.h

@@ -52,6 +52,9 @@ class MinidumpCrashpadInfoWriter final : public internal::MinidumpStreamWriter {
   //!     methods after this method.
   void InitializeFromSnapshot(const ProcessSnapshot* process_snapshot);
 
+  //! \brief Sets MinidumpCrashpadInfo::report_id.
+  void SetReportID(const UUID& report_id);
+
   //! \brief Sets MinidumpCrashpadInfo::client_id.
   void SetClientID(const UUID& client_id);
 

minidump/minidump_crashpad_info_writer_test.cc

@@ -80,15 +80,21 @@ TEST(MinidumpCrashpadInfoWriter, Empty) {
       string_file.string(), &crashpad_info, &simple_annotations, &module_list));
 
   EXPECT_EQ(MinidumpCrashpadInfo::kVersion, crashpad_info->version);
+  EXPECT_EQ(UUID(), crashpad_info->report_id);
   EXPECT_EQ(UUID(), crashpad_info->client_id);
   EXPECT_FALSE(simple_annotations);
   EXPECT_FALSE(module_list);
 }
 
-TEST(MinidumpCrashpadInfoWriter, ClientID) {
+TEST(MinidumpCrashpadInfoWriter, ReportAndClientID) {
   MinidumpFileWriter minidump_file_writer;
   auto crashpad_info_writer = make_scoped_ptr(new MinidumpCrashpadInfoWriter());
 
+  UUID report_id;
+  ASSERT_TRUE(
+      report_id.InitializeFromString("01234567-89ab-cdef-0123-456789abcdef"));
+  crashpad_info_writer->SetReportID(report_id);
+
   UUID client_id;
   ASSERT_TRUE(
       client_id.InitializeFromString("00112233-4455-6677-8899-aabbccddeeff"));
@@ -109,6 +115,7 @@ TEST(MinidumpCrashpadInfoWriter, ClientID) {
       string_file.string(), &crashpad_info, &simple_annotations, &module_list));
 
   EXPECT_EQ(MinidumpCrashpadInfo::kVersion, crashpad_info->version);
+  EXPECT_EQ(report_id, crashpad_info->report_id);
   EXPECT_EQ(client_id, crashpad_info->client_id);
   EXPECT_FALSE(simple_annotations);
   EXPECT_FALSE(module_list);
@@ -208,6 +215,10 @@ TEST(MinidumpCrashpadInfoWriter, CrashpadModuleList) {
 }
 
 TEST(MinidumpCrashpadInfoWriter, InitializeFromSnapshot) {
+  UUID report_id;
+  ASSERT_TRUE(
+      report_id.InitializeFromString("fedcba98-7654-3210-fedc-ba9876543210"));
+
   UUID client_id;
   ASSERT_TRUE(
       client_id.InitializeFromString("fedcba98-7654-3210-0123-456789abcdef"));
@@ -230,6 +241,7 @@ TEST(MinidumpCrashpadInfoWriter, InitializeFromSnapshot) {
   // Try again with a useful module.
   process_snapshot.reset(new TestProcessSnapshot());
 
+  process_snapshot->SetReportID(report_id);
   process_snapshot->SetClientID(client_id);
 
   std::map<std::string, std::string> annotations_simple_map;
@@ -259,6 +271,7 @@ TEST(MinidumpCrashpadInfoWriter, InitializeFromSnapshot) {
 
   EXPECT_EQ(MinidumpCrashpadInfo::kVersion, info->version);
 
+  EXPECT_EQ(report_id, info->report_id);
   EXPECT_EQ(client_id, info->client_id);
 
   ASSERT_TRUE(simple_annotations);

minidump/minidump_extensions.h

@@ -459,6 +459,7 @@ struct ALIGNAS(4) PACKED MinidumpCrashpadInfo {
   // members, an explicit constructor must be provided.
   MinidumpCrashpadInfo()
       : version(),
+        report_id(),
         client_id(),
         simple_annotations(),
         module_list() {
@@ -480,6 +481,17 @@ struct ALIGNAS(4) PACKED MinidumpCrashpadInfo {
   //! no need for any fields present in later versions.
   uint32_t version;
 
+  //! \brief A %UUID identifying an individual crash report.
+  //!
+  //! This provides a stable identifier for a crash even as the report is
+  //! converted to different formats, provided that all formats support storing
+  //! a crash report ID.
+  //!
+  //! If no identifier is available, this field will contain zeroes.
+  //!
+  //! This field is present when #version is at least `1`.
+  UUID report_id;
+
   //! \brief A %UUID identifying the client that crashed.
   //!
   //! Client identification is within the scope of the application, but it is

snapshot/mac/process_snapshot_mac.cc

@@ -25,6 +25,7 @@ ProcessSnapshotMac::ProcessSnapshotMac()
       modules_(),
       exception_(),
       process_reader_(),
+      report_id_(),
       client_id_(),
       annotations_simple_map_(),
       snapshot_time_(),
@@ -138,6 +139,11 @@ void ProcessSnapshotMac::ProcessCPUTimes(timeval* user_time,
   process_reader_.CPUTimes(user_time, system_time);
 }
 
+void ProcessSnapshotMac::ReportID(UUID* report_id) const {
+  INITIALIZATION_STATE_DCHECK_VALID(initialized_);
+  *report_id = report_id_;
+}
+
 void ProcessSnapshotMac::ClientID(UUID* client_id) const {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
   *client_id = client_id_;

snapshot/mac/process_snapshot_mac.h

@@ -77,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 ReportID().
+  //!
+  //! On Mac OS X, the crash report ID is under the control of the snapshot
+  //! producer, which may call this method to set the report ID. If this is not
+  //! done, ReportID() will return an identifier consisting entirely of zeroes.
+  void SetReportID(const UUID& report_id) { report_id_ = report_id; }
+
   //! \brief Sets the value to be returned by ClientID().
   //!
   //! On Mac OS X, the client ID is under the control of the snapshot producer,
@@ -109,6 +116,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 ReportID(UUID* report_id) const override;
   void ClientID(UUID* client_id) const override;
   const std::map<std::string, std::string>& AnnotationsSimpleMap()
       const override;
@@ -129,6 +137,7 @@ class ProcessSnapshotMac final : public ProcessSnapshot {
   PointerVector<internal::ModuleSnapshotMac> modules_;
   scoped_ptr<internal::ExceptionSnapshotMac> exception_;
   ProcessReader process_reader_;
+  UUID report_id_;
   UUID client_id_;
   std::map<std::string, std::string> annotations_simple_map_;
   timeval snapshot_time_;

snapshot/minidump/process_snapshot_minidump.cc

@@ -127,6 +127,11 @@ void ProcessSnapshotMinidump::ProcessCPUTimes(timeval* user_time,
   system_time->tv_usec = 0;
 }
 
+void ProcessSnapshotMinidump::ReportID(UUID* report_id) const {
+  INITIALIZATION_STATE_DCHECK_VALID(initialized_);
+  *report_id = crashpad_info_.report_id;
+}
+
 void ProcessSnapshotMinidump::ClientID(UUID* client_id) const {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
   *client_id = crashpad_info_.client_id;

snapshot/minidump/process_snapshot_minidump.h

@@ -60,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 ReportID(UUID* report_id) const override;
   void ClientID(UUID* client_id) const override;
   const std::map<std::string, std::string>& AnnotationsSimpleMap()
       const override;

snapshot/process_snapshot.h

@@ -79,6 +79,17 @@ class ProcessSnapshot {
   virtual void ProcessCPUTimes(timeval* user_time,
                                timeval* system_time) const = 0;
 
+  //! \brief Returns a %UUID identifying the event that the snapshot describes.
+  //!
+  //! This provides a stable identifier for a crash even as the report is
+  //! converted to different formats, provided that all formats support storing
+  //! a crash report ID. When a report is originally created, a report ID should
+  //! be assigned. From that point on, any operations involving the same report
+  //! should preserve the same report ID.
+  //!
+  //! If no identifier is available, this field will contain zeroes.
+  virtual void ReportID(UUID* client_id) const = 0;
+
   //! \brief Returns a %UUID identifying the client that the snapshot
   //!     represents.
   //!

snapshot/test/test_process_snapshot.cc

@@ -26,6 +26,7 @@ TestProcessSnapshot::TestProcessSnapshot()
       process_start_time_(),
       process_cpu_user_time_(),
       process_cpu_system_time_(),
+      report_id_(),
       client_id_(),
       annotations_simple_map_(),
       system_(),
@@ -59,6 +60,10 @@ void TestProcessSnapshot::ProcessCPUTimes(timeval* user_time,
   *system_time = process_cpu_system_time_;
 }
 
+void TestProcessSnapshot::ReportID(UUID* report_id) const {
+  *report_id = report_id_;
+}
+
 void TestProcessSnapshot::ClientID(UUID* client_id) const {
   *client_id = client_id_;
 }

snapshot/test/test_process_snapshot.h

@@ -58,6 +58,7 @@ class TestProcessSnapshot final : public ProcessSnapshot {
     process_cpu_user_time_ = user_time;
     process_cpu_system_time_ = system_time;
   }
+  void SetReportID(const UUID& report_id) { report_id_ = report_id; }
   void SetClientID(const UUID& client_id) { client_id_ = client_id; }
   void SetAnnotationsSimpleMap(
       const std::map<std::string, std::string>& annotations_simple_map) {
@@ -101,6 +102,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 ReportID(UUID* report_id) const override;
   void ClientID(UUID* client_id) const override;
   const std::map<std::string, std::string>& AnnotationsSimpleMap()
       const override;
@@ -116,6 +118,7 @@ class TestProcessSnapshot final : public ProcessSnapshot {
   timeval process_start_time_;
   timeval process_cpu_user_time_;
   timeval process_cpu_system_time_;
+  UUID report_id_;
   UUID client_id_;
   std::map<std::string, std::string> annotations_simple_map_;
   scoped_ptr<SystemSnapshot> system_;