Closed Bug 1257486 Opened 4 years ago Closed 4 years ago

Add additional memory annotations to content process crash reports

Categories

(Toolkit :: Crash Reporting, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: jonathan, Assigned: cyu)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [MemShrink:P1])

Attachments

(1 file)

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.
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?
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]
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.
Summary: Crash report memory information not implemented for child processes (content processes) → Add additional memory annotations to content process crash reports
Assignee: nobody → cyu
Flags: needinfo?(kchen)
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?
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/
Attachment #8735815 - Flags: review?(benjamin)
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.
Attachment #8735815 - Flags: review-
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)
https://reviewboard.mozilla.org/r/42953/#review40381

Updated with 2 xpcshell test cases added and locally tested.
Flags: needinfo?(cyu)
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 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+
https://hg.mozilla.org/mozilla-central/rev/4f662b15f40b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Cervantes, should this fix be uplifted to Aurora 47 in preparation for our e10s experiments on Beta 47?
Flags: needinfo?(cyu)
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?
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+
You need to log in before you can comment on or make changes to this bug.