ios: Store Crashpad client annotations in the intermediate dump.

The iOS crashpad client was mistakenly setting the process annotations
(typically things like version and product name) when converting the
intermediate dump into a minidump.  This is incorrect, as those
annotations are determined at intermediate dump creation time.

Instead, correctly write those annotations during intermediate dump
creation. Passing extra annotations during intermediate dump to
minidump is still supported.

Bug: crashpad: 31
Change-Id: Ic5e29debdc123011d130f75a48345071575466d9
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3266127
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>

client/ios_handler/in_process_handler.cc

@@ -114,7 +114,7 @@ void InProcessHandler::DumpExceptionFromSignal(
     ucontext_t* context) {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
   {
-    ScopedReport report(writer_.get(), system_data);
+    ScopedReport report(writer_.get(), system_data, annotations_);
     InProcessIntermediateDumpHandler::WriteExceptionFromSignal(
         writer_.get(), system_data, siginfo, context);
   }
@@ -133,7 +133,7 @@ void InProcessHandler::DumpExceptionFromMachException(
     mach_msg_type_number_t old_state_count) {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
   {
-    ScopedReport report(writer_.get(), system_data);
+    ScopedReport report(writer_.get(), system_data, annotations_);
     InProcessIntermediateDumpHandler::WriteExceptionFromMachException(
         writer_.get(),
         behavior,
@@ -153,7 +153,8 @@ void InProcessHandler::DumpExceptionFromNSExceptionFrames(
     const uint64_t* frames,
     const size_t num_frames) {
   {
-    ScopedReport report(writer_.get(), system_data, frames, num_frames);
+    ScopedReport report(
+        writer_.get(), system_data, annotations_, frames, num_frames);
     InProcessIntermediateDumpHandler::WriteExceptionFromNSException(
         writer_.get());
   }
@@ -161,34 +162,14 @@ void InProcessHandler::DumpExceptionFromNSExceptionFrames(
 }
 
 void InProcessHandler::ProcessIntermediateDumps(
-    const std::map<std::string, std::string>& extra_annotations) {
+    const std::map<std::string, std::string>& annotations) {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
 
-  std::map<std::string, std::string> annotations(annotations_);
-  annotations.insert(extra_annotations.begin(), extra_annotations.end());
-
   for (auto& file : PendingFiles())
-    ProcessIntermediateDumpWithCompleteAnnotations(file, annotations);
+    ProcessIntermediateDump(file, annotations);
 }
 
 void InProcessHandler::ProcessIntermediateDump(
-    const base::FilePath& file,
-    const std::map<std::string, std::string>& extra_annotations) {
-  INITIALIZATION_STATE_DCHECK_VALID(initialized_);
-
-  std::map<std::string, std::string> annotations(annotations_);
-  annotations.insert(extra_annotations.begin(), extra_annotations.end());
-  ProcessIntermediateDumpWithCompleteAnnotations(file, annotations);
-}
-
-void InProcessHandler::StartProcessingPendingReports() {
-  if (!upload_thread_started_ && upload_thread_) {
-    upload_thread_->Start();
-    upload_thread_started_ = true;
-  }
-}
-
-void InProcessHandler::ProcessIntermediateDumpWithCompleteAnnotations(
     const base::FilePath& file,
     const std::map<std::string, std::string>& annotations) {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
@@ -199,6 +180,13 @@ void InProcessHandler::ProcessIntermediateDumpWithCompleteAnnotations(
   }
 }
 
+void InProcessHandler::StartProcessingPendingReports() {
+  if (!upload_thread_started_ && upload_thread_) {
+    upload_thread_->Start();
+    upload_thread_started_ = true;
+  }
+}
+
 void InProcessHandler::SaveSnapshot(
     ProcessSnapshotIOSIntermediateDump& process_snapshot) {
   std::unique_ptr<CrashReportDatabase::NewReport> new_report;
@@ -293,11 +281,12 @@ InProcessHandler::ScopedAlternateWriter::~ScopedAlternateWriter() {
 InProcessHandler::ScopedReport::ScopedReport(
     IOSIntermediateDumpWriter* writer,
     const IOSSystemDataCollector& system_data,
+    const std::map<std::string, std::string>& annotations,
     const uint64_t* frames,
     const size_t num_frames)
     : rootMap_(writer) {
   InProcessIntermediateDumpHandler::WriteHeader(writer);
-  InProcessIntermediateDumpHandler::WriteProcessInfo(writer);
+  InProcessIntermediateDumpHandler::WriteProcessInfo(writer, annotations);
   InProcessIntermediateDumpHandler::WriteSystemInfo(writer, system_data);
   InProcessIntermediateDumpHandler::WriteThreadInfo(writer, frames, num_frames);
   InProcessIntermediateDumpHandler::WriteModuleInfo(writer);

client/ios_handler/in_process_handler.h

@@ -156,6 +156,7 @@ class InProcessHandler {
    public:
     ScopedReport(IOSIntermediateDumpWriter* writer,
                  const IOSSystemDataCollector& system_data,
+                 const std::map<std::string, std::string>& annotations,
                  const uint64_t* frames = nullptr,
                  const size_t num_frames = 0);
     ~ScopedReport() {}
@@ -178,9 +179,6 @@ class InProcessHandler {
     open_new_file_after_report_ = open_new_file_after_report;
   }
 
-  void ProcessIntermediateDumpWithCompleteAnnotations(
-      const base::FilePath& file,
-      const std::map<std::string, std::string>& annotations);
   void SaveSnapshot(ProcessSnapshotIOSIntermediateDump& process_snapshot);
   std::vector<base::FilePath> PendingFiles();
   bool OpenNewFile();

client/ios_handler/in_process_intermediate_dump_handler.cc

@@ -582,7 +582,8 @@ void InProcessIntermediateDumpHandler::WriteHeader(
 
 // static
 void InProcessIntermediateDumpHandler::WriteProcessInfo(
-    IOSIntermediateDumpWriter* writer) {
+    IOSIntermediateDumpWriter* writer,
+    const std::map<std::string, std::string>& annotations) {
   IOSIntermediateDumpWriter::ScopedMap process_map(
       writer, IntermediateDumpKey::kProcessInfo);
 
@@ -647,6 +648,24 @@ void InProcessIntermediateDumpHandler::WriteProcessInfo(
   } else {
     CRASHPAD_RAW_LOG("task_info task_basic_info");
   }
+
+  if (!annotations.empty()) {
+    IOSIntermediateDumpWriter::ScopedArray simple_annotations_array(
+        writer, IntermediateDumpKey::kAnnotationsSimpleMap);
+    for (const auto& annotation_pair : annotations) {
+      const std::string& key = annotation_pair.first;
+      const std::string& value = annotation_pair.second;
+      IOSIntermediateDumpWriter::ScopedArrayMap annotation_map(writer);
+      WriteProperty(writer,
+                    IntermediateDumpKey::kAnnotationName,
+                    key.c_str(),
+                    key.length());
+      WriteProperty(writer,
+                    IntermediateDumpKey::kAnnotationValue,
+                    value.c_str(),
+                    value.length());
+    }
+  }
 }
 
 // static

client/ios_handler/in_process_intermediate_dump_handler.h

@@ -20,6 +20,8 @@
 #include <signal.h>
 #include <sys/types.h>
 
+#include <map>
+
 #include "client/crashpad_info.h"
 #include "util/ios/ios_intermediate_dump_writer.h"
 #include "util/ios/ios_system_data_collector.h"
@@ -46,7 +48,10 @@ class InProcessIntermediateDumpHandler final {
   //! \brief Write ProcessSnapshot data to the intermediate dump.
   //!
   //! \param[in] writer The dump writer
-  static void WriteProcessInfo(IOSIntermediateDumpWriter* writer);
+  //! \param[in] annotations The simple map annotations.
+  static void WriteProcessInfo(
+      IOSIntermediateDumpWriter* writer,
+      const std::map<std::string, std::string>& annotations);
 
   //! \brief Write SystemSnapshot data to the intermediate dump.
   //!

client/ios_handler/in_process_intermediate_dump_handler_test.cc

@@ -55,7 +55,8 @@ class InProcessIntermediateDumpHandlerTest : public testing::Test {
   void WriteReport() {
     internal::IOSIntermediateDumpWriter::ScopedRootMap rootMap(writer_.get());
     InProcessIntermediateDumpHandler::WriteHeader(writer_.get());
-    InProcessIntermediateDumpHandler::WriteProcessInfo(writer_.get());
+    InProcessIntermediateDumpHandler::WriteProcessInfo(
+        writer_.get(), {{"before_dump", "pre"}});
     InProcessIntermediateDumpHandler::WriteSystemInfo(writer_.get(),
                                                       system_data_);
     InProcessIntermediateDumpHandler::WriteThreadInfo(writer_.get(), 0, 0);
@@ -147,7 +148,8 @@ TEST_F(InProcessIntermediateDumpHandlerTest, TestAnnotations) {
   ASSERT_TRUE(process_snapshot.Initialize(path(), {{"after_dump", "post"}}));
 
   auto process_map = process_snapshot.AnnotationsSimpleMap();
-  EXPECT_EQ(process_map.size(), 1u);
+  EXPECT_EQ(process_map.size(), 2u);
+  EXPECT_EQ(process_map["before_dump"], "pre");
   EXPECT_EQ(process_map["after_dump"], "post");
 
   std::map<std::string, std::string> all_annotations_simple_map;

snapshot/ios/process_snapshot_ios_intermediate_dump.cc

@@ -84,6 +84,21 @@ bool ProcessSnapshotIOSIntermediateDump::Initialize(
 
   GetDataValueFromMap(process_info, Key::kSnapshotTime, &snapshot_time_);
 
+  const IOSIntermediateDumpList* simple_map_dump =
+      process_info->GetAsList(IntermediateDumpKey::kAnnotationsSimpleMap);
+  if (simple_map_dump) {
+    for (auto& annotation : *simple_map_dump) {
+      const IOSIntermediateDumpData* name_dump =
+          annotation->GetAsData(IntermediateDumpKey::kAnnotationName);
+      const IOSIntermediateDumpData* value_dump =
+          annotation->GetAsData(IntermediateDumpKey::kAnnotationValue);
+      if (name_dump && value_dump) {
+        annotations_simple_map_.insert(
+            make_pair(name_dump->GetString(), value_dump->GetString()));
+      }
+    }
+  }
+
   const IOSIntermediateDumpMap* system_info =
       GetMapFromMap(root_map, Key::kSystemInfo);
   if (!system_info) {

test/ios/crash_type_xctest.mm

@@ -184,6 +184,33 @@
   [self verifyCrashReportException:SIGHUP];
 }
 
+- (void)testClientAnnotations {
+  [rootObject_ crashKillAbort];
+
+  // Set app launch args to trigger different client annotations.
+  NSArray<NSString*>* old_args = app_.launchArguments;
+  app_.launchArguments = @[ @"--alternate-client-annotations" ];
+  [self verifyCrashReportException:SIGABRT];
+  app_.launchArguments = old_args;
+
+  // Confirm the initial crash took the standard annotations.
+  NSDictionary* dict = [rootObject_ getProcessAnnotations];
+  XCTAssertTrue([dict[@"crashpad"] isEqualToString:@"yes"]);
+  XCTAssertTrue([dict[@"plat"] isEqualToString:@"iOS"]);
+  XCTAssertTrue([dict[@"prod"] isEqualToString:@"xcuitest"]);
+  XCTAssertTrue([dict[@"ver"] isEqualToString:@"1"]);
+
+  // Confirm passing alternate client annotation args works.
+  [rootObject_ clearPendingReports];
+  [rootObject_ crashKillAbort];
+  [self verifyCrashReportException:SIGABRT];
+  dict = [rootObject_ getProcessAnnotations];
+  XCTAssertTrue([dict[@"crashpad"] isEqualToString:@"no"]);
+  XCTAssertTrue([dict[@"plat"] isEqualToString:@"macOS"]);
+  XCTAssertTrue([dict[@"prod"] isEqualToString:@"some_app"]);
+  XCTAssertTrue([dict[@"ver"] isEqualToString:@"42"]);
+}
+
 - (void)testCrashWithCrashInfoMessage {
   if (@available(iOS 15.0, *)) {
     // Figure out how to test this on iOS15.

test/ios/host/cptest_application_delegate.mm

@@ -92,7 +92,18 @@ OperationStatus GetPendingReports(std::vector<Report>* pending_reports) {
 - (BOOL)application:(UIApplication*)application
     didFinishLaunchingWithOptions:(NSDictionary*)launchOptions {
   // Start up crashpad.
-  if (client_.StartCrashpadInProcessHandler(GetDatabaseDir(), "", {})) {
+  std::map<std::string, std::string> annotations = {
+      {"prod", "xcuitest"}, {"ver", "1"}, {"plat", "iOS"}, {"crashpad", "yes"}};
+
+  NSArray<NSString*>* arguments = [[NSProcessInfo processInfo] arguments];
+  if ([arguments containsObject:@"--alternate-client-annotations"]) {
+    annotations = {{"prod", "some_app"},
+                   {"ver", "42"},
+                   {"plat", "macOS"},
+                   {"crashpad", "no"}};
+  }
+  if (client_.StartCrashpadInProcessHandler(
+          GetDatabaseDir(), "", annotations)) {
     client_.ProcessIntermediateDumps();
   }
 
@@ -203,6 +214,26 @@ OperationStatus GetPendingReports(std::vector<Report>* pending_reports) {
   return [dict passByValue];
 }
 
+- (NSDictionary*)getProcessAnnotations {
+  std::vector<Report> pending_reports;
+  OperationStatus status = GetPendingReports(&pending_reports);
+  if (status != crashpad::CrashReportDatabase::kNoError ||
+      pending_reports.size() != 1) {
+    return @{};
+  }
+
+  auto reader = std::make_unique<crashpad::FileReader>();
+  reader->Open(pending_reports[0].file_path);
+  crashpad::ProcessSnapshotMinidump process_snapshot;
+  process_snapshot.Initialize(reader.get());
+  NSDictionary* dict = [@{} mutableCopy];
+  for (const auto& kv : process_snapshot.AnnotationsSimpleMap()) {
+    [dict setValue:@(kv.second.c_str()) forKey:@(kv.first.c_str())];
+  }
+
+  return [dict passByValue];
+}
+
 - (void)crashBadAccess {
   strcpy(nullptr, "bla");
 }

test/ios/host/cptest_shared_object.h

@@ -42,6 +42,10 @@
 // (strings only) respectively.
 - (NSDictionary*)getAnnotations;
 
+// Return an NSDictionary representing the ProcessSnapshotMinidump
+// AnnotationsSimpleMap.
+- (NSDictionary*)getProcessAnnotations;
+
 // Triggers an EXC_BAD_ACCESS exception and crash.
 - (void)crashBadAccess;