1267329 - CreateMinidumpsAndPair doesn't record memory information

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect, P1)

Description

[Benjamin Smedberg]

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?

Comment 1

[(not currently active) Ted Mielczarek]

Yeah, this should be straightforward.

Comment 2

[Ting-Yu Chou [:ting] (away)]

Attachment: patch v1

I stole this bug because Kanru told me Ted seems quite busy.

Comment 3

[Ting-Yu Chou [:ting] (away)]

Attachment: patch v2

Fixed the other places calling WriteMinidump() and WriteMinidumpForChild(), but I am not sure do they need GetMinidumpType().

Comment 4

[Benjamin Smedberg]

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.

Comment 5

[Ting-Yu Chou [:ting] (away)]

Attachment: 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.

Comment 6

[Ting-Yu Chou [:ting] (away)]

Attachment: part2 v1: record memory information for minidumps on windows

Comment 7

[Jim Mathies [:jimm]]

ted - ping?

Comment 8

[(not currently active) Ted Mielczarek]

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/

Comment 9

[(not currently active) Ted Mielczarek]

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.

Comment 10

[Ting-Yu Chou [:ting] (away)]

Attachment: part1 v2: add an extra argument for the minidump type to write on windows

Comment 11

[(not currently active) Ted Mielczarek]

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

Comment 12

[Ting-Yu Chou [:ting] (away)]

https://codereview.chromium.org/2107083002

Comment 13

[(not currently active) Ted Mielczarek]

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!

Comment 14

[Ting-Yu Chou [:ting] (away)]

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.

Comment 15

[Ting-Yu Chou [:ting] (away)]

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.

Comment 16

[Ting-Yu Chou [:ting] (away)]

Attachment: part2 v2: record memory information for minidumps on windows

Rebased.

Comment 17

[Ting-Yu Chou [:ting] (away)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=742df98d82d5

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3fc070ac7bc3
https://hg.mozilla.org/mozilla-central/rev/5ea00998d8a4

Comment 20

[Chris Peterson [:cpeterson]]

Ting-Yu, should we uplift your minidump fix to Aurora 49 or even Beta 48?

Comment 21

[Ting-Yu Chou [:ting] (away)]

I think we should. Ted, are there any concerns if we uplift the breakpad changes to 49 or 48?

Comment 22

[(not currently active) Ted Mielczarek]

Those are very simple changes, they should be fine to uplift.

Comment 23

[Ting-Yu Chou [:ting] (away)]

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

Comment 24

[Ting-Yu Chou [:ting] (away)]

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

Comment 25

[Gerry Chang [:gchang]]

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?

Comment 26

[Ting-Yu Chou [:ting] (away)]

There's no user impact but engineer impact. The child process's minidump won't contain memory info for diagnostics work.

Comment 27

[Gerry Chang [:gchang]]

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.

Comment 28

[Gerry Chang [:gchang]]

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.

Comment 29

[Carsten Book [:Tomcat]]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/01b86a0a32b2
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/c1cee3d09904

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/9dc6814ac5ba
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/85112a93cf1d