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 269711 f73d722759c1
parent 269710 7894e641adfa
child 269712 e90655d486ca
push id15915
push usernfitzgerald@mozilla.com
push dateTue, 27 Oct 2015 21:16:38 +0000
treeherderfx-team@f73d722759c1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1218597
milestone44.0a1
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]