Closed
Bug 1218597
Opened 8 years ago
Closed 8 years ago
NS_ERROR_UNEXPECTED when reading heap snapshot
Categories
(DevTools :: Memory, defect)
DevTools
Memory
Tracking
(firefox44 fixed)
RESOLVED
FIXED
Firefox 44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: fitzgen, Assigned: fitzgen)
References
Details
Attachments
(1 file)
8.41 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•8 years ago
|
||
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
Assignee | ||
Comment 2•8 years ago
|
||
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
> }
Assignee | ||
Comment 3•8 years ago
|
||
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.
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8679564 -
Flags: review?(nfroyd)
Assignee | ||
Comment 5•8 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=28715285ee89
![]() |
||
Comment 6•8 years ago
|
||
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 8•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f73d722759c1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Updated•5 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•