Closed Bug 1218597 Opened 8 years ago Closed 8 years ago

NS_ERROR_UNEXPECTED when reading heap snapshot

Categories

(DevTools :: Memory, defect)

defect
Not set
normal

Tracking

(firefox44 fixed)

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(1 file)

STR:

* Open https://swannodette.github.io/todomvc/labs/architecture-examples/om/
* Enable allocations tracking
* Invert tree
* Select by allocation stack breakdown
* Click "benchmark 1" a few times
* Take a heap snapshot

ER:

Successfully save and read a heap snapshot

AR:

NS_ERROR_UNEXPECTED
Looks like we are either generating bad protobuf messages, or misreading them.

> [Parent 98975] WARNING: '!message.ParseFromCodedStream(&codedStream)', file /Users/fitzgen/src/mozilla-central/devtools/shared/heapsnapshot/HeapSnapshot.cpp, line 117
But it looks like we are trying to keep reading even though we have reached the end...

> (gdb) p stream
> $6 = (google::protobuf::io::ArrayInputStream) {
> <google::protobuf::io::ZeroCopyInputStream> = {
> _vptr$ZeroCopyInputStream = 0x10b7a86b0
> },
> members of google::protobuf::io::ArrayInputStream: 
> data_ = 0x11da00000, 
> size_ = 691935, 
> block_size_ = 691935, 
> position_ = 691935, 
> last_returned_size_ = 691935
> }
Aha! What is happening is really deep allocation stacks that are triggering protobuf's recursion limit. We can set the recursion limit ourselves (google::protobuf::io::CodedInputStream::SetRecursionLimit), but I'm not sure what a good default is. We should probably also limit the stack frame depth we serialize to help avoid this issue in practice.

And with that, I'm going to call it a night.
Comment on attachment 8679564 [details] [diff] [review]
Limit the number of stack frames serialized in core dumps

Review of attachment 8679564 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with optional change below.

::: devtools/shared/heapsnapshot/tests/unit/test_HeapSnapshot_deepStack_01.js
@@ +35,5 @@
> +  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");

It might be worth checking that we truncated the allocation stack here, so that if we ever updated MAX_STACK_DEPTH, we would ideally see that this test failed because we're not truncating anymore?
Attachment #8679564 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/f73d722759c1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.