Bug 1196461 - De-duplicate strings in heap snapshot core dumps; r=shu,jimb

This changeset replaces all of the

    // char16_t[]
    optional bytes someProperty = 1;

one- and two-byte string properties in the CoreDump.proto protobuf definition
file with:

    oneof {
        // char16_t[]
        bytes  someProperty    = 1;
        uint64 somePropertyRef = 2;
    }

The first time the N^th unique string is serialized, then someProperty is used
and the full string is serialized in the protobuf message. All following times
that string is serialized, somePropertyRef is used and its value is N.

Among the other things, this also changes JS::ubi::Edge::name from a raw pointer
with commented rules about who does or doesn't own and should and shouldn't free
the raw pointer to a UniquePtr that enforces those rules rather than relying on
developers reading and obeying the rules in the comments.

devtools/shared/heapsnapshot/.gitattributes

@@ -0,0 +1 @@
+CoreDump.pb.* binary

devtools/shared/heapsnapshot/AutoMemMap.h

@@ -4,6 +4,9 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+#ifndef mozilla_devtools_AutoMemMap_h
+#define mozilla_devtools_AutoMemMap_h
+
 #include <prio.h>
 #include "mozilla/GuardObjects.h"
 
@@ -68,3 +71,5 @@ public:
 
 } // namespace devtools
 } // namespace mozilla
+
+#endif // mozilla_devtools_AutoMemMap_h

devtools/shared/heapsnapshot/CoreDump.proto

@@ -38,11 +38,25 @@
 //     | .                                                                     |
 //     +-----------------------------------------------------------------------+
 //
-// In practice, certain message fields have a lot of duplication (such as type
+// Core dumps should always be written with a
-// or edge name strings). Rather than try and de-duplicate this information at
+// `google::protobuf::io::GzipOutputStream` and read from a
-// the protobuf message and field level, core dumps should be written with
-// `google::protobuf::io::GzipOutputStream` and read from
 // `google::protobuf::io::GzipInputStream`.
+//
+// Note that all strings are de-duplicated. The first time the N^th unique
+// string is encountered, the full string is serialized. Subsequent times that
+// same string is encountered, it is referenced by N. This de-duplication
+// happens across string properties, not on a per-property basis. For example,
+// if the same K^th unique string is first used as an Edge::EdgeNameOrRef and
+// then as a StackFrame::Data::FunctionDisplayNameOrRef, the first will be the
+// actual string as the functionDisplayName oneof property, and the second will
+// be a reference to the first as the edgeNameRef oneof property whose value is
+// K.
+//
+// We would ordinarily abstract these de-duplicated strings with messages of
+// their own, but unfortunately, the protobuf compiler does not have a way to
+// inline a messsage within another message and the child message must be
+// referenced by pointer. This leads to extra mallocs that we wish to avoid.
+
 
 package mozilla.devtools.protobuf;
 
@@ -65,36 +79,59 @@ message StackFrame {
     }
 
     message Data {
-        optional uint64 id = 1;
+        optional uint64        id           = 1;
-        optional StackFrame parent = 2;
+        optional StackFrame    parent       = 2;
-        optional uint32 line = 3;
+        optional uint32        line         = 3;
-        optional uint32 column = 4;
+        optional uint32        column       = 4;
-        // char16_t[]
+
-        optional bytes source = 5;
+        // De-duplicated two-byte string.
-        // char16_t[]
+        oneof SourceOrRef {
-        optional bytes functionDisplayName = 6;
+            bytes  source                   = 5;
-        optional bool isSystem = 7;
+            uint64 sourceRef                = 6;
-        optional bool isSelfHosted = 8;
+        }
+
+        // De-duplicated two-byte string.
+        oneof FunctionDisplayNameOrRef {
+            bytes  functionDisplayName      = 7;
+            uint64 functionDisplayNameRef   = 8;
+        }
+
+        optional bool          isSystem     = 9;
+        optional bool          isSelfHosted = 10;
     }
 }
 
 // A serialized version of `JS::ubi::Node` and its outgoing edges.
 message Node {
-    optional uint64     id                = 1;
+    optional uint64     id                   = 1;
-    // char16_t[]
+
-    optional bytes      typeName          = 2;
+    // De-duplicated two-byte string.
-    optional uint64     size              = 3;
+    oneof TypeNameOrRef {
-    repeated Edge       edges             = 4;
+        bytes           typeName             = 2;
-    optional StackFrame allocationStack   = 5;
+        uint64          typeNameRef          = 3;
-    // char[]
+    }
-    optional bytes      jsObjectClassName = 6;
+    
+    optional uint64     size                 = 4;
+    repeated Edge       edges                = 5;
+    optional StackFrame allocationStack      = 6;
+
+    // De-duplicated one-byte string.
+    oneof JSObjectClassNameOrRef {
+        bytes           jsObjectClassName    = 7;
+        uint64          jsObjectClassNameRef = 8;
+    }
+
     // JS::ubi::CoarseType. Defaults to Other.
-    optional uint32     coarseType        = 7 [default = 0];
+    optional uint32     coarseType           = 9 [default = 0];
 }
 
 // A serialized edge from the heap graph.
 message Edge {
-    optional uint64 referent = 1;
+    optional uint64 referent    = 1;
-    // char16_t[]
+
-    optional bytes  name     = 2;
+    // De-duplicated two-byte string.
+    oneof EdgeNameOrRef {
+        bytes       name    = 2;
+        uint64      nameRef = 3;
+    }
 }

devtools/shared/heapsnapshot/DeserializedNode.cpp

@@ -10,11 +10,6 @@
 namespace mozilla {
 namespace devtools {
 
-DeserializedEdge::DeserializedEdge()
-  : referent(0)
-  , name(nullptr)
-{ }
-
 DeserializedEdge::DeserializedEdge(DeserializedEdge&& rhs)
 {
   referent = rhs.referent;
@@ -29,26 +24,6 @@ DeserializedEdge& DeserializedEdge::operator=(DeserializedEdge&& rhs)
   return *this;
 }
 
-bool
-DeserializedEdge::init(const protobuf::Edge& edge, HeapSnapshot& owner)
-{
-  // Although the referent property is optional in the protobuf format for
-  // future compatibility, we can't semantically have an edge to nowhere and
-  // require a referent here.
-  if (!edge.has_referent())
-    return false;
-  referent = edge.referent();
-
-  if (edge.has_name()) {
-    const char16_t* duplicateEdgeName = reinterpret_cast<const char16_t*>(edge.name().c_str());
-    name = owner.borrowUniqueString(duplicateEdgeName, edge.name().length() / sizeof(char16_t));
-    if (!name)
-      return false;
-  }
-
-  return true;
-}
-
 JS::ubi::Node
 DeserializedNode::getEdgeReferent(const DeserializedEdge& edge)
 {

devtools/shared/heapsnapshot/DeserializedNode.h

@@ -40,13 +40,13 @@ struct DeserializedEdge {
   // A borrowed reference to a string owned by this node's owning HeapSnapshot.
   const char16_t* name;
 
-  explicit DeserializedEdge();
+  explicit DeserializedEdge(NodeId referent, const char16_t* edgeName = nullptr)
+    : referent(referent)
+    , name(edgeName)
+  { }
   DeserializedEdge(DeserializedEdge&& rhs);
   DeserializedEdge& operator=(DeserializedEdge&& rhs);
 
-  // Initialize this `DeserializedEdge` from the given `protobuf::Edge` message.
-  bool init(const protobuf::Edge& edge, HeapSnapshot& owner);
-
 private:
   DeserializedEdge(const DeserializedEdge&) = delete;
   DeserializedEdge& operator=(const DeserializedEdge&) = delete;
@@ -65,7 +65,8 @@ struct DeserializedNode {
   uint64_t            size;
   EdgeVector          edges;
   Maybe<StackFrameId> allocationStack;
-  UniquePtr<char[]>   jsObjectClassName;
+  // A borrowed reference to a string owned by this node's owning HeapSnapshot.
+  const char*         jsObjectClassName;
   // A weak pointer to this node's owning `HeapSnapshot`. Safe without
   // AddRef'ing because this node's lifetime is equal to that of its owner.
   HeapSnapshot*       owner;
@@ -76,7 +77,7 @@ struct DeserializedNode {
                    uint64_t size,
                    EdgeVector&& edges,
                    Maybe<StackFrameId> allocationStack,
-                   UniquePtr<char[]>&& className,
+                   const char* className,
                    HeapSnapshot& owner)
     : id(id)
     , coarseType(coarseType)
@@ -84,7 +85,7 @@ struct DeserializedNode {
     , size(size)
     , edges(Move(edges))
     , allocationStack(allocationStack)
-    , jsObjectClassName(Move(className))
+    , jsObjectClassName(className)
     , owner(&owner)
   { }
   virtual ~DeserializedNode() { }
@@ -96,7 +97,7 @@ struct DeserializedNode {
     , size(rhs.size)
     , edges(Move(rhs.edges))
     , allocationStack(rhs.allocationStack)
-    , jsObjectClassName(Move(rhs.jsObjectClassName))
+    , jsObjectClassName(rhs.jsObjectClassName)
     , owner(rhs.owner)
   { }
 
@@ -258,7 +259,7 @@ public:
   bool isLive() const override { return false; }
   const char16_t* typeName() const override;
   Node::Size size(mozilla::MallocSizeOf mallocSizeof) const override;
-  const char* jsObjectClassName() const override { return get().jsObjectClassName.get(); }
+  const char* jsObjectClassName() const override { return get().jsObjectClassName; }
 
   bool hasAllocationStack() const override { return get().allocationStack.isSome(); }
   StackFrame allocationStack() const override;

devtools/shared/heapsnapshot/HeapSnapshot.cpp

@@ -50,6 +50,8 @@ using ::google::protobuf::io::CodedInputStream;
 using ::google::protobuf::io::GzipInputStream;
 using ::google::protobuf::io::ZeroCopyInputStream;
 
+using JS::ubi::AtomOrTwoByteChars;
+
 NS_IMPL_CYCLE_COLLECTION_CLASS(HeapSnapshot)
 
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(HeapSnapshot)
@@ -122,76 +124,159 @@ parseMessage(ZeroCopyInputStream& stream, MessageType& message)
   return true;
 }
 
+template<typename CharT, typename InternedStringSet>
+struct GetOrInternStringMatcher
+{
+  using ReturnType = const CharT*;
+
+  InternedStringSet& internedStrings;
+
+  explicit GetOrInternStringMatcher(InternedStringSet& strings) : internedStrings(strings) { }
+
+  const CharT* match(const std::string* str) {
+    MOZ_ASSERT(str);
+    size_t length = str->length() / sizeof(CharT);
+    auto tempString = reinterpret_cast<const CharT*>(str->data());
+
+    UniquePtr<CharT[], NSFreePolicy> owned(NS_strndup(tempString, length));
+    if (!owned || !internedStrings.append(Move(owned)))
+      return nullptr;
+
+    return internedStrings.back().get();
+  }
+
+  const CharT* match(uint64_t ref) {
+    if (MOZ_LIKELY(ref < internedStrings.length())) {
+      auto& string = internedStrings[ref];
+      MOZ_ASSERT(string);
+      return string.get();
+    }
+
+    return nullptr;
+  }
+};
+
+template<
+  // Either char or char16_t.
+  typename CharT,
+  // A reference to either `internedOneByteStrings` or `internedTwoByteStrings`
+  // if CharT is char or char16_t respectively.
+  typename InternedStringSet>
+const CharT*
+HeapSnapshot::getOrInternString(InternedStringSet& internedStrings,
+                                Maybe<StringOrRef>& maybeStrOrRef)
+{
+  // Incomplete message: has neither a string nor a reference to an already
+  // interned string.
+  if (MOZ_UNLIKELY(maybeStrOrRef.isNothing()))
+    return nullptr;
+
+  GetOrInternStringMatcher<CharT, InternedStringSet> m(internedStrings);
+  return maybeStrOrRef->match(m);
+}
+
+// Get a de-duplicated string as a Maybe<StringOrRef> from the given `msg`.
+#define GET_STRING_OR_REF_WITH_PROP_NAMES(msg, strPropertyName, refPropertyName) \
+  (msg.has_##refPropertyName()                                                   \
+    ? Some(StringOrRef(msg.refPropertyName()))                                   \
+    : msg.has_##strPropertyName()                                                \
+      ? Some(StringOrRef(&msg.strPropertyName()))                                \
+      : Nothing())
+
+#define GET_STRING_OR_REF(msg, property)      \
+  (msg.has_##property##ref()                  \
+     ? Some(StringOrRef(msg.property##ref())) \
+     : msg.has_##property()                   \
+       ? Some(StringOrRef(&msg.property()))   \
+       : Nothing())
+
 bool
 HeapSnapshot::saveNode(const protobuf::Node& node)
 {
-  if (!node.has_id())
+  // NB: de-duplicated string properties must be read back and interned in the
+  // same order here as they are written and serialized in
+  // `CoreDumpWriter::writeNode` or else indices in references to already
+  // serialized strings will be off.
+
+  if (NS_WARN_IF(!node.has_id()))
     return false;
   NodeId id = node.id();
 
   // Should only deserialize each node once.
-  if (nodes.has(id))
+  if (NS_WARN_IF(nodes.has(id)))
     return false;
 
-  if (!JS::ubi::Uint32IsValidCoarseType(node.coarsetype()))
+  if (NS_WARN_IF(!JS::ubi::Uint32IsValidCoarseType(node.coarsetype())))
     return false;
   auto coarseType = JS::ubi::Uint32ToCoarseType(node.coarsetype());
 
-  if (!node.has_typename_())
+  Maybe<StringOrRef> typeNameOrRef = GET_STRING_OR_REF_WITH_PROP_NAMES(node, typename_, typenameref);
+  auto typeName = getOrInternString<char16_t>(internedTwoByteStrings, typeNameOrRef);
+  if (NS_WARN_IF(!typeName))
     return false;
 
-  auto duplicatedTypeName = reinterpret_cast<const char16_t*>(
+  if (NS_WARN_IF(!node.has_size()))
-    node.typename_().data());
-  auto length = node.typename_().length() / sizeof(char16_t);
-  auto typeName = borrowUniqueString(duplicatedTypeName, length);
-  if (!typeName)
-    return false;
-
-  if (!node.has_size())
     return false;
   uint64_t size = node.size();
 
   auto edgesLength = node.edges_size();
   DeserializedNode::EdgeVector edges;
-  if (!edges.reserve(edgesLength))
+  if (NS_WARN_IF(!edges.reserve(edgesLength)))
     return false;
   for (decltype(edgesLength) i = 0; i < edgesLength; i++) {
-    DeserializedEdge edge;
+    auto& protoEdge = node.edges(i);
-    if (!edge.init(node.edges(i), *this))
+
+    if (NS_WARN_IF(!protoEdge.has_referent()))
       return false;
-    edges.infallibleAppend(Move(edge));
+    NodeId referent = protoEdge.referent();
+
+    const char16_t* edgeName = nullptr;
+    if (protoEdge.EdgeNameOrRef_case() != protobuf::Edge::EDGENAMEORREF_NOT_SET) {
+      Maybe<StringOrRef> edgeNameOrRef = GET_STRING_OR_REF(protoEdge, name);
+      edgeName = getOrInternString<char16_t>(internedTwoByteStrings, edgeNameOrRef);
+      if (NS_WARN_IF(!edgeName))
+        return false;
+    }
+
+    edges.infallibleAppend(DeserializedEdge(referent, edgeName));
   }
 
   Maybe<StackFrameId> allocationStack;
   if (node.has_allocationstack()) {
     StackFrameId id = 0;
-    if (!saveStackFrame(node.allocationstack(), id))
+    if (NS_WARN_IF(!saveStackFrame(node.allocationstack(), id)))
       return false;
     allocationStack.emplace(id);
   }
   MOZ_ASSERT(allocationStack.isSome() == node.has_allocationstack());
 
-  UniquePtr<char[]> jsObjectClassName;
+  const char* jsObjectClassName = nullptr;
-  if (node.has_jsobjectclassname()) {
+  if (node.JSObjectClassNameOrRef_case() != protobuf::Node::JSOBJECTCLASSNAMEORREF_NOT_SET) {
-    auto length = node.jsobjectclassname().length();
+    Maybe<StringOrRef> clsNameOrRef = GET_STRING_OR_REF(node, jsobjectclassname);
-    jsObjectClassName.reset(static_cast<char*>(malloc(length + 1)));
+    jsObjectClassName = getOrInternString<char>(internedOneByteStrings, clsNameOrRef);
-    if (!jsObjectClassName)
+    if (NS_WARN_IF(!jsObjectClassName))
       return false;
-    strncpy(jsObjectClassName.get(), node.jsobjectclassname().data(),
-            length);
-    jsObjectClassName.get()[length] = '\0';
   }
 
-  return nodes.putNew(id, DeserializedNode(id, coarseType, typeName, size,
+  if (NS_WARN_IF(!nodes.putNew(id, DeserializedNode(id, coarseType, typeName,
-                                           Move(edges), allocationStack,
+                                                    size, Move(edges),
-                                           Move(jsObjectClassName),
+                                                    allocationStack,
-                                           *this));
+                                                    jsObjectClassName, *this))))
+  {
+    return false;
+  };
+
+  return true;
 }
 
 bool
 HeapSnapshot::saveStackFrame(const protobuf::StackFrame& frame,
                              StackFrameId& outFrameId)
 {
+  // NB: de-duplicated string properties must be read in the same order here as
+  // they are written in `CoreDumpWriter::getProtobufStackFrame` or else indices
+  // in references to already serialized strings will be off.
+
   if (frame.has_ref()) {
     // We should only get a reference to the previous frame if we have already
     // seen the previous frame.
@@ -216,14 +301,6 @@ HeapSnapshot::saveStackFrame(const protobuf::StackFrame& frame,
   if (frames.has(id))
     return false;
 
-  Maybe<StackFrameId> parent;
-  if (data.has_parent()) {
-    StackFrameId parentId = 0;
-    if (!saveStackFrame(data.parent(), parentId))
-      return false;
-    parent = Some(parentId);
-  }
-
   if (!data.has_line())
     return false;
   uint32_t line = data.line();
@@ -232,25 +309,6 @@ HeapSnapshot::saveStackFrame(const protobuf::StackFrame& frame,
     return false;
   uint32_t column = data.column();
 
-  auto duplicatedSource = reinterpret_cast<const char16_t*>(
-    data.source().data());
-  size_t sourceLength = data.source().length() / sizeof(char16_t);
-  const char16_t* source = borrowUniqueString(duplicatedSource, sourceLength);
-  if (!source)
-    return false;
-
-  const char16_t* functionDisplayName = nullptr;
-  if (data.has_functiondisplayname() && data.functiondisplayname().length() > 0) {
-    auto duplicatedName = reinterpret_cast<const char16_t*>(
-      data.functiondisplayname().data());
-    size_t nameLength = data.functiondisplayname().length() / sizeof(char16_t);
-    functionDisplayName = borrowUniqueString(duplicatedName, nameLength);
-    if (!functionDisplayName)
-      return false;
-  }
-  MOZ_ASSERT(!!functionDisplayName == (data.has_functiondisplayname() &&
-                                       data.functiondisplayname().length() > 0));
-
   if (!data.has_issystem())
     return false;
   bool isSystem = data.issystem();
@@ -259,6 +317,29 @@ HeapSnapshot::saveStackFrame(const protobuf::StackFrame& frame,
     return false;
   bool isSelfHosted = data.isselfhosted();
 
+  Maybe<StringOrRef> sourceOrRef = GET_STRING_OR_REF(data, source);
+  auto source = getOrInternString<char16_t>(internedTwoByteStrings, sourceOrRef);
+  if (!source)
+    return false;
+
+  const char16_t* functionDisplayName = nullptr;
+  if (data.FunctionDisplayNameOrRef_case() !=
+      protobuf::StackFrame_Data::FUNCTIONDISPLAYNAMEORREF_NOT_SET)
+  {
+    Maybe<StringOrRef> nameOrRef = GET_STRING_OR_REF(data, functiondisplayname);
+    functionDisplayName = getOrInternString<char16_t>(internedTwoByteStrings, nameOrRef);
+    if (!functionDisplayName)
+      return false;
+  }
+
+  Maybe<StackFrameId> parent;
+  if (data.has_parent()) {
+    StackFrameId parentId = 0;
+    if (!saveStackFrame(data.parent(), parentId))
+      return false;
+    parent = Some(parentId);
+  }
+
   if (!frames.putNew(id, DeserializedStackFrame(id, parent, line, column,
                                                 source, functionDisplayName,
                                                 isSystem, isSelfHosted, *this)))
@@ -296,7 +377,7 @@ StreamHasData(GzipInputStream& stream)
 bool
 HeapSnapshot::init(const uint8_t* buffer, uint32_t size)
 {
-  if (!nodes.init() || !frames.init() || !strings.init())
+  if (!nodes.init() || !frames.init())
     return false;
 
   ArrayInputStream stream(buffer, size);
@@ -338,22 +419,6 @@ HeapSnapshot::init(const uint8_t* buffer, uint32_t size)
   return true;
 }
 
-const char16_t*
-HeapSnapshot::borrowUniqueString(const char16_t* duplicateString, size_t length)
-{
-  MOZ_ASSERT(duplicateString);
-  UniqueStringHashPolicy::Lookup lookup(duplicateString, length);
-  auto ptr = strings.lookupForAdd(lookup);
-
-  if (!ptr) {
-    UniqueString owned(NS_strndup(duplicateString, length));
-    if (!owned || !strings.add(ptr, Move(owned)))
-      return nullptr;
-  }
-
-  MOZ_ASSERT(ptr->get() != duplicateString);
-  return ptr->get();
-}
 
 /*** Heap Snapshot Analyses ***********************************************************************/
 
@@ -407,6 +472,10 @@ HeapSnapshot::TakeCensus(JSContext* cx, JS::HandleObject options,
   }
 }
 
+#undef GET_STRING_OR_REF_WITH_PROP_NAMES
+#undef GET_STRING_OR_REF
+
+
 /*** Saving Heap Snapshots ************************************************************************/
 
 // If we are only taking a snapshot of the heap affected by the given set of
@@ -549,17 +618,225 @@ EstablishBoundaries(JSContext* cx,
 }
 
 
+// A variant covering all the various two-byte strings that we can get from the
+// ubi::Node API.
+class TwoByteString : public Variant<JSAtom*, const char16_t*, JS::ubi::EdgeName>
+{
+  using Base = Variant<JSAtom*, const char16_t*, JS::ubi::EdgeName>;
+
+  struct AsTwoByteStringMatcher
+  {
+    using ReturnType = TwoByteString;
+
+    TwoByteString match(JSAtom* atom) {
+      return TwoByteString(atom);
+    }
+
+    TwoByteString match(const char16_t* chars) {
+      return TwoByteString(chars);
+    }
+  };
+
+  struct IsNonNullMatcher
+  {
+    using ReturnType = bool;
+
+    template<typename T>
+    bool match(const T& t) { return t != nullptr; }
+  };
+
+  struct LengthMatcher
+  {
+    using ReturnType = size_t;
+
+    size_t match(JSAtom* atom) {
+      MOZ_ASSERT(atom);
+      JS::ubi::AtomOrTwoByteChars s(atom);
+      return s.length();
+    }
+
+    size_t match(const char16_t* chars) {
+      MOZ_ASSERT(chars);
+      return NS_strlen(chars);
+    }
+
+    size_t match(const JS::ubi::EdgeName& ptr) {
+      MOZ_ASSERT(ptr);
+      return NS_strlen(ptr.get());
+    }
+  };
+
+  struct CopyToBufferMatcher
+  {
+    using ReturnType = size_t;
+
+    RangedPtr<char16_t> destination;
+    size_t              maxLength;
+
+    CopyToBufferMatcher(RangedPtr<char16_t> destination, size_t maxLength)
+      : destination(destination)
+      , maxLength(maxLength)
+    { }
+
+    size_t match(JS::ubi::EdgeName& ptr) {
+      return ptr ? match(ptr.get()) : 0;
+    }
+
+    size_t match(JSAtom* atom) {
+      MOZ_ASSERT(atom);
+      JS::ubi::AtomOrTwoByteChars s(atom);
+      return s.copyToBuffer(destination, maxLength);
+    }
+
+    size_t match(const char16_t* chars) {
+      MOZ_ASSERT(chars);
+      JS::ubi::AtomOrTwoByteChars s(chars);
+      return s.copyToBuffer(destination, maxLength);
+    }
+  };
+
+public:
+  template<typename T>
+  MOZ_IMPLICIT TwoByteString(T&& rhs) : Base(Forward<T>(rhs)) { }
+
+  template<typename T>
+  TwoByteString& operator=(T&& rhs) {
+    MOZ_ASSERT(this != &rhs, "self-move disallowed");
+    this->~TwoByteString();
+    new (this) TwoByteString(Forward<T>(rhs));
+    return *this;
+  }
+
+  TwoByteString(const TwoByteString&) = delete;
+  TwoByteString& operator=(const TwoByteString&) = delete;
+
+  // Rewrap the inner value of a JS::ubi::AtomOrTwoByteChars as a TwoByteString.
+  static TwoByteString from(JS::ubi::AtomOrTwoByteChars&& s) {
+    AsTwoByteStringMatcher m;
+    return s.match(m);
+  }
+
+  // Returns true if the given TwoByteString is non-null, false otherwise.
+  bool isNonNull() const {
+    IsNonNullMatcher m;
+    return match(m);
+  }
+
+  // Return the length of the string, 0 if it is null.
+  size_t length() const {
+    LengthMatcher m;
+    return match(m);
+  }
+
+  // Copy the contents of a TwoByteString into the provided buffer. The buffer
+  // is NOT null terminated. The number of characters written is returned.
+  size_t copyToBuffer(RangedPtr<char16_t> destination, size_t maxLength) {
+    CopyToBufferMatcher m(destination, maxLength);
+    return match(m);
+  }
+
+  struct HashPolicy;
+};
+
+// A hashing policy for TwoByteString.
+//
+// Atoms are pointer hashed and use pointer equality, which means that we
+// tolerate some duplication across atoms and the other two types of two-byte
+// strings. In practice, we expect the amount of this duplication to be very low
+// because each type is generally a different semantic thing in addition to
+// having a slightly different representation. For example, the set of edge
+// names and the set stack frames' source names naturally tend not to overlap
+// very much if at all.
+struct TwoByteString::HashPolicy {
+  using Lookup = TwoByteString;
+
+  struct HashingMatcher {
+    using ReturnType  = js::HashNumber;
+
+    js::HashNumber match(const JSAtom* atom) {
+      return js::DefaultHasher<const JSAtom*>::hash(atom);
+    }
+
+    js::HashNumber match(const char16_t* chars) {
+      MOZ_ASSERT(chars);
+      auto length = NS_strlen(chars);
+      return HashString(chars, length);
+    }
+
+    js::HashNumber match(const JS::ubi::EdgeName& ptr) {
+      MOZ_ASSERT(ptr);
+      return match(ptr.get());
+    }
+  };
+
+  static js::HashNumber hash(const Lookup& l) {
+    HashingMatcher hasher;
+    return l.match(hasher);
+  }
+
+  struct EqualityMatcher {
+    using ReturnType = bool;
+    const TwoByteString& rhs;
+    explicit EqualityMatcher(const TwoByteString& rhs) : rhs(rhs) { }
+
+    bool match(const JSAtom* atom) {
+      return rhs.is<JSAtom*>() && rhs.as<JSAtom*>() == atom;
+    }
+
+    bool match(const char16_t* chars) {
+      MOZ_ASSERT(chars);
+
+      const char16_t* rhsChars = nullptr;
+      if (rhs.is<const char16_t*>())
+        rhsChars = rhs.as<const char16_t*>();
+      else if (rhs.is<JS::ubi::EdgeName>())
+        rhsChars = rhs.as<JS::ubi::EdgeName>().get();
+      else
+        return false;
+      MOZ_ASSERT(rhsChars);
+
+      auto length = NS_strlen(chars);
+      if (NS_strlen(rhsChars) != length)
+        return false;
+
+      return memcmp(chars, rhsChars, length * sizeof(char16_t)) == 0;
+    }
+
+    bool match(const JS::ubi::EdgeName& ptr) {
+      MOZ_ASSERT(ptr);
+      return match(ptr.get());
+    }
+  };
+
+  static bool match(const TwoByteString& k, const Lookup& l) {
+    EqualityMatcher eq(l);
+    return k.match(eq);
+  }
+
+  static void rekey(TwoByteString& k, TwoByteString&& newKey) {
+    k = Move(newKey);
+  }
+};
+
 // A `CoreDumpWriter` that serializes nodes to protobufs and writes them to the
 // given `ZeroCopyOutputStream`.
 class MOZ_STACK_CLASS StreamWriter : public CoreDumpWriter
 {
-  using Set = js::HashSet<uint64_t>;
+  using FrameSet         = js::HashSet<uint64_t>;
+  using TwoByteStringMap = js::HashMap<TwoByteString, uint64_t, TwoByteString::HashPolicy>;
+  using OneByteStringMap = js::HashMap<const char*, uint64_t>;
 
-  JSContext* cx;
+  JSContext*       cx;
-  bool       wantNames;
+  bool             wantNames;
   // The set of |JS::ubi::StackFrame::identifier()|s that have already been
   // serialized and written to the core dump.
-  Set        framesAlreadySerialized;
+  FrameSet         framesAlreadySerialized;
+  // The set of two-byte strings that have already been serialized and written
+  // to the core dump.
+  TwoByteStringMap twoByteStringsAlreadySerialized;
+  // The set of one-byte strings that have already been serialized and written
+  // to the core dump.
+  OneByteStringMap oneByteStringsAlreadySerialized;
 
   ::google::protobuf::io::ZeroCopyOutputStream& stream;
 
@@ -573,7 +850,64 @@ class MOZ_STACK_CLASS StreamWriter : public CoreDumpWriter
     return !codedStream.HadError();
   }
 
+  // Attach the full two-byte string or a reference to a two-byte string that
+  // has already been serialized to a protobuf message.
+  template <typename SetStringFunction,
+            typename SetRefFunction>
+  bool attachTwoByteString(TwoByteString& string, SetStringFunction setString,
+                           SetRefFunction setRef) {
+    auto ptr = twoByteStringsAlreadySerialized.lookupForAdd(string);
+    if (ptr) {
+      setRef(ptr->value());
+      return true;
+    }
+
+    auto length = string.length();
+    auto stringData = MakeUnique<std::string>(length * sizeof(char16_t), '\0');
+    if (!stringData)
+      return false;
+
+    auto buf = const_cast<char16_t*>(reinterpret_cast<const char16_t*>(stringData->data()));
+    string.copyToBuffer(RangedPtr<char16_t>(buf, length), length);
+
+    uint64_t ref = twoByteStringsAlreadySerialized.count();
+    if (!twoByteStringsAlreadySerialized.add(ptr, Move(string), ref))
+      return false;
+
+    setString(stringData.release());
+    return true;
+  }
+
+  // Attach the full one-byte string or a reference to a one-byte string that
+  // has already been serialized to a protobuf message.
+  template <typename SetStringFunction,
+            typename SetRefFunction>
+  bool attachOneByteString(const char* string, SetStringFunction setString,
+                           SetRefFunction setRef) {
+    auto ptr = oneByteStringsAlreadySerialized.lookupForAdd(string);
+    if (ptr) {
+      setRef(ptr->value());
+      return true;
+    }
+
+    auto length = strlen(string);
+    auto stringData = MakeUnique<std::string>(string, length);
+    if (!stringData)
+      return false;
+
+    uint64_t ref = oneByteStringsAlreadySerialized.count();
+    if (!oneByteStringsAlreadySerialized.add(ptr, string, ref))
+      return false;
+
+    setString(stringData.release());
+    return true;
+  }
+
   protobuf::StackFrame* getProtobufStackFrame(JS::ubi::StackFrame& frame) {
+    // NB: de-duplicated string properties must be written in the same order
+    // here as they are read in `HeapSnapshot::saveStackFrame` or else indices
+    // in references to already serialized strings will be off.
+
     MOZ_ASSERT(frame,
                "null frames should be represented as the lack of a serialized "
                "stack frame");
@@ -598,24 +932,22 @@ class MOZ_STACK_CLASS StreamWriter : public CoreDumpWriter
     data->set_issystem(frame.isSystem());
     data->set_isselfhosted(frame.isSelfHosted());
 
-    auto source = MakeUnique<std::string>(frame.sourceLength() * sizeof(char16_t),
+    auto dupeSource = TwoByteString::from(frame.source());
-                                          '\0');
+    if (!attachTwoByteString(dupeSource,
-    if (!source)
+                             [&] (std::string* source) { data->set_allocated_source(source); },
+                             [&] (uint64_t ref) { data->set_sourceref(ref); }))
+    {
       return nullptr;
-    auto buf = const_cast<char16_t*>(reinterpret_cast<const char16_t*>(source->data()));
+    }
-    frame.source(RangedPtr<char16_t>(buf, frame.sourceLength()),
-                 frame.sourceLength());
-    data->set_allocated_source(source.release());
 
-    auto nameLength = frame.functionDisplayNameLength();
+    auto dupeName = TwoByteString::from(frame.functionDisplayName());
-    if (nameLength > 0) {
+    if (dupeName.isNonNull()) {
-      auto functionDisplayName = MakeUnique<std::string>(nameLength * sizeof(char16_t),
+      if (!attachTwoByteString(dupeName,
-                                                         '\0');
+                               [&] (std::string* name) { data->set_allocated_functiondisplayname(name); },
-      if (!functionDisplayName)
+                               [&] (uint64_t ref) { data->set_functiondisplaynameref(ref); }))
+      {
         return nullptr;
-      auto buf = const_cast<char16_t*>(reinterpret_cast<const char16_t*>(functionDisplayName->data()));
+      }
-      frame.functionDisplayName(RangedPtr<char16_t>(buf, nameLength), nameLength);
-      data->set_allocated_functiondisplayname(functionDisplayName.release());
     }
 
     auto parent = frame.parent();
@@ -641,14 +973,20 @@ public:
     : cx(cx)
     , wantNames(wantNames)
     , framesAlreadySerialized(cx)
+    , twoByteStringsAlreadySerialized(cx)
+    , oneByteStringsAlreadySerialized(cx)
     , stream(stream)
   { }
 
-  bool init() { return framesAlreadySerialized.init(); }
+  bool init() {
+    return framesAlreadySerialized.init() &&
+           twoByteStringsAlreadySerialized.init() &&
+           oneByteStringsAlreadySerialized.init();
+  }
 
   ~StreamWriter() override { }
 
-  virtual bool writeMetadata(uint64_t timestamp) override {
+  virtual bool writeMetadata(uint64_t timestamp) final {
     protobuf::Metadata metadata;
     metadata.set_timestamp(timestamp);
     return writeMessage(metadata);
@@ -656,20 +994,55 @@ public:
 
   virtual bool writeNode(const JS::ubi::Node& ubiNode,
                          EdgePolicy includeEdges) final {
+    // NB: de-duplicated string properties must be written in the same order
+    // here as they are read in `HeapSnapshot::saveNode` or else indices in
+    // references to already serialized strings will be off.
+
     protobuf::Node protobufNode;
     protobufNode.set_id(ubiNode.identifier());
 
     protobufNode.set_coarsetype(JS::ubi::CoarseTypeToUint32(ubiNode.coarseType()));
 
-    const char16_t* typeName = ubiNode.typeName();
+    auto typeName = TwoByteString(ubiNode.typeName());
-    size_t length = NS_strlen(typeName) * sizeof(char16_t);
+    if (NS_WARN_IF(!attachTwoByteString(typeName,
-    protobufNode.set_typename_(typeName, length);
+                                        [&] (std::string* name) { protobufNode.set_allocated_typename_(name); },
+                                        [&] (uint64_t ref) { protobufNode.set_typenameref(ref); })))
+    {
+      return false;
+    }
 
     JSRuntime* rt = JS_GetRuntime(cx);
     mozilla::MallocSizeOf mallocSizeOf = dbg::GetDebuggerMallocSizeOf(rt);
     MOZ_ASSERT(mallocSizeOf);
     protobufNode.set_size(ubiNode.size(mallocSizeOf));
 
+    if (includeEdges) {
+      auto edges = ubiNode.edges(JS_GetRuntime(cx), wantNames);
+      if (NS_WARN_IF(!edges))
+        return false;
+
+      for ( ; !edges->empty(); edges->popFront()) {
+        ubi::Edge& ubiEdge = edges->front();
+
+        protobuf::Edge* protobufEdge = protobufNode.add_edges();
+        if (NS_WARN_IF(!protobufEdge)) {
+          return false;
+        }
+
+        protobufEdge->set_referent(ubiEdge.referent.identifier());
+
+        if (wantNames && ubiEdge.name) {
+          TwoByteString edgeName(Move(ubiEdge.name));
+          if (NS_WARN_IF(!attachTwoByteString(edgeName,
+                                              [&] (std::string* name) { protobufEdge->set_allocated_name(name); },
+                                              [&] (uint64_t ref) { protobufEdge->set_nameref(ref); })))
+          {
+            return false;
+          }
+        }
+      }
+    }
+
     if (ubiNode.hasAllocationStack()) {
       auto ubiStackFrame = ubiNode.allocationStack();
       auto protoStackFrame = getProtobufStackFrame(ubiStackFrame);
@@ -679,29 +1052,11 @@ public:
     }
 
     if (auto className = ubiNode.jsObjectClassName()) {
-      size_t length = strlen(className);
+      if (NS_WARN_IF(!attachOneByteString(className,
-      protobufNode.set_jsobjectclassname(className, length);
+                                          [&] (std::string* name) { protobufNode.set_allocated_jsobjectclassname(name); },
-    }
+                                          [&] (uint64_t ref) { protobufNode.set_jsobjectclassnameref(ref); })))
-
+      {
-    if (includeEdges) {
-      auto edges = ubiNode.edges(JS_GetRuntime(cx), wantNames);
-      if (NS_WARN_IF(!edges))
         return false;
-
-      for ( ; !edges->empty(); edges->popFront()) {
-        const ubi::Edge& ubiEdge = edges->front();
-
-        protobuf::Edge* protobufEdge = protobufNode.add_edges();
-        if (NS_WARN_IF(!protobufEdge)) {
-          return false;
-        }
-
-        protobufEdge->set_referent(ubiEdge.referent.identifier());
-
-        if (wantNames && ubiEdge.name) {
-          size_t length = NS_strlen(ubiEdge.name) * sizeof(char16_t);
-          protobufEdge->set_name(ubiEdge.name, length);
-        }
       }
     }
 

devtools/shared/heapsnapshot/HeapSnapshot.h

@@ -34,36 +34,14 @@ struct NSFreePolicy {
   }
 };
 
-using UniqueString = UniquePtr<char16_t[], NSFreePolicy>;
+using UniqueTwoByteString = UniquePtr<char16_t[], NSFreePolicy>;
-
+using UniqueOneByteString = UniquePtr<char[], NSFreePolicy>;
-struct UniqueStringHashPolicy {
-  struct Lookup {
-    const char16_t* str;
-    size_t          length;
-
-    Lookup(const char16_t* str, size_t length)
-      : str(str)
-      , length(length)
-    { }
-  };
-
-  static js::HashNumber hash(const Lookup& lookup) {
-    MOZ_ASSERT(lookup.str);
-    return HashString(lookup.str, lookup.length);
-  }
-
-  static bool match(const UniqueString& existing, const Lookup& lookup) {
-    MOZ_ASSERT(lookup.str);
-    if (NS_strlen(existing.get()) != lookup.length)
-      return false;
-    return memcmp(existing.get(), lookup.str, lookup.length * sizeof(char16_t)) == 0;
-  }
-};
 
 class HeapSnapshot final : public nsISupports
                          , public nsWrapperCache
 {
   friend struct DeserializedNode;
+  friend struct DeserializedEdge;
   friend struct DeserializedStackFrame;
   friend struct JS::ubi::Concrete<JS::ubi::DeserializedNode>;
 
@@ -72,7 +50,6 @@ class HeapSnapshot final : public nsISupports
     , rootId(0)
     , nodes(cx)
     , frames(cx)
-    , strings(cx)
     , mParent(aParent)
   {
     MOZ_ASSERT(aParent);
@@ -108,14 +85,15 @@ class HeapSnapshot final : public nsISupports
                                DeserializedStackFrame::HashPolicy>;
   FrameSet frames;
 
-  // Core dump files have many duplicate strings: type names are repeated for
+  Vector<UniqueTwoByteString> internedTwoByteStrings;
-  // each node, and although in theory edge names are highly customizable for
+  Vector<UniqueOneByteString> internedOneByteStrings;
-  // specific edges, in practice they are also highly duplicated. Rather than
+
-  // make each Deserialized{Node,Edge} malloc their own copy of their edge and
+  using StringOrRef = Variant<const std::string*, uint64_t>;
-  // type names, we de-duplicate the strings here and Deserialized{Node,Edge}
+
-  // get borrowed pointers into this set.
+  template<typename CharT,
-  using UniqueStringSet = js::HashSet<UniqueString, UniqueStringHashPolicy>;
+           typename InternedStringSet>
-  UniqueStringSet strings;
+  const CharT* getOrInternString(InternedStringSet& internedStrings,
+                                 Maybe<StringOrRef>& maybeStrOrRef);
 
 protected:
   nsCOMPtr<nsISupports> mParent;

devtools/shared/heapsnapshot/tests/gtest/DeserializedNodeUbiNodes.cpp

@@ -43,9 +43,8 @@ DEF_TEST(DeserializedNodeUbiNodes, {
     NodeId id = uint64_t(1) << 33;
     uint64_t size = uint64_t(1) << 60;
     MockDeserializedNode mocked(id, typeName, size);
-    mocked.jsObjectClassName = mozilla::UniquePtr<char[]>(strdup(className));
-    ASSERT_TRUE(!!mocked.jsObjectClassName);
     mocked.coarseType = JS::ubi::CoarseType::Script;
+    mocked.jsObjectClassName = className;
 
     DeserializedNode& deserialized = mocked;
     JS::ubi::Node ubi(&deserialized);
@@ -57,15 +56,14 @@ DEF_TEST(DeserializedNodeUbiNodes, {
     EXPECT_EQ(JS::ubi::CoarseType::Script, ubi.coarseType());
     EXPECT_EQ(id, ubi.identifier());
     EXPECT_FALSE(ubi.isLive());
-    EXPECT_EQ(strcmp(ubi.jsObjectClassName(), className), 0);
+    EXPECT_EQ(ubi.jsObjectClassName(), className);
 
     // Test the ubi::Node's edges.
 
     UniquePtr<DeserializedNode> referent1(new MockDeserializedNode(1,
                                                                    nullptr,
                                                                    10));
-    DeserializedEdge edge1;
+    DeserializedEdge edge1(referent1->id);
-    edge1.referent = referent1->id;
     mocked.addEdge(Move(edge1));
     EXPECT_CALL(mocked,
                 getEdgeReferent(Field(&DeserializedEdge::referent,
@@ -76,8 +74,7 @@ DEF_TEST(DeserializedNodeUbiNodes, {
     UniquePtr<DeserializedNode> referent2(new MockDeserializedNode(2,
                                                                    nullptr,
                                                                    20));
-    DeserializedEdge edge2;
+    DeserializedEdge edge2(referent2->id);
-    edge2.referent = referent2->id;
     mocked.addEdge(Move(edge2));
     EXPECT_CALL(mocked,
                 getEdgeReferent(Field(&DeserializedEdge::referent,
@@ -88,8 +85,7 @@ DEF_TEST(DeserializedNodeUbiNodes, {
     UniquePtr<DeserializedNode> referent3(new MockDeserializedNode(3,
                                                                    nullptr,
                                                                    30));
-    DeserializedEdge edge3;
+    DeserializedEdge edge3(referent3->id);
-    edge3.referent = referent3->id;
     mocked.addEdge(Move(edge3));
     EXPECT_CALL(mocked,
                 getEdgeReferent(Field(&DeserializedEdge::referent,
@@ -97,5 +93,5 @@ DEF_TEST(DeserializedNodeUbiNodes, {
       .Times(1)
       .WillOnce(Return(JS::ubi::Node(referent3.get())));
 
-    ubi.edges(JS_GetRuntime(cx));
+    ubi.edges(rt);
   });

devtools/shared/heapsnapshot/tests/gtest/DevTools.h

@@ -182,7 +182,7 @@ class Concrete<FakeNode> : public Base
     return concreteTypeName;
   }
 
-  UniquePtr<EdgeRange> edges(JSRuntime* rt, bool wantNames) const override {
+  UniquePtr<EdgeRange> edges(JSRuntime*, bool) const override {
     return UniquePtr<EdgeRange>(js_new<PreComputedEdgeRange>(get().edges));
   }
 
@@ -270,6 +270,14 @@ MATCHER_P(UTF16StrEq, str, "") {
   return NS_strcmp(arg, str) == 0;
 }
 
+MATCHER_P(UniqueUTF16StrEq, str, "") {
+  return NS_strcmp(arg.get(), str) == 0;
+}
+
+MATCHER(UniqueIsNull, "") {
+  return arg.get() == nullptr;
+}
+
 } // namespace testing
 
 

devtools/shared/heapsnapshot/tests/gtest/SerializesEdgeNames.cpp

@@ -30,11 +30,11 @@ DEF_TEST(SerializesEdgeNames, {
       writer,
       writeNode(AllOf(EdgesLength(rt, 3),
                       Edge(rt, 0, Field(&JS::ubi::Edge::name,
-                                        UTF16StrEq(edgeName))),
+                                        UniqueUTF16StrEq(edgeName))),
                       Edge(rt, 1, Field(&JS::ubi::Edge::name,
-                                        UTF16StrEq(emptyStr))),
+                                        UniqueUTF16StrEq(emptyStr))),
                       Edge(rt, 2, Field(&JS::ubi::Edge::name,
-                                        IsNull()))),
+                                        UniqueIsNull()))),
                 _)
     )
       .Times(1)

devtools/shared/heapsnapshot/tests/gtest/UniqueStringHashPolicy.cpp

@@ -1,25 +0,0 @@
-/* -*-  Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2; -*- */
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
-// Bug 1171226 - Test UniqueStringHashPolicy::match
-
-#include "DevTools.h"
-#include "mozilla/devtools/HeapSnapshot.h"
-
-using mozilla::devtools::UniqueString;
-using mozilla::devtools::UniqueStringHashPolicy;
-
-DEF_TEST(UniqueStringHashPolicy_match, {
-    //                                               1
-    //                                     01234567890123456
-    UniqueString str1(NS_strdup(MOZ_UTF16("some long string and a tail")));
-    ASSERT_TRUE(!!str1);
-
-    UniqueStringHashPolicy::Lookup lookup(MOZ_UTF16("some long string with same prefix"), 16);
-
-    // str1 is longer than Lookup.length, so they shouldn't match, even though
-    // the first 16 chars are equal!
-    ASSERT_FALSE(UniqueStringHashPolicy::match(str1, lookup));
-  });

devtools/shared/heapsnapshot/tests/gtest/moz.build

@@ -18,7 +18,6 @@ UNIFIED_SOURCES = [
     'SerializesEdgeNames.cpp',
     'SerializesEverythingInHeapGraphOnce.cpp',
     'SerializesTypeNames.cpp',
-    'UniqueStringHashPolicy.cpp',
 ]
 
 # THE MOCK_METHOD2 macro from gtest triggers this clang warning and it's hard

js/public/UbiNode.h

@@ -25,6 +25,7 @@
 #include "js/RootingAPI.h"
 #include "js/TracingAPI.h"
 #include "js/TypeDecls.h"
+#include "js/Value.h"
 #include "js/Vector.h"
 
 // JS::ubi::Node
@@ -186,6 +187,7 @@ class DefaultDelete<JS::ubi::StackFrame> : public JS::DeletePolicy<JS::ubi::Stac
 namespace JS {
 namespace ubi {
 
+using mozilla::Forward;
 using mozilla::Maybe;
 using mozilla::Move;
 using mozilla::RangedPtr;
@@ -199,7 +201,29 @@ using mozilla::Variant;
 // heap snapshots store their strings as const char16_t*. In order to provide
 // zero-cost accessors to these strings in a single interface that works with
 // both cases, we use this variant type.
-using AtomOrTwoByteChars = Variant<JSAtom*, const char16_t*>;
+class AtomOrTwoByteChars : public Variant<JSAtom*, const char16_t*> {
+    using Base = Variant<JSAtom*, const char16_t*>;
+
+  public:
+    template<typename T>
+    MOZ_IMPLICIT AtomOrTwoByteChars(T&& rhs) : Base(Forward<T>(rhs)) { }
+
+    template<typename T>
+    AtomOrTwoByteChars& operator=(T&& rhs) {
+        MOZ_ASSERT(this != &rhs, "self-move disallowed");
+        this->~AtomOrTwoByteChars();
+        new (this) AtomOrTwoByteChars(Forward<T>(rhs));
+        return *this;
+    }
+
+    // Return the length of the given AtomOrTwoByteChars string.
+    size_t length();
+
+    // Copy the given AtomOrTwoByteChars string into the destination buffer,
+    // inflating if necessary. Does NOT null terminate. Returns the number of
+    // characters written to destination.
+    size_t copyToBuffer(RangedPtr<char16_t> destination, size_t length);
+};
 
 // The base class implemented by each ConcreteStackFrame<T> type. Subclasses
 // must not add data members to this class.
@@ -786,23 +810,25 @@ class Node {
 
 /*** Edge and EdgeRange ***************************************************************************/
 
+using EdgeName = UniquePtr<const char16_t[], JS::FreePolicy>;
+
 // An outgoing edge to a referent node.
 class Edge {
   public:
     Edge() : name(nullptr), referent() { }
 
     // Construct an initialized Edge, taking ownership of |name|.
-    Edge(char16_t* name, const Node& referent) {
+    Edge(char16_t* name, const Node& referent)
-        this->name = name;
+        : name(name)
-        this->referent = referent;
+        , referent(referent)
-    }
+    { }
 
     // Move construction and assignment.
-    Edge(Edge&& rhs) {
+    Edge(Edge&& rhs)
-        name = rhs.name;
+        : name(mozilla::Move(rhs.name))
-        referent = rhs.referent;
+        , referent(rhs.referent)
-        rhs.name = nullptr;
+    { }
-    }
+
     Edge& operator=(Edge&& rhs) {
         MOZ_ASSERT(&rhs != this);
         this->~Edge();
@@ -810,10 +836,6 @@ class Edge {
         return *this;
     }
 
-    ~Edge() {
-        js_free(const_cast<char16_t*>(name));
-    }
-
     Edge(const Edge&) = delete;
     Edge& operator=(const Edge&) = delete;
 
@@ -826,7 +848,7 @@ class Edge {
     // (In real life we'll want a better representation for names, to avoid
     // creating tons of strings when the names follow a pattern; and we'll need
     // to think about lifetimes carefully to ensure traversal stays cheap.)
-    const char16_t* name;
+    EdgeName name;
 
     // This edge's referent.
     Node referent;

js/public/Utility.h

@@ -128,7 +128,7 @@ extern JS_PUBLIC_DATA(uint32_t) targetThread;
 static inline bool
 OOMThreadCheck()
 {
-    return (!js::oom::targetThread 
+    return (!js::oom::targetThread
             || js::oom::targetThread == js::oom::GetThreadType());
 }
 
@@ -435,15 +435,15 @@ namespace JS {
 template<typename T>
 struct DeletePolicy
 {
-    void operator()(T* ptr) {
+    void operator()(const T* ptr) {
-        js_delete(ptr);
+        js_delete(const_cast<T*>(ptr));
     }
 };
 
 struct FreePolicy
 {
-    void operator()(void* ptr) {
+    void operator()(const void* ptr) {
-        js_free(ptr);
+        js_free(const_cast<void*>(ptr));
     }
 };
 

js/src/builtin/TestingFunctions.cpp

@@ -2141,7 +2141,7 @@ struct FindPathHandler {
 
         // Record how we reached this node. This is the last edge on a
         // shortest path to this node.
-        EdgeName edgeName = DuplicateString(cx, edge.name);
+        EdgeName edgeName = DuplicateString(cx, edge.name.get());
         if (!edgeName)
             return false;
         *backEdge = mozilla::Move(BackEdge(origin, Move(edgeName)));

js/src/vm/UbiNode.cpp

@@ -52,16 +52,6 @@ using JS::ubi::StackFrame;
 using JS::ubi::TracerConcrete;
 using JS::ubi::TracerConcreteWithCompartment;
 
-template<typename CharT>
-static size_t
-copyToBuffer(const CharT* src, RangedPtr<char16_t> dest, size_t length)
-{
-    size_t i = 0;
-    for ( ; i < length; i++)
-        dest[i] = src[i];
-    return i;
-}
-
 struct CopyToBufferMatcher
 {
     using ReturnType = size_t;
@@ -74,6 +64,16 @@ struct CopyToBufferMatcher
       , maxLength(maxLength)
     { }
 
+    template<typename CharT>
+    static size_t
+    copyToBufferHelper(const CharT* src, RangedPtr<char16_t> dest, size_t length)
+    {
+        size_t i = 0;
+        for ( ; i < length; i++)
+            dest[i] = src[i];
+        return i;
+    }
+
     size_t
     match(JSAtom* atom)
     {
@@ -83,8 +83,8 @@ struct CopyToBufferMatcher
         size_t length = std::min(atom->length(), maxLength);
         JS::AutoCheckCannotGC noGC;
         return atom->hasTwoByteChars()
-            ? copyToBuffer(atom->twoByteChars(noGC), destination, length)
+            ? copyToBufferHelper(atom->twoByteChars(noGC), destination, length)
-            : copyToBuffer(atom->latin1Chars(noGC), destination, length);
+            : copyToBufferHelper(atom->latin1Chars(noGC), destination, length);
     }
 
     size_t
@@ -94,22 +94,15 @@ struct CopyToBufferMatcher
             return 0;
 
         size_t length = std::min(js_strlen(chars), maxLength);
-        return copyToBuffer(chars, destination, length);
+        return copyToBufferHelper(chars, destination, length);
     }
 };
 
 size_t
-StackFrame::source(RangedPtr<char16_t> destination, size_t length) const
+JS::ubi::AtomOrTwoByteChars::copyToBuffer(RangedPtr<char16_t> destination, size_t length)
 {
     CopyToBufferMatcher m(destination, length);
-    return source().match(m);
+    return match(m);
-}
-
-size_t
-StackFrame::functionDisplayName(RangedPtr<char16_t> destination, size_t length) const
-{
-    CopyToBufferMatcher m(destination, length);
-    return functionDisplayName().match(m);
 }
 
 struct LengthMatcher
@@ -130,17 +123,36 @@ struct LengthMatcher
 };
 
 size_t
-StackFrame::sourceLength()
+JS::ubi::AtomOrTwoByteChars::length()
 {
     LengthMatcher m;
-    return source().match(m);
+    return match(m);
+}
+
+size_t
+StackFrame::source(RangedPtr<char16_t> destination, size_t length) const
+{
+    auto s = source();
+    return s.copyToBuffer(destination, length);
+}
+
+size_t
+StackFrame::functionDisplayName(RangedPtr<char16_t> destination, size_t length) const
+{
+    auto name = functionDisplayName();
+    return name.copyToBuffer(destination, length);
+}
+
+size_t
+StackFrame::sourceLength()
+{
+    return source().length();
 }
 
 size_t
 StackFrame::functionDisplayNameLength()
 {
-    LengthMatcher m;
+    return functionDisplayName().length();
-    return functionDisplayName().match(m);
 }
 
 // All operations on null ubi::Nodes crash.

xpcom/glue/nsCRTGlue.cpp

@@ -128,17 +128,21 @@ NS_strdup(const char16_t* aString)
   return NS_strndup(aString, len);
 }
 
-char16_t*
+template<typename CharT>
-NS_strndup(const char16_t* aString, uint32_t aLen)
+CharT*
+NS_strndup(const CharT* aString, uint32_t aLen)
 {
-  char16_t* newBuf = (char16_t*)NS_Alloc((aLen + 1) * sizeof(char16_t));
+  auto newBuf = (CharT*)NS_Alloc((aLen + 1) * sizeof(CharT));
   if (newBuf) {
-    memcpy(newBuf, aString, aLen * sizeof(char16_t));
+    memcpy(newBuf, aString, aLen * sizeof(CharT));
     newBuf[aLen] = '\0';
   }
   return newBuf;
 }
 
+template char16_t* NS_strndup<char16_t>(const char16_t* aString, uint32_t aLen);
+template char* NS_strndup<char>(const char* aString, uint32_t aLen);
+
 char*
 NS_strdup(const char* aString)
 {

xpcom/glue/nsCRTGlue.h

@@ -63,10 +63,14 @@ char16_t* NS_strdup(const char16_t* aString);
 char* NS_strdup(const char* aString);
 
 /**
- * strndup for char16_t strings... this function will ensure that the
+ * strndup for char16_t or char strings (normal strndup is not available on
- * new string is null-terminated. Uses the NS_Alloc allocator.
+ * windows). This function will ensure that the new string is
+ * null-terminated. Uses the NS_Alloc allocator.
+ *
+ * CharT may be either char16_t or char.
  */
-char16_t* NS_strndup(const char16_t* aString, uint32_t aLen);
+template<typename CharT>
+CharT* NS_strndup(const CharT* aString, uint32_t aLen);
 
 // The following case-conversion methods only deal in the ascii repertoire
 // A-Z and a-z