NS_ERROR_UNEXPECTED when reading heap snapshot

RESOLVED FIXED in Firefox 44

Status

()

Firefox
Developer Tools: Memory
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

unspecified
Firefox 44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment)

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.
Created attachment 8679564 [details] [diff] [review]
Limit the number of stack frames serialized in core dumps
Attachment #8679564 - Flags: review?(nfroyd)
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=28715285ee89
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+

Comment 7

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/f73d722759c1
https://hg.mozilla.org/mozilla-central/rev/f73d722759c1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in before you can comment on or make changes to this bug.