Bug 1218597 - Limit the number of stack frames serialized in core dumps; r=froydnj
authorNick Fitzgerald <fitzgen@gmail.com>
Tue, 27 Oct 2015 14:13:39 -0700
changeset 269821 f73d722759c1
parent 269820 7894e641adfa
child 269822 e90655d486ca
push id29592
push usercbook@mozilla.com
push dateWed, 28 Oct 2015 09:38:46 +0000
treeherdermozilla-central@872927368b0e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1218597
milestone44.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1218597 - Limit the number of stack frames serialized in core dumps; r=froydnj
devtools/shared/heapsnapshot/CoreDump.proto
devtools/shared/heapsnapshot/HeapSnapshot.cpp
devtools/shared/heapsnapshot/HeapSnapshot.h
devtools/shared/heapsnapshot/tests/unit/test_HeapSnapshot_deepStack_01.js
devtools/shared/heapsnapshot/tests/unit/xpcshell.ini
--- a/devtools/shared/heapsnapshot/CoreDump.proto
+++ b/devtools/shared/heapsnapshot/CoreDump.proto
@@ -105,17 +105,17 @@ message StackFrame {
 message Node {
     optional uint64     id                   = 1;
 
     // De-duplicated two-byte string.
     oneof TypeNameOrRef {
         bytes           typeName             = 2;
         uint64          typeNameRef          = 3;
     }
-    
+
     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;
--- a/devtools/shared/heapsnapshot/HeapSnapshot.cpp
+++ b/devtools/shared/heapsnapshot/HeapSnapshot.cpp
@@ -99,28 +99,38 @@ HeapSnapshot::Create(JSContext* cx,
 template<typename MessageType>
 static bool
 parseMessage(ZeroCopyInputStream& stream, MessageType& message)
 {
   // We need to create a new `CodedInputStream` for each message so that the
   // 64MB limit is applied per-message rather than to the whole stream.
   CodedInputStream codedStream(&stream);
 
+  // The protobuf message nesting that core dumps exhibit is dominated by
+  // allocation stacks' frames. In the most deeply nested case, each frame has
+  // two messages: a StackFrame message and a StackFrame::Data message. These
+  // frames are on top of a small constant of other messages. There are a
+  // MAX_STACK_DEPTH number of frames, so we multiply this by 3 to make room for
+  // the two messages per frame plus some head room for the constant number of
+  // non-dominating messages.
+  codedStream.SetRecursionLimit(HeapSnapshot::MAX_STACK_DEPTH * 3);
+
   // Because protobuf messages aren't self-delimiting, we serialize each message
   // preceeded by its size in bytes. When deserializing, we read this size and
   // then limit reading from the stream to the given byte size. If we didn't,
   // then the first message would consume the entire stream.
 
   uint32_t size = 0;
   if (NS_WARN_IF(!codedStream.ReadVarint32(&size)))
     return false;
 
   auto limit = codedStream.PushLimit(size);
   if (NS_WARN_IF(!message.ParseFromCodedStream(&codedStream)) ||
-      NS_WARN_IF(!codedStream.ConsumedEntireMessage()))
+      NS_WARN_IF(!codedStream.ConsumedEntireMessage()) ||
+      NS_WARN_IF(codedStream.BytesUntilLimit() != 0))
   {
     return false;
   }
 
   codedStream.PopLimit(limit);
   return true;
 }
 
@@ -904,17 +914,18 @@ class MOZ_STACK_CLASS StreamWriter : pub
     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) {
+  protobuf::StackFrame* getProtobufStackFrame(JS::ubi::StackFrame& frame,
+                                              size_t depth = 1) {
     // 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");
 
@@ -952,18 +963,18 @@ class MOZ_STACK_CLASS StreamWriter : pub
                                [&] (std::string* name) { data->set_allocated_functiondisplayname(name); },
                                [&] (uint64_t ref) { data->set_functiondisplaynameref(ref); }))
       {
         return nullptr;
       }
     }
 
     auto parent = frame.parent();
-    if (parent) {
-      auto protobufParent = getProtobufStackFrame(parent);
+    if (parent && depth < HeapSnapshot::MAX_STACK_DEPTH) {
+      auto protobufParent = getProtobufStackFrame(parent, depth + 1);
       if (!protobufParent)
         return nullptr;
       data->set_allocated_parent(protobufParent);
     }
 
     protobufStackFrame->set_allocated_data(data.release());
 
     if (!framesAlreadySerialized.put(id))
--- a/devtools/shared/heapsnapshot/HeapSnapshot.h
+++ b/devtools/shared/heapsnapshot/HeapSnapshot.h
@@ -66,16 +66,23 @@ class HeapSnapshot final : public nsISup
   bool saveNode(const protobuf::Node& node);
 
   // Save the given `protobuf::StackFrame` message in this `HeapSnapshot` as a
   // `DeserializedStackFrame`. The saved stack frame's id is returned via the
   // out parameter.
   bool saveStackFrame(const protobuf::StackFrame& frame,
                       StackFrameId& outFrameId);
 
+public:
+  // The maximum number of stack frames that we will serialize into a core
+  // dump. This helps prevent over-recursion in the protobuf library when
+  // deserializing stacks.
+  static const size_t MAX_STACK_DEPTH = 60;
+
+private:
   // If present, a timestamp in the same units that `PR_Now` gives.
   Maybe<uint64_t> timestamp;
 
   // The id of the root node for this deserialized heap graph.
   NodeId rootId;
 
   // The set of nodes in this deserialized heap graph, keyed by id.
   using NodeSet = js::HashSet<DeserializedNode, DeserializedNode::HashPolicy>;
new file mode 100644
--- /dev/null
+++ b/devtools/shared/heapsnapshot/tests/unit/test_HeapSnapshot_deepStack_01.js
@@ -0,0 +1,70 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+// Test that we can save a core dump with very deep allocation stacks and read
+// it back into a HeapSnapshot.
+
+function stackDepth(stack) {
+  return stack ? 1 + stackDepth(stack.parent) : 0;
+}
+
+function run_test() {
+  // Create a Debugger observing a debuggee's allocations.
+  const debuggee = new Cu.Sandbox(null);
+  const dbg = new Debugger(debuggee);
+  dbg.memory.trackingAllocationSites = true;
+
+  // Allocate some objects in the debuggee that will have their allocation
+  // stacks recorded by the Debugger.
+
+  debuggee.eval("this.objects = []");
+  debuggee.eval(
+    (function recursiveAllocate(n) {
+      if (n <= 0)
+        return;
+
+      // Make sure to recurse before pushing the object so that when TCO is
+      // implemented sometime in the future, it doesn't invalidate this test.
+      recursiveAllocate(n - 1);
+      this.objects.push({});
+    }).toString()
+  );
+  debuggee.eval("recursiveAllocate = recursiveAllocate.bind(this);");
+  debuggee.eval("recursiveAllocate(200);");
+
+  // Now save a snapshot that will include the allocation stacks and read it
+  // back again.
+
+  const filePath = ChromeUtils.saveHeapSnapshot({ runtime: true });
+  ok(true, "Should be able to save a snapshot.");
+
+  const snapshot = ChromeUtils.readHeapSnapshot(filePath);
+  ok(snapshot, "Should be able to read a heap snapshot");
+  ok(snapshot instanceof HeapSnapshot, "Should be an instanceof HeapSnapshot");
+
+  const report = snapshot.takeCensus({
+    breakdown: { by: "allocationStack",
+                 then: { by: "count", bytes: true, count: true },
+                 noStack: { by: "count", bytes: true, count: true }
+               }
+  });
+
+  // Keep this synchronized with `HeapSnapshot::MAX_STACK_DEPTH`!
+  const MAX_STACK_DEPTH = 60;
+
+  let foundStacks = false;
+  report.forEach((v, k) => {
+    if (k === "noStack") {
+      return;
+    }
+
+    foundStacks = true;
+    const depth = stackDepth(k);
+    dumpn("Stack depth is " + depth);
+    ok(depth <= MAX_STACK_DEPTH,
+       "Every stack should have depth less than or equal to the maximum stack depth");
+  });
+  ok(foundStacks);
+
+  do_test_finished();
+}
--- a/devtools/shared/heapsnapshot/tests/unit/xpcshell.ini
+++ b/devtools/shared/heapsnapshot/tests/unit/xpcshell.ini
@@ -29,16 +29,17 @@ support-files =
 [test_HeapAnalyses_takeCensus_01.js]
 [test_HeapAnalyses_takeCensus_02.js]
 [test_HeapAnalyses_takeCensus_03.js]
 [test_HeapAnalyses_takeCensus_04.js]
 [test_HeapAnalyses_takeCensus_05.js]
 [test_HeapAnalyses_takeCensus_06.js]
 [test_HeapAnalyses_takeCensus_07.js]
 [test_HeapSnapshot_creationTime_01.js]
+[test_HeapSnapshot_deepStack_01.js]
 [test_HeapSnapshot_takeCensus_01.js]
 [test_HeapSnapshot_takeCensus_02.js]
 [test_HeapSnapshot_takeCensus_03.js]
 [test_HeapSnapshot_takeCensus_04.js]
 [test_HeapSnapshot_takeCensus_05.js]
 [test_HeapSnapshot_takeCensus_06.js]
 [test_HeapSnapshot_takeCensus_07.js]
 [test_HeapSnapshot_takeCensus_08.js]