CreateMinidumpsAndPair doesn't record memory information

RESOLVED FIXED in Firefox 48

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: benjamin, Assigned: ting)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(e10s+, firefox48 fixed, firefox49 fixed, firefox50 fixed)

Details

(Whiteboard: e10st?)

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

2 years ago
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)
tracking-e10s: --- → ?

Updated

2 years ago
tracking-e10s: ? → +
Priority: -- → P1
(Assignee)

Comment 2

2 years ago
Created attachment 8751140 [details] [diff] [review]
patch v1

I stole this bug because Kanru told me Ted seems quite busy.
Assignee: ted → janus926
Status: NEW → ASSIGNED
Attachment #8751140 - Flags: review?(benjamin)
(Assignee)

Comment 3

2 years ago
Created attachment 8751179 [details] [diff] [review]
patch v2

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)
(Reporter)

Comment 4

2 years ago
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)
(Assignee)

Comment 5

2 years ago
Created attachment 8752641 [details] [diff] [review]
part1 v1: add an extra argument for the minidump type to write on windows

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)
(Assignee)

Comment 6

2 years ago
Created attachment 8752642 [details] [diff] [review]
part2 v1: record memory information for minidumps on windows
Attachment #8752642 - Flags: review?(ted)
(Assignee)

Updated

2 years ago
Attachment #8752642 - Attachment description: part2 v2: record memory information for minidumps on windows → part2 v1: record memory information for minidumps on windows

Comment 7

2 years ago
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+
(Assignee)

Comment 10

2 years ago
Created attachment 8762439 [details] [diff] [review]
part1 v2: add an extra argument for the minidump type to write on windows
Attachment #8752641 - Attachment is obsolete: true
Attachment #8762439 - Flags: review?(ted)

Updated

2 years ago
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)
(Assignee)

Comment 14

2 years ago
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
(Assignee)

Comment 15

2 years ago
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
(Assignee)

Comment 16

2 years ago
Created attachment 8766604 [details] [diff] [review]
part2 v2: record memory information for minidumps on windows

Rebased.
Attachment #8752642 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 18

2 years ago
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

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3fc070ac7bc3
https://hg.mozilla.org/mozilla-central/rev/5ea00998d8a4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Ting-Yu, should we uplift your minidump fix to Aurora 49 or even Beta 48?
status-firefox48: --- → ?
status-firefox49: --- → ?
Flags: needinfo?(janus926)
(Assignee)

Comment 21

2 years ago
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)
(Assignee)

Comment 23

2 years ago
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?
(Assignee)

Comment 24

2 years ago
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)
(Assignee)

Comment 26

2 years ago
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+

Updated

2 years ago
status-firefox48: ? → affected
status-firefox49: ? → affected
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.