Add additional memory annotations to content process crash reports

RESOLVED FIXED in Firefox 47

Status

()

Toolkit
Crash Reporting
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Jonathan Howard, Assigned: cyu)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox46 wontfix, firefox47 fixed, firefox48 fixed)

Details

(Whiteboard: [MemShrink:P1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
Bug 1236108 adds support for OOM Allocation Size and changing signature unknown to small/large. This clone is for the additional information it does not add;
TotalPhysicalMemory
TotalVirtualMemory
Total Virtual Memory
Available Virtual Memory
System Memory Use Percentage
Available Page File
Available Physical Memory

TotalPhysicalMemory may be of use assessing stability of e10s.

+++ This bug was initially created as a clone of Bug #1236108 +++

While looking at the crash reports in bug 1235633, I noticed that crash signature is "OOM | unknown" rather than "OOM | large" or "OOM | small", which is odd, because I've seen a crash at the same allocation site before and it was annotated properly. Then I looked at the individual crash signatures and none of the ones I looked at have the nice memory information (Total Virtual Memory, Available Virtual Memory, System Memory Use Percentage, etc.) that we usually get in a crash report. I'm guessing the "unknown" results from the allocation size field being missing, too.

If you click on the "experiment" link in bug 1234647 comment 0, every OOM crash signature is "unknown", so it looks like this isn't working at all.

The odd thing is, if I look at the top 300 crashes from Nightly or Aurora, there are no "OOM | unknown", so I'm not sure if there's a problem specific to beta with these memory annotations or what.

These memory annotations are important to understanding our OOM crashes.

Comment 1

2 years ago
https://github.com/mozilla/socorro/blob/651afa2a52a08981102ddbcec9806e2193c2207d/socorro/processor/signature_utilities.py#L849 is the code that builds "OOM | ..." signatures in Socorro. The code that assigns "small" or "large" fully depends on the OOMAllocationSize annotation only. Can you poinjt me to a crash that wrongly gets "OOM | unknown | ..." instead of something else?

Updated

2 years ago
tracking-e10s: ? → +
Priority: -- → P1
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #1)
> Can you poinjt me to a crash that wrongly
> gets "OOM | unknown | ..." instead of something else?

Ignore everything in comment 0 after "+++ This bug was initially created as a clone of Bug #1236108 +++". That's just copied from bug 1236108. aklotz fixed the OOM | Unknown issue. This bug is about the other memory fields that are added to crash reports.
Whiteboard: [MemShrink:P?] → [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P1]

Comment 3

2 years ago
Kanru, can someone on your team pick this up?
Flags: needinfo?(kchen)
Most of those "OOM | unknown" I expect to be mitigated by bug 1256541 which is sitting on inbound. But yes, the additional annotations would be useful.

Updated

2 years ago
Summary: Crash report memory information not implemented for child processes (content processes) → Add additional memory annotations to content process crash reports
Blocks: 1259512
Assignee: nobody → cyu
Flags: needinfo?(kchen)
(Assignee)

Comment 5

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e3ab0c47c78
(Assignee)

Comment 6

2 years ago
Created attachment 8735815 [details]
MozReview Request: Bug 1257486 - Annotate global memory status in the crash reporter for child process; r?bsmedberg

Review commit: https://reviewboard.mozilla.org/r/42953/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42953/
Attachment #8735815 - Flags: review?(benjamin)
https://reviewboard.mozilla.org/r/42953/#review40083

::: toolkit/crashreporter/nsExceptionHandler.cpp:1285
(Diff revision 1)
>  
>    // Now open the file...
>    PlatformWriter apiData;
>    OpenAPIData(apiData, tempPath);
>  
> +#ifdef XP_WIN

Can you please move this below the comment that is currently underneath it?
(Assignee)

Comment 8

2 years ago
Comment on attachment 8735815 [details]
MozReview Request: Bug 1257486 - Annotate global memory status in the crash reporter for child process; r?bsmedberg

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42953/diff/1-2/

Updated

2 years ago
Attachment #8735815 - Flags: review?(benjamin)

Comment 9

2 years ago
Comment on attachment 8735815 [details]
MozReview Request: Bug 1257486 - Annotate global memory status in the crash reporter for child process; r?bsmedberg

https://reviewboard.mozilla.org/r/42953/#review40381

Please add a test for this. A mochitest-browser that loads a remote tab, crashes it intentionally, and then checks the crash report metadata for the memory stats should be possible.

Updated

2 years ago
Attachment #8735815 - Flags: review-

Comment 10

2 years ago
For diagnosing in beta, we want to get this uplifted to 47 before the next merge. How are we doing on a test here cyu?
Flags: needinfo?(cyu)
(Assignee)

Comment 11

2 years ago
https://reviewboard.mozilla.org/r/42953/#review40381

Updated with 2 xpcshell test cases added and locally tested.
(Assignee)

Updated

2 years ago
Flags: needinfo?(cyu)
(Assignee)

Comment 12

2 years ago
Comment on attachment 8735815 [details]
MozReview Request: Bug 1257486 - Annotate global memory status in the crash reporter for child process; r?bsmedberg

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42953/diff/2-3/
Attachment #8735815 - Flags: review- → review?(benjamin)

Comment 13

2 years ago
Comment on attachment 8735815 [details]
MozReview Request: Bug 1257486 - Annotate global memory status in the crash reporter for child process; r?bsmedberg

https://reviewboard.mozilla.org/r/42953/#review41275

r=bsmedberg ty!
Attachment #8735815 - Flags: review?(benjamin) → review+

Comment 14

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f662b15f40b

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4f662b15f40b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Cervantes, should this fix be uplifted to Aurora 47 in preparation for our e10s experiments on Beta 47?
status-firefox46: --- → wontfix
status-firefox47: --- → ?
Flags: needinfo?(cyu)
(Assignee)

Comment 17

2 years ago
Comment on attachment 8735815 [details]
MozReview Request: Bug 1257486 - Annotate global memory status in the crash reporter for child process; r?bsmedberg

Approval Request Comment
[Feature/regressing bug #]: No bug. This adds more annotations on OOM to gather information on crash.
[User impact if declined]: Less information in diagnosing OOM crashes.
[Describe test coverage new/current, TreeHerder]: Just landed/tested on m-c.
[Risks and why]: Pretty low since the patch contains only diagnostic code during crashes.
[String/UUID change made/needed]:
Flags: needinfo?(cyu)
Attachment #8735815 - Flags: approval-mozilla-aurora?

Updated

2 years ago
status-firefox47: ? → affected
Comment on attachment 8735815 [details]
MozReview Request: Bug 1257486 - Annotate global memory status in the crash reporter for child process; r?bsmedberg

Improves OOM crash diagnostics info, Aurora47+
Attachment #8735815 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 19

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/82a2aaaabc06
status-firefox47: affected → fixed
You need to log in before you can comment on or make changes to this bug.