Closed Bug 1267329 Opened 5 years ago Closed 5 years ago

CreateMinidumpsAndPair doesn't record memory information

Categories

(Toolkit :: Crash Reporting, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
e10s + ---
firefox48 --- fixed
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: benjamin, Assigned: ting)

References

Details

(Whiteboard: e10st?)

Attachments

(2 files, 4 obsolete files)

CrashReporter::CreateMinidumpsAndPair is used in e10s to record the state of a child process, through CrashReporterParent::GenerateCompleteMinidump.

This function does not have the logic to record the memory information with a minidump, which can be valuable for diagnostics.

http://hg.mozilla.org/mozilla-central/annotate/1152d99d8c53/toolkit/crashreporter/google-breakpad/src/client/windows/handler/exception_handler.cc#l808 is the location where this might be fixed: you have to initialize the ExceptionHandler with an extra argument to specify the minidump type. We could then pass GetMinidumpType() from nsExceptionHandler.cpp to ExceptionHandler::WriteMinidump.

Ted, do you have bandwidth to take this?
Flags: needinfo?(ted)
Yeah, this should be straightforward.
Assignee: nobody → ted
Flags: needinfo?(ted)
Priority: -- → P1
Attached patch patch v1 (obsolete) — Splinter Review
I stole this bug because Kanru told me Ted seems quite busy.
Assignee: ted → janus926
Status: NEW → ASSIGNED
Attachment #8751140 - Flags: review?(benjamin)
Attached patch patch v2 (obsolete) — Splinter Review
Fixed the other places calling WriteMinidump() and WriteMinidumpForChild(), but I am not sure do they need GetMinidumpType().
Attachment #8751140 - Attachment is obsolete: true
Attachment #8751140 - Flags: review?(benjamin)
Attachment #8751179 - Flags: review?(benjamin)
Comment on attachment 8751179 [details] [diff] [review]
patch v2

This is a patch to google-breakpad, so it needs to be submitted upstream since we stay in sync. I am not a breakpad reviewer.
Attachment #8751179 - Flags: review?(benjamin)
Separate the patch to two parts, one for google breakpad and one for nsExceptionHandler.
Attachment #8751179 - Attachment is obsolete: true
Attachment #8752641 - Flags: review?(ted)
Attachment #8752642 - Attachment description: part2 v2: record memory information for minidumps on windows → part2 v1: record memory information for minidumps on windows
ted - ping?
Comment on attachment 8752641 [details] [diff] [review]
part1 v1: add an extra argument for the minidump type to write on windows

Review of attachment 8752641 [details] [diff] [review]:
-----------------------------------------------------------------

This is probably fine, but it needs to go upstream. Can you give me a patch against upstream Breakpad?
https://chromium.googlesource.com/breakpad/breakpad/+/master/
Attachment #8752641 - Flags: review?(ted)
Comment on attachment 8752642 [details] [diff] [review]
part2 v1: record memory information for minidumps on windows

Review of attachment 8752642 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine once we get the Breakpad part sorted out.
Attachment #8752642 - Flags: review?(ted) → review+
Attachment #8752641 - Attachment is obsolete: true
Attachment #8762439 - Flags: review?(ted)
Whiteboard: e10st?
Comment on attachment 8762439 [details] [diff] [review]
part1 v2: add an extra argument for the minidump type to write on windows

Review of attachment 8762439 [details] [diff] [review]:
-----------------------------------------------------------------

This should be fine. I'll get it landed upstream for you. If you can submit a patch to the Breakpad code review tool that will make my life easier, but if not I will take care of it:
https://chromium.googlesource.com/breakpad/breakpad/+/master/#To-request-change-review
Attachment #8762439 - Flags: review?(ted) → review+
Landed upstream:
https://chromium.googlesource.com/breakpad/breakpad/+/dfd2da797999be085d55ae9430f2a434788652e3%5E%21/

Feel free to land this in m-c now, as well as close that codereview issue. Thanks for the patch against upstream!
Flags: needinfo?(ted)
Comment on attachment 8752641 [details] [diff] [review]
part1 v1: add an extra argument for the minidump type to write on windows

Still need this for m-c.
Attachment #8752641 - Attachment is obsolete: false
Comment on attachment 8762439 [details] [diff] [review]
part1 v2: add an extra argument for the minidump type to write on windows

Obsoleted so sheriff won't get confused as this is for upstream.
Attachment #8762439 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/3fc070ac7bc3
part 1 - Add a new argument to specify the minidump type to write on Windows. r=ted
https://hg.mozilla.org/integration/fx-team/rev/5ea00998d8a4
part 2 - Record the memory information for the minidumps on Windows. r=ted
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3fc070ac7bc3
https://hg.mozilla.org/mozilla-central/rev/5ea00998d8a4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Ting-Yu, should we uplift your minidump fix to Aurora 49 or even Beta 48?
Flags: needinfo?(janus926)
I think we should. Ted, are there any concerns if we uplift the breakpad changes to 49 or 48?
Flags: needinfo?(janus926) → needinfo?(ted)
Those are very simple changes, they should be fine to uplift.
Flags: needinfo?(ted)
Comment on attachment 8752641 [details] [diff] [review]
part1 v1: add an extra argument for the minidump type to write on windows

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: none
[Describe test coverage new/current, TreeHerder]: covered by existing tests
[Risks and why]: low, add a new argument of dump type for writing minidump
[String/UUID change made/needed]: none
Attachment #8752641 - Flags: approval-mozilla-beta?
Attachment #8752641 - Flags: approval-mozilla-aurora?
Comment on attachment 8766604 [details] [diff] [review]
part2 v2: record memory information for minidumps on windows

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: none
[Describe test coverage new/current, TreeHerder]: covered by existing tests
[Risks and why]: low, it just passes one more argument (dump type) to the handler writing minidump
[String/UUID change made/needed]: none
Attachment #8766604 - Flags: approval-mozilla-beta?
Attachment #8766604 - Flags: approval-mozilla-aurora?
Hi Ting-Yu,
What is the user impact if we only let this ride the train on 49/50 and won't fix in 48?
Flags: needinfo?(janus926)
There's no user impact but engineer impact. The child process's minidump won't contain memory info for diagnostics work.
Flags: needinfo?(janus926)
Comment on attachment 8766604 [details] [diff] [review]
part2 v2: record memory information for minidumps on windows

Review of attachment 8766604 [details] [diff] [review]:
-----------------------------------------------------------------

This patch can help engineer to better diagnose work. Take it in 48 beta 9 and aurora.
Attachment #8766604 - Flags: approval-mozilla-beta?
Attachment #8766604 - Flags: approval-mozilla-beta+
Attachment #8766604 - Flags: approval-mozilla-aurora?
Attachment #8766604 - Flags: approval-mozilla-aurora+
Comment on attachment 8752641 [details] [diff] [review]
part1 v1: add an extra argument for the minidump type to write on windows

Review of attachment 8752641 [details] [diff] [review]:
-----------------------------------------------------------------

Same as above.
Attachment #8752641 - Flags: approval-mozilla-beta?
Attachment #8752641 - Flags: approval-mozilla-beta+
Attachment #8752641 - Flags: approval-mozilla-aurora?
Attachment #8752641 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.