Pool TypeName strings when writing MINIDUMP_HANDLE_DESCRIPTOR

Follow up to TODO in https://codereview.chromium.org/1419623003/.

R=mark@chromium.org
BUG=crashpad:21, crashpad:52

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

minidump/minidump_handle_writer.cc

@@ -17,6 +17,7 @@
 #include <string>
 
 #include "base/logging.h"
+#include "base/stl_util.h"
 #include "minidump/minidump_extensions.h"
 #include "util/file/file_writer.h"
 
@@ -27,6 +28,7 @@ MinidumpHandleDataWriter::MinidumpHandleDataWriter()
 }
 
 MinidumpHandleDataWriter::~MinidumpHandleDataWriter() {
+  STLDeleteContainerPairSecondPointers(strings_.begin(), strings_.end());
 }
 
 void MinidumpHandleDataWriter::InitializeFromSnapshot(
@@ -37,7 +39,6 @@ void MinidumpHandleDataWriter::InitializeFromSnapshot(
   // Because we RegisterRVA() on the string writer below, we preallocate and
   // never resize the handle_descriptors_ vector.
   handle_descriptors_.resize(handle_snapshots.size());
-  strings_.reserve(handle_snapshots.size());
   for (size_t i = 0; i < handle_snapshots.size(); ++i) {
     const HandleSnapshot& handle_snapshot = handle_snapshots[i];
     MINIDUMP_HANDLE_DESCRIPTOR& descriptor = handle_descriptors_[i];
@@ -47,11 +48,16 @@ void MinidumpHandleDataWriter::InitializeFromSnapshot(
     if (handle_snapshot.type_name.empty()) {
       descriptor.TypeNameRva = 0;
     } else {
-      // TODO(scottmg): There is often a number of repeated type names here, the
+      auto it = strings_.lower_bound(handle_snapshot.type_name);
-      // strings ought to be pooled.
+      internal::MinidumpUTF16StringWriter* writer;
-      strings_.push_back(new internal::MinidumpUTF16StringWriter());
+      if (it != strings_.end() && it->first == handle_snapshot.type_name) {
-      strings_.back()->SetUTF8(handle_snapshot.type_name);
+        writer = it->second;
-      strings_.back()->RegisterRVA(&descriptor.TypeNameRva);
+      } else {
+        writer = new internal::MinidumpUTF16StringWriter();
+        strings_.insert(it, std::make_pair(handle_snapshot.type_name, writer));
+        writer->SetUTF8(handle_snapshot.type_name);
+      }
+      writer->RegisterRVA(&descriptor.TypeNameRva);
     }
 
     descriptor.ObjectNameRva = 0;
@@ -86,8 +92,8 @@ std::vector<internal::MinidumpWritable*> MinidumpHandleDataWriter::Children() {
   DCHECK_GE(state(), kStateFrozen);
 
   std::vector<MinidumpWritable*> children;
-  for (auto* string : strings_)
+  for (const auto& pair : strings_)
-    children.push_back(string);
+    children.push_back(pair.second);
   return children;
 }
 

minidump/minidump_handle_writer.h

@@ -18,13 +18,14 @@
 #include <windows.h>
 #include <dbghelp.h>
 
+#include <map>
+#include <string>
 #include <vector>
 
 #include "minidump/minidump_stream_writer.h"
 #include "minidump/minidump_string_writer.h"
 #include "minidump/minidump_writable.h"
 #include "snapshot/handle_snapshot.h"
-#include "util/stdlib/pointer_container.h"
 
 namespace crashpad {
 
@@ -65,7 +66,7 @@ class MinidumpHandleDataWriter final : public internal::MinidumpStreamWriter {
  private:
   MINIDUMP_HANDLE_DATA_STREAM handle_data_stream_base_;
   std::vector<MINIDUMP_HANDLE_DESCRIPTOR> handle_descriptors_;
-  PointerVector<internal::MinidumpUTF16StringWriter> strings_;
+  std::map<std::string, internal::MinidumpUTF16StringWriter*> strings_;
 
   DISALLOW_COPY_AND_ASSIGN(MinidumpHandleDataWriter);
 };

minidump/minidump_handle_writer_test.cc

@@ -122,6 +122,80 @@ TEST(MinidumpHandleDataWriter, OneHandle) {
   EXPECT_EQ(handle_snapshot.pointer_count, handle_descriptor->PointerCount);
 }
 
+TEST(MinidumpHandleDataWriter, RepeatedTypeName) {
+  MinidumpFileWriter minidump_file_writer;
+  auto handle_data_writer = make_scoped_ptr(new MinidumpHandleDataWriter());
+
+  HandleSnapshot handle_snapshot;
+  handle_snapshot.handle = 0x1234;
+  handle_snapshot.type_name = "Something";
+  handle_snapshot.attributes = 0x12345678;
+  handle_snapshot.granted_access = 0x9abcdef0;
+  handle_snapshot.pointer_count = 4567;
+  handle_snapshot.handle_count = 9876;
+
+  HandleSnapshot handle_snapshot2;
+  handle_snapshot2.handle = 0x4321;
+  handle_snapshot2.type_name = "Something";  // Note: same as above.
+  handle_snapshot2.attributes = 0x87654321;
+  handle_snapshot2.granted_access = 0x0fedcba9;
+  handle_snapshot2.pointer_count = 7654;
+  handle_snapshot2.handle_count = 6789;
+
+  std::vector<HandleSnapshot> snapshot;
+  snapshot.push_back(handle_snapshot);
+  snapshot.push_back(handle_snapshot2);
+
+  handle_data_writer->InitializeFromSnapshot(snapshot);
+
+  minidump_file_writer.AddStream(handle_data_writer.Pass());
+
+  StringFile string_file;
+  ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file));
+
+  const size_t kTypeNameStringDataLength =
+      (handle_snapshot.type_name.size() + 1) * sizeof(base::char16);
+  ASSERT_EQ(sizeof(MINIDUMP_HEADER) + sizeof(MINIDUMP_DIRECTORY) +
+                sizeof(MINIDUMP_HANDLE_DATA_STREAM) +
+                (sizeof(MINIDUMP_HANDLE_DESCRIPTOR) * 2) +
+                sizeof(MINIDUMP_STRING) + kTypeNameStringDataLength,
+            string_file.string().size());
+
+  const MINIDUMP_HANDLE_DATA_STREAM* handle_data_stream = nullptr;
+  ASSERT_NO_FATAL_FAILURE(
+      GetHandleDataStream(string_file.string(), &handle_data_stream));
+
+  EXPECT_EQ(2u, handle_data_stream->NumberOfDescriptors);
+  const MINIDUMP_HANDLE_DESCRIPTOR* handle_descriptor =
+      reinterpret_cast<const MINIDUMP_HANDLE_DESCRIPTOR*>(
+          &handle_data_stream[1]);
+  EXPECT_EQ(handle_snapshot.handle, handle_descriptor->Handle);
+  EXPECT_EQ(handle_snapshot.type_name,
+            base::UTF16ToUTF8(MinidumpStringAtRVAAsString(
+                string_file.string(), handle_descriptor->TypeNameRva)));
+  EXPECT_EQ(0u, handle_descriptor->ObjectNameRva);
+  EXPECT_EQ(handle_snapshot.attributes, handle_descriptor->Attributes);
+  EXPECT_EQ(handle_snapshot.granted_access, handle_descriptor->GrantedAccess);
+  EXPECT_EQ(handle_snapshot.handle_count, handle_descriptor->HandleCount);
+  EXPECT_EQ(handle_snapshot.pointer_count, handle_descriptor->PointerCount);
+
+  const MINIDUMP_HANDLE_DESCRIPTOR* handle_descriptor2 =
+      reinterpret_cast<const MINIDUMP_HANDLE_DESCRIPTOR*>(
+          reinterpret_cast<const unsigned char*>(&handle_data_stream[1]) +
+          sizeof(MINIDUMP_HANDLE_DESCRIPTOR));
+  EXPECT_EQ(handle_snapshot2.handle, handle_descriptor2->Handle);
+  EXPECT_EQ(handle_snapshot2.type_name,
+            base::UTF16ToUTF8(MinidumpStringAtRVAAsString(
+                string_file.string(), handle_descriptor2->TypeNameRva)));
+  EXPECT_EQ(0u, handle_descriptor2->ObjectNameRva);
+  EXPECT_EQ(handle_snapshot2.attributes, handle_descriptor2->Attributes);
+  EXPECT_EQ(handle_snapshot2.granted_access, handle_descriptor2->GrantedAccess);
+  EXPECT_EQ(handle_snapshot2.handle_count, handle_descriptor2->HandleCount);
+  EXPECT_EQ(handle_snapshot2.pointer_count, handle_descriptor2->PointerCount);
+
+  EXPECT_EQ(handle_descriptor->TypeNameRva, handle_descriptor2->TypeNameRva);
+}
+
 }  // namespace
 }  // namespace test
 }  // namespace crashpad