Add MinidumpCrashpadInfo::simple_annotations.

Some annotations will exist at a broader scope than per-module, which is
the only place that annotations can currently be stored. The product
name and version are not under the control of any module, but are
established when the first Crashpad client establishes a handler. These
annotations will be stored in a minidump’s MinidumpCrashpadInfo
structure, which applies to the entire minidump.

Within the snapshot interface, this data is carried within the
“process” snapshot because it is the top-level structure in that family.
Note that the data may not correspond directly with a process, however.

TEST=minidump_test MinidumpCrashpadInfoWriter.*
R=rsesek@chromium.org

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

minidump/minidump_crashpad_info_writer.cc

@@ -16,13 +16,17 @@
 
 #include "base/logging.h"
 #include "minidump/minidump_module_crashpad_info_writer.h"
+#include "minidump/minidump_simple_string_dictionary_writer.h"
 #include "snapshot/process_snapshot.h"
 #include "util/file/file_writer.h"
 
 namespace crashpad {
 
 MinidumpCrashpadInfoWriter::MinidumpCrashpadInfoWriter()
-    : MinidumpStreamWriter(), crashpad_info_(), module_list_(nullptr) {
+    : MinidumpStreamWriter(),
+      crashpad_info_(),
+      simple_annotations_(nullptr),
+      module_list_(nullptr) {
   crashpad_info_.version = MinidumpCrashpadInfo::kVersion;
 }
 
@@ -34,6 +38,14 @@ void MinidumpCrashpadInfoWriter::InitializeFromSnapshot(
   DCHECK_EQ(state(), kStateMutable);
   DCHECK(!module_list_);
 
+  auto simple_annotations =
+      make_scoped_ptr(new MinidumpSimpleStringDictionaryWriter());
+  simple_annotations->InitializeFromMap(
+      process_snapshot->AnnotationsSimpleMap());
+  if (simple_annotations->IsUseful()) {
+    SetSimpleAnnotations(simple_annotations.Pass());
+  }
+
   auto modules = make_scoped_ptr(new MinidumpModuleCrashpadInfoListWriter());
   modules->InitializeFromSnapshot(process_snapshot->Modules());
 
@@ -42,6 +54,13 @@ void MinidumpCrashpadInfoWriter::InitializeFromSnapshot(
   }
 }
 
+void MinidumpCrashpadInfoWriter::SetSimpleAnnotations(
+    scoped_ptr<MinidumpSimpleStringDictionaryWriter> simple_annotations) {
+  DCHECK_EQ(state(), kStateMutable);
+
+  simple_annotations_ = simple_annotations.Pass();
+}
+
 void MinidumpCrashpadInfoWriter::SetModuleList(
     scoped_ptr<MinidumpModuleCrashpadInfoListWriter> module_list) {
   DCHECK_EQ(state(), kStateMutable);
@@ -56,6 +75,10 @@ bool MinidumpCrashpadInfoWriter::Freeze() {
     return false;
   }
 
+  if (simple_annotations_) {
+    simple_annotations_->RegisterLocationDescriptor(
+        &crashpad_info_.simple_annotations);
+  }
   if (module_list_) {
     module_list_->RegisterLocationDescriptor(&crashpad_info_.module_list);
   }
@@ -74,6 +97,9 @@ MinidumpCrashpadInfoWriter::Children() {
   DCHECK_GE(state(), kStateFrozen);
 
   std::vector<MinidumpWritable*> children;
+  if (simple_annotations_) {
+    children.push_back(simple_annotations_.get());
+  }
   if (module_list_) {
     children.push_back(module_list_.get());
   }
@@ -92,7 +118,7 @@ MinidumpStreamType MinidumpCrashpadInfoWriter::StreamType() const {
 }
 
 bool MinidumpCrashpadInfoWriter::IsUseful() const {
-  return module_list_;
+  return simple_annotations_ || module_list_;
 }
 
 }  // namespace crashpad

minidump/minidump_crashpad_info_writer.h

@@ -25,6 +25,7 @@
 namespace crashpad {
 
 class MinidumpModuleCrashpadInfoListWriter;
+class MinidumpSimpleStringDictionaryWriter;
 class ProcessSnapshot;
 
 //! \brief The writer for a MinidumpCrashpadInfo stream in a minidump file.
@@ -50,6 +51,17 @@ class MinidumpCrashpadInfoWriter final : public internal::MinidumpStreamWriter {
   //!     methods after this method.
   void InitializeFromSnapshot(const ProcessSnapshot* process_snapshot);
 
+  //! \brief Arranges for MinidumpCrashpadInfo::simple_annotations to point to
+  //!     the MinidumpSimpleStringDictionaryWriter object to be written by \a
+  //!     simple_annotations.
+  //!
+  //! This object takes ownership of \a simple_annotations and becomes its
+  //! parent in the overall tree of internal::MinidumpWritable objects.
+  //!
+  //! \note Valid in #kStateMutable.
+  void SetSimpleAnnotations(
+      scoped_ptr<MinidumpSimpleStringDictionaryWriter> simple_annotations);
+
   //! \brief Arranges for MinidumpCrashpadInfo::module_list to point to the
   //!     MinidumpModuleCrashpadInfoList object to be written by \a
   //!     module_list.
@@ -82,6 +94,7 @@ class MinidumpCrashpadInfoWriter final : public internal::MinidumpStreamWriter {
 
  private:
   MinidumpCrashpadInfo crashpad_info_;
+  scoped_ptr<MinidumpSimpleStringDictionaryWriter> simple_annotations_;
   scoped_ptr<MinidumpModuleCrashpadInfoListWriter> module_list_;
 
   DISALLOW_COPY_AND_ASSIGN(MinidumpCrashpadInfoWriter);

minidump/minidump_crashpad_info_writer_test.cc

@@ -17,10 +17,14 @@
 #include <windows.h>
 #include <dbghelp.h>
 
+#include <map>
+#include <string>
+
 #include "gtest/gtest.h"
 #include "minidump/minidump_extensions.h"
 #include "minidump/minidump_file_writer.h"
 #include "minidump/minidump_module_crashpad_info_writer.h"
+#include "minidump/minidump_simple_string_dictionary_writer.h"
 #include "minidump/test/minidump_file_writer_test_util.h"
 #include "minidump/test/minidump_string_writer_test_util.h"
 #include "minidump/test/minidump_writable_test_util.h"
@@ -32,9 +36,11 @@ namespace crashpad {
 namespace test {
 namespace {
 
-void GetCrashpadInfoStream(const std::string& file_contents,
-                           const MinidumpCrashpadInfo** crashpad_info,
-                           const MinidumpModuleCrashpadInfoList** module_list) {
+void GetCrashpadInfoStream(
+    const std::string& file_contents,
+    const MinidumpCrashpadInfo** crashpad_info,
+    const MinidumpSimpleStringDictionary** simple_annotations,
+    const MinidumpModuleCrashpadInfoList** module_list) {
   const MINIDUMP_DIRECTORY* directory;
   const MINIDUMP_HEADER* header =
       MinidumpHeaderAtStart(file_contents, &directory);
@@ -47,6 +53,10 @@ void GetCrashpadInfoStream(const std::string& file_contents,
       file_contents, directory[0].Location);
   ASSERT_TRUE(*crashpad_info);
 
+  *simple_annotations =
+      MinidumpWritableAtLocationDescriptor<MinidumpSimpleStringDictionary>(
+          file_contents, (*crashpad_info)->simple_annotations);
+
   *module_list =
       MinidumpWritableAtLocationDescriptor<MinidumpModuleCrashpadInfoList>(
           file_contents, (*crashpad_info)->module_list);
@@ -63,13 +73,62 @@ TEST(MinidumpCrashpadInfoWriter, Empty) {
   ASSERT_TRUE(minidump_file_writer.WriteEverything(&file_writer));
 
   const MinidumpCrashpadInfo* crashpad_info = nullptr;
+  const MinidumpSimpleStringDictionary* simple_annotations = nullptr;
   const MinidumpModuleCrashpadInfoList* module_list = nullptr;
 
   ASSERT_NO_FATAL_FAILURE(GetCrashpadInfoStream(
-      file_writer.string(), &crashpad_info, &module_list));
+      file_writer.string(), &crashpad_info, &simple_annotations, &module_list));
+
+  EXPECT_EQ(MinidumpCrashpadInfo::kVersion, crashpad_info->version);
+  EXPECT_FALSE(simple_annotations);
+  EXPECT_FALSE(module_list);
+}
+
+TEST(MinidumpCrashpadInfoWriter, SimpleAnnotations) {
+  MinidumpFileWriter minidump_file_writer;
+  auto crashpad_info_writer = make_scoped_ptr(new MinidumpCrashpadInfoWriter());
+
+  const char kKey[] =
+      "a thing that provides a means of gaining access to or understanding "
+      "something";
+  const char kValue[] =
+      "the numerical amount denoted by an algebraic term; a magnitude, "
+      "quantity, or number";
+  auto simple_string_dictionary_writer =
+      make_scoped_ptr(new MinidumpSimpleStringDictionaryWriter());
+  auto simple_string_dictionary_entry_writer =
+      make_scoped_ptr(new MinidumpSimpleStringDictionaryEntryWriter());
+  simple_string_dictionary_entry_writer->SetKeyValue(kKey, kValue);
+  simple_string_dictionary_writer->AddEntry(
+      simple_string_dictionary_entry_writer.Pass());
+  crashpad_info_writer->SetSimpleAnnotations(
+      simple_string_dictionary_writer.Pass());
+
+  EXPECT_TRUE(crashpad_info_writer->IsUseful());
+
+  minidump_file_writer.AddStream(crashpad_info_writer.Pass());
+
+  StringFileWriter file_writer;
+  ASSERT_TRUE(minidump_file_writer.WriteEverything(&file_writer));
+
+  const MinidumpCrashpadInfo* crashpad_info = nullptr;
+  const MinidumpSimpleStringDictionary* simple_annotations = nullptr;
+  const MinidumpModuleCrashpadInfoList* module_list = nullptr;
+
+  ASSERT_NO_FATAL_FAILURE(GetCrashpadInfoStream(
+      file_writer.string(), &crashpad_info, &simple_annotations, &module_list));
 
   EXPECT_EQ(MinidumpCrashpadInfo::kVersion, crashpad_info->version);
   EXPECT_FALSE(module_list);
+
+  ASSERT_TRUE(simple_annotations);
+  ASSERT_EQ(1u, simple_annotations->count);
+  EXPECT_EQ(kKey,
+            MinidumpUTF8StringAtRVAAsString(
+                file_writer.string(), simple_annotations->entries[0].key));
+  EXPECT_EQ(kValue,
+            MinidumpUTF8StringAtRVAAsString(
+                file_writer.string(), simple_annotations->entries[0].value));
 }
 
 TEST(MinidumpCrashpadInfoWriter, CrashpadModuleList) {
@@ -93,12 +152,15 @@ TEST(MinidumpCrashpadInfoWriter, CrashpadModuleList) {
   ASSERT_TRUE(minidump_file_writer.WriteEverything(&file_writer));
 
   const MinidumpCrashpadInfo* crashpad_info = nullptr;
-  const MinidumpModuleCrashpadInfoList* module_list;
+  const MinidumpSimpleStringDictionary* simple_annotations = nullptr;
+  const MinidumpModuleCrashpadInfoList* module_list = nullptr;
 
   ASSERT_NO_FATAL_FAILURE(GetCrashpadInfoStream(
-      file_writer.string(), &crashpad_info, &module_list));
+      file_writer.string(), &crashpad_info, &simple_annotations, &module_list));
 
   EXPECT_EQ(MinidumpCrashpadInfo::kVersion, crashpad_info->version);
+  EXPECT_FALSE(simple_annotations);
+
   ASSERT_TRUE(module_list);
   ASSERT_EQ(1u, module_list->count);
 
@@ -116,6 +178,8 @@ TEST(MinidumpCrashpadInfoWriter, CrashpadModuleList) {
 }
 
 TEST(MinidumpCrashpadInfoWriter, InitializeFromSnapshot) {
+  const char kKey[] = "version";
+  const char kValue[] = "40.0.2214.111";
   const char kEntry[] = "This is a simple annotation in a list.";
 
   // Test with a useless module, one that doesn’t carry anything that would
@@ -132,6 +196,10 @@ TEST(MinidumpCrashpadInfoWriter, InitializeFromSnapshot) {
   // Try again with a useful module.
   process_snapshot.reset(new TestProcessSnapshot());
 
+  std::map<std::string, std::string> annotations_simple_map;
+  annotations_simple_map[kKey] = kValue;
+  process_snapshot->SetAnnotationsSimpleMap(annotations_simple_map);
+
   module_snapshot.reset(new TestModuleSnapshot());
   std::vector<std::string> annotations_list(1, std::string(kEntry));
   module_snapshot->SetAnnotationsVector(annotations_list);
@@ -148,11 +216,22 @@ TEST(MinidumpCrashpadInfoWriter, InitializeFromSnapshot) {
   ASSERT_TRUE(minidump_file_writer.WriteEverything(&file_writer));
 
   const MinidumpCrashpadInfo* info = nullptr;
+  const MinidumpSimpleStringDictionary* simple_annotations;
   const MinidumpModuleCrashpadInfoList* module_list;
   ASSERT_NO_FATAL_FAILURE(GetCrashpadInfoStream(
-      file_writer.string(), &info, &module_list));
+      file_writer.string(), &info, &simple_annotations, &module_list));
 
   EXPECT_EQ(MinidumpCrashpadInfo::kVersion, info->version);
+
+  ASSERT_TRUE(simple_annotations);
+  ASSERT_EQ(1u, simple_annotations->count);
+  EXPECT_EQ(kKey,
+            MinidumpUTF8StringAtRVAAsString(
+                file_writer.string(), simple_annotations->entries[0].key));
+  EXPECT_EQ(kValue,
+            MinidumpUTF8StringAtRVAAsString(
+                file_writer.string(), simple_annotations->entries[0].value));
+
   ASSERT_TRUE(module_list);
   ASSERT_EQ(1u, module_list->count);
 
@@ -171,13 +250,13 @@ TEST(MinidumpCrashpadInfoWriter, InitializeFromSnapshot) {
 
   ASSERT_EQ(1u, list_annotations->count);
   EXPECT_EQ(kEntry,
-            MinidumpUTF8StringAtRVAAsString(
-                file_writer.string(), list_annotations->children[0]));
+            MinidumpUTF8StringAtRVAAsString(file_writer.string(),
+                                            list_annotations->children[0]));
 
-  const MinidumpSimpleStringDictionary* simple_annotations =
+  const MinidumpSimpleStringDictionary* module_simple_annotations =
       MinidumpWritableAtLocationDescriptor<MinidumpSimpleStringDictionary>(
           file_writer.string(), module->simple_annotations);
-  EXPECT_FALSE(simple_annotations);
+  EXPECT_FALSE(module_simple_annotations);
 }
 
 }  // namespace

minidump/minidump_extensions.h

@@ -456,6 +456,15 @@ struct ALIGNAS(4) PACKED MinidumpCrashpadInfo {
   //! no need for any fields present in later versions.
   uint32_t version;
 
+  //! \brief A MinidumpSimpleStringDictionary pointing to strings interpreted as
+  //!     key-value pairs.
+  //!
+  //! These key-value pairs correspond to
+  //! ProcessSnapshot::AnnotationsSimpleMap().
+  //!
+  //! This field is present when #version is at least `1`.
+  MINIDUMP_LOCATION_DESCRIPTOR simple_annotations;
+
   //! \brief A pointer to a #MinidumpModuleCrashpadInfoList structure.
   //!
   //! This field is present when #version is at least `1`.

snapshot/mac/process_snapshot_mac.cc

@@ -23,6 +23,7 @@ ProcessSnapshotMac::ProcessSnapshotMac()
       modules_(),
       exception_(),
       process_reader_(),
+      annotations_simple_map_(),
       snapshot_time_(),
       initialized_() {
 }
@@ -104,6 +105,11 @@ void ProcessSnapshotMac::ProcessCPUTimes(timeval* user_time,
   process_reader_.CPUTimes(user_time, system_time);
 }
 
+const std::map<std::string, std::string>&
+ProcessSnapshotMac::AnnotationsSimpleMap() const {
+  return annotations_simple_map_;
+}
+
 const SystemSnapshot* ProcessSnapshotMac::System() const {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
   return &system_;

snapshot/mac/process_snapshot_mac.h

@@ -19,6 +19,8 @@
 #include <sys/time.h>
 #include <unistd.h>
 
+#include <map>
+#include <string>
 #include <vector>
 
 #include "base/basictypes.h"
@@ -72,6 +74,17 @@ class ProcessSnapshotMac final : public ProcessSnapshot {
                            const natural_t* state,
                            mach_msg_type_number_t state_count);
 
+  //! \brief Sets the value to be returned by AnnotationsSimpleMap().
+  //!
+  //! On Mac OS X, all process annotations are under the control of the snapshot
+  //! producer, which may call this method to establish these annotations.
+  //! Contrast this with module annotations, which are under the control of the
+  //! process being snapshotted.
+  void SetAnnotationsSimpleMap(
+      const std::map<std::string, std::string>& annotations_simple_map) {
+    annotations_simple_map_ = annotations_simple_map;
+  }
+
   // ProcessSnapshot:
 
   pid_t ProcessID() const override;
@@ -79,6 +92,8 @@ 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;
+  const std::map<std::string, std::string>& AnnotationsSimpleMap()
+      const override;
   const SystemSnapshot* System() const override;
   std::vector<const ThreadSnapshot*> Threads() const override;
   std::vector<const ModuleSnapshot*> Modules() const override;
@@ -96,6 +111,7 @@ class ProcessSnapshotMac final : public ProcessSnapshot {
   PointerVector<internal::ModuleSnapshotMac> modules_;
   scoped_ptr<internal::ExceptionSnapshotMac> exception_;
   ProcessReader process_reader_;
+  std::map<std::string, std::string> annotations_simple_map_;
   timeval snapshot_time_;
   InitializationStateDcheck initialized_;
 

snapshot/module_snapshot.h

@@ -142,10 +142,14 @@ class ModuleSnapshot {
   //!
   //! For Mac OS X snapshots, these annotations are found by interpreting the
   //! `__DATA, __crashpad_info` section as `CrashpadInfo`. Clients can use the
-  //! Crashpad client interface to store annotations in this structure.
+  //! Crashpad client interface to store annotations in this structure. Most
+  //! annotations under the client’s direct control will be retrievable by this
+  //! method. For clients such as Chrome, this includes the process type.
   //!
   //! The annotations returned by this method do not duplicate those returned by
-  //! AnnotationsVector().
+  //! AnnotationsVector(). Additional annotations related to the process,
+  //! system, or snapshot producer may be obtained by calling
+  //! ProcessSnapshot::AnnotationsSimpleMap().
   virtual std::map<std::string, std::string> AnnotationsSimpleMap() const = 0;
 };
 

snapshot/process_snapshot.h

@@ -18,6 +18,8 @@
 #include <sys/time.h>
 #include <sys/types.h>
 
+#include <map>
+#include <string>
 #include <vector>
 
 namespace crashpad {
@@ -75,6 +77,32 @@ class ProcessSnapshot {
   virtual void ProcessCPUTimes(timeval* user_time,
                                timeval* system_time) const = 0;
 
+  //! \brief Returns key-value string annotations recorded for the process,
+  //!     system, or snapshot producer.
+  //!
+  //! This method retrieves annotations recorded for a process. These
+  //! annotations are intended for diagnostic use, including crash analysis.
+  //! “Simple annotations” are structured as a sequence of key-value pairs,
+  //! where all keys and values are strings. These are referred to in Chrome as
+  //! “crash keys.”
+  //!
+  //! Annotations stored here may reflect the process, system, or snapshot
+  //! producer. Most annotations not under the client’s direct control will be
+  //! retrievable by this method. For clients such as Chrome, this includes the
+  //! product name and version.
+  //!
+  //! Additional per-module annotations may be obtained by calling
+  //! ModuleSnapshot::AnnotationsSimpleMap().
+  //
+  // This interface currently returns a const& because all implementations store
+  // the data within their objects in this format, and are therefore able to
+  // provide this access without requiring a copy. Future implementations may
+  // obtain data on demand or store data in a different format, at which point
+  // the cost of maintaining this data in ProcessSnapshot subclass objects will
+  // need to be taken into account, and this interface possibly revised.
+  virtual const std::map<std::string, std::string>& AnnotationsSimpleMap()
+      const = 0;
+
   //! \brief Returns a SystemSnapshot reflecting the characteristics of the
   //!     system that ran the snapshot process at the time of the snapshot.
   //!

snapshot/test/test_process_snapshot.cc

@@ -26,6 +26,7 @@ TestProcessSnapshot::TestProcessSnapshot()
       process_start_time_(),
       process_cpu_user_time_(),
       process_cpu_system_time_(),
+      annotations_simple_map_(),
       system_(),
       threads_(),
       modules_(),
@@ -57,6 +58,11 @@ void TestProcessSnapshot::ProcessCPUTimes(timeval* user_time,
   *system_time = process_cpu_system_time_;
 }
 
+const std::map<std::string, std::string>&
+TestProcessSnapshot::AnnotationsSimpleMap() const {
+  return annotations_simple_map_;
+}
+
 const SystemSnapshot* TestProcessSnapshot::System() const {
   return system_.get();
 }

snapshot/test/test_process_snapshot.h

@@ -19,6 +19,8 @@
 #include <sys/time.h>
 #include <sys/types.h>
 
+#include <map>
+#include <string>
 #include <vector>
 
 #include "base/basictypes.h"
@@ -55,6 +57,10 @@ class TestProcessSnapshot final : public ProcessSnapshot {
     process_cpu_user_time_ = user_time;
     process_cpu_system_time_ = system_time;
   }
+  void SetAnnotationsSimpleMap(
+      const std::map<std::string, std::string>& annotations_simple_map) {
+    annotations_simple_map_ = annotations_simple_map;
+  }
 
   //! \brief Sets the system snapshot to be returned by System().
   //!
@@ -93,6 +99,8 @@ 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;
+  const std::map<std::string, std::string>& AnnotationsSimpleMap()
+      const override;
   const SystemSnapshot* System() const override;
   std::vector<const ThreadSnapshot*> Threads() const override;
   std::vector<const ModuleSnapshot*> Modules() const override;
@@ -105,6 +113,7 @@ class TestProcessSnapshot final : public ProcessSnapshot {
   timeval process_start_time_;
   timeval process_cpu_user_time_;
   timeval process_cpu_system_time_;
+  std::map<std::string, std::string> annotations_simple_map_;
   scoped_ptr<SystemSnapshot> system_;
   PointerVector<ThreadSnapshot> threads_;
   PointerVector<ModuleSnapshot> modules_;