1218597 - NS_ERROR_UNEXPECTED when reading heap snapshot

Status: RESOLVED FIXED

(DevTools :: Memory, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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
> }

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Limit the number of stack frames serialized in core dumps

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=28715285ee89

Comment 6

[Nathan Froyd [:froydnj]]

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?

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f73d722759c1

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f73d722759c1