Bug 1258236 - Change the way we test for end-of-stream in ReadHeapSnapshot; r=sfink
authorNick Fitzgerald <fitzgen@gmail.com>
Fri, 25 Mar 2016 14:53:00 +0100
changeset 290707 334c1b88d28a46e3ebefbd42a55190bee4b3fefc
parent 290706 e5c8ba7cabaad7382c9b7f6b54fa10b268e1e1cc
child 290708 3357cb502d0c5a987083a3d0ea196d3d77fa12bd
push id19656
push usergwagner@mozilla.com
push dateMon, 04 Apr 2016 13:43:23 +0000
treeherderb2g-inbound@e99061fde28a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink
bugs1258236
milestone48.0a1
Bug 1258236 - Change the way we test for end-of-stream in ReadHeapSnapshot; r=sfink Fold reading the size of the next message and testing whether the stream has more data into the same operation. MozReview-Commit-ID: GUJymKy7qsj
devtools/shared/heapsnapshot/HeapSnapshot.cpp
--- a/devtools/shared/heapsnapshot/HeapSnapshot.cpp
+++ b/devtools/shared/heapsnapshot/HeapSnapshot.cpp
@@ -102,41 +102,32 @@ HeapSnapshot::Create(JSContext* cx,
     rv.Throw(NS_ERROR_UNEXPECTED);
     return nullptr;
   }
   return snapshot.forget();
 }
 
 template<typename MessageType>
 static bool
-parseMessage(ZeroCopyInputStream& stream, MessageType& message)
+parseMessage(ZeroCopyInputStream& stream, uint32_t sizeOfMessage, 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);
+  auto limit = codedStream.PushLimit(sizeOfMessage);
   if (NS_WARN_IF(!message.ParseFromCodedStream(&codedStream)) ||
       NS_WARN_IF(!codedStream.ConsumedEntireMessage()) ||
       NS_WARN_IF(codedStream.BytesUntilLimit() != 0))
   {
     return false;
   }
 
   codedStream.PopLimit(limit);
@@ -386,60 +377,54 @@ HeapSnapshot::saveStackFrame(const proto
 
   outFrameId = id;
   return true;
 }
 
 #undef GET_STRING_OR_REF_WITH_PROP_NAMES
 #undef GET_STRING_OR_REF
 
-static inline bool
-StreamHasData(GzipInputStream& stream)
+// Because protobuf messages aren't self-delimiting, we serialize each message
+// preceded 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.
+static bool
+readSizeOfNextMessage(ZeroCopyInputStream& stream, uint32_t* sizep)
 {
-  // Test for the end of the stream. The protobuf library gives no way to tell
-  // the difference between an underlying read error and the stream being
-  // done. All we can do is attempt to read data and extrapolate guestimations
-  // from the result of that operation.
-
-  const void* buf;
-  int size;
-  bool more = stream.Next(&buf, &size);
-  if (!more)
-    // Could not read any more data. We are optimistic and assume the stream is
-    // just exhausted and there is not an underlying IO error, since this
-    // function is only called at message boundaries.
-    return false;
-
-  // There is more data still available in the stream. Return the data we read
-  // to the stream and let the parser get at it.
-  stream.BackUp(size);
-  return true;
+  MOZ_ASSERT(sizep);
+  CodedInputStream codedStream(&stream);
+  return codedStream.ReadVarint32(sizep) && *sizep > 0;
 }
 
 bool
 HeapSnapshot::init(JSContext* cx, const uint8_t* buffer, uint32_t size)
 {
   if (!nodes.init() || !frames.init())
     return false;
 
   ArrayInputStream stream(buffer, size);
   GzipInputStream gzipStream(&stream);
+  uint32_t sizeOfMessage = 0;
 
   // First is the metadata.
 
   protobuf::Metadata metadata;
-  if (!parseMessage(gzipStream, metadata))
+  if (NS_WARN_IF(!readSizeOfNextMessage(gzipStream, &sizeOfMessage)))
+    return false;
+  if (!parseMessage(gzipStream, sizeOfMessage, metadata))
     return false;
   if (metadata.has_timestamp())
     timestamp.emplace(metadata.timestamp());
 
   // Next is the root node.
 
   protobuf::Node root;
-  if (!parseMessage(gzipStream, root))
+  if (NS_WARN_IF(!readSizeOfNextMessage(gzipStream, &sizeOfMessage)))
+    return false;
+  if (!parseMessage(gzipStream, sizeOfMessage, root))
     return false;
 
   // Although the id is optional in the protobuf format for future proofing, we
   // can't currently do anything without it.
   if (NS_WARN_IF(!root.has_id()))
     return false;
   rootId = root.id();
 
@@ -448,19 +433,23 @@ HeapSnapshot::init(JSContext* cx, const 
   if (NS_WARN_IF(!edgeReferents.init()))
     return false;
 
   if (NS_WARN_IF(!saveNode(root, edgeReferents)))
     return false;
 
   // Finally, the rest of the nodes in the core dump.
 
-  while (StreamHasData(gzipStream)) {
+  // Test for the end of the stream. The protobuf library gives no way to tell
+  // the difference between an underlying read error and the stream being
+  // done. All we can do is attempt to read the size of the next message and
+  // extrapolate guestimations from the result of that operation.
+  while (readSizeOfNextMessage(gzipStream, &sizeOfMessage)) {
     protobuf::Node node;
-    if (!parseMessage(gzipStream, node))
+    if (!parseMessage(gzipStream, sizeOfMessage, node))
       return false;
     if (NS_WARN_IF(!saveNode(node, edgeReferents)))
       return false;
   }
 
   // Check the set of node ids referred to by edges we found and ensure that we
   // have the node corresponding to each id. If we don't have all of them, it is
   // unsafe to perform analyses of this heap snapshot.