620974 - Add memory mapping info to minidumps

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[(not currently active) Ted Mielczarek]

We discussed this at the all hands last week as something we could add to minidumps to assist debugging of JIT crashes. Turns out on Windows, MinidumpWriteDump already supports this (as of a fairly recent version), so it's just a matter of passing a new flag to the function:
http://msdn.microsoft.com/en-us/library/ms680519%28v=VS.85%29.aspx
"MinidumpWithFullMemoryInfo".

I believe new enough Microsoft debuggers should be able to interpret this info out of the box, but you might need to use WinDBG.

Comment 1

[(not currently active) Ted Mielczarek]

Okay, this is a trivial fix. One downside is that this will bloat our minidumps slightly. In a test crash (with Crash Me Now), the dump went from 134Kb -> 205Kb, so about 1.5x larger. This isn't a problem individually, but in aggregate it does increase our dump storage requirements, which probably won't make the Socorro folks happy. I think we should at least make this change for the beta cycle to get the extra post-mortem debugging info, and we can switch it off for the RC if the minidump size becomes a problem.

Sample crash:
http://people.mozilla.com/~tmielczarek/crashme-full-memory-info.dmp
Compare vs:
http://people.mozilla.com/~tmielczarek/crashme-standard.dmp

If you load crashme-full-memory-info.dmp into WinDBG, you can use the !valist and !vprot commands to view this additional info (!valist shows all of it, !vaprot lets you ask about a particular address). I don't know how to access the data from Visual Studio.

I'll attach a patch in a minute.

Comment 2

[(not currently active) Ted Mielczarek]

Attachment: Add memory mapping info to Windows minidumps

Simple patch, just switches to use the alternate constructor for
ExceptionHandler that allows passing in the MINIDUMP_TYPE parameter. The other
two parameters have to do with out-of-process dump generation, passing them as
NULL makes them effectively ignored. The fewer-argument constructor we were
previously using would just pass them down as NULL internally anyway.

Comment 3

[(not currently active) Ted Mielczarek]

I forgot to add info with my approval request. This patch adds extra info to our Windows minidumps that should help diagnosing JIT crashes. This is something the JS developers and I discussed at the all hands last week.

Comment 4

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/f7a6e3530d99

Comment 5

[Timothy Nikkel (:tnikkel)]

Had to back this out
http://hg.mozilla.org/mozilla-central/rev/84745bfb592e
for xpcshell failures
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1293225343.1293228398.18201.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1293214394.1293217204.10151.gz

Comment 6

[(not currently active) Ted Mielczarek]

Attachment: Add memory mapping info to Windows minidumps.

This is sort of a WIP, but it works on Win2k3. Apparently you can't pass newer MINIDUMP_TYPE values to an older version of DbgHelp, it chokes on them. I'm explicitly version checking here, since I don't know of a cleaner way to do it.

Comment 7

[timeless]

Comment on attachment 501758 [details] [diff] [review]
Add memory mapping info to Windows minidumps.

yeah, this is correct.

it's actually relatively ok to hold libraries you're going to use open forever as long as they do ASLR.

Comment 8

[(not currently active) Ted Mielczarek]

The only case I'm worried about is if someone disables then re-enables crash reporting, we'll open (and leave open) another handle. I think I'm going to make it a global and release it in UnsetExceptionHandler just to avoid that.

Comment 9

[(not currently active) Ted Mielczarek]

Attachment: Add memory mapping info to Windows minidumps.

Ok, finished patch. This one actually does free the module handle in UnsetExceptionHandler.

Comment 10

[(not currently active) Ted Mielczarek]

I pushed to try, and this patch was green on Win2k3.

Comment 11

[(not currently active) Ted Mielczarek]

Pushed to m-c again:
http://hg.mozilla.org/mozilla-central/rev/cc4c72425f0e

Comment 12

[(not currently active) Ted Mielczarek]

Ugh, I think I read the documentation somewhat too literally. It says "DbgHelp 6.1 and earlier:  This value is not supported."

However, on Windows 7, the DbgHelp.dll is version 6.1.7600.16385, and it does support this, but as written my code will not use it on that version. :-/

Comment 13

[(not currently active) Ted Mielczarek]

Actually, this patch doesn't work at all. *sigh*

Turns out that version API DbgHelp exposes? It always returns the same version, "4.0.5.0". It's some outdated API, although conveniently the docs don't mention that.

Comment 14

[(not currently active) Ted Mielczarek]

Attachment: add memory mapping info to Windows minidumps when we have a new enough dbghelp.dll

This actually works. I tested with stock Windows 7 dbghelp.dll, and a copy from Windows 2003, and got minidumps in both cases. Loading the minidumps in WinDBG showed that the one from the stock Win7 dll had a memory info list, and the Win2k3 one did not.

Comment 15

[(not currently active) Ted Mielczarek]

Comment on attachment 502847 [details] [diff] [review]
add memory mapping info to Windows minidumps when we have a new enough dbghelp.dll

I would like to unit test this, but that means I need to write some C++ code to parse the minidump, which is pretty painful. I'll see what I can do.

Comment 16

[(not currently active) Ted Mielczarek]

Attachment: Add Breakpad processor code to binary test component to allow testing minidump contents

I wrote some tests. While I was at it, I wound up refactoring the existing tests a bit to stop using an xpcom component and switch to js-ctypes, since it's just simpler in the end.

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 503228 [details] [diff] [review]
Add Breakpad processor code to binary test component to allow testing minidump contents

This also relies on a Breakpad patch that I landed upstream with r=mento to build on Windows:
http://code.google.com/p/google-breakpad/source/detail?r=751

Comment 18

[(not currently active) Ted Mielczarek]

Attachment: Add Breakpad processor code to binary test component to allow testing minidump contents

Same patch, just rebased on top of bsmedberg's patch from bug 624835.

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 502847 [details] [diff] [review]
add memory mapping info to Windows minidumps when we have a new enough dbghelp.dll

I think timeless is travelling, and I'd like to get this landed. This (+ the tests) was totally green on the try server builds + xpcshell tests.

Comment 20

[(not currently active) Ted Mielczarek]

Comment on attachment 501960 [details] [diff] [review]
Add memory mapping info to Windows minidumps.

I'm going to back this patch out when I land the new one, since it doesn't actually fix the problem.

Comment 21

[(not currently active) Ted Mielczarek]

Pushed to m-c (third time's the charm):
http://hg.mozilla.org/mozilla-central/rev/f725e0b7787b - backout of previous non-working patch
http://hg.mozilla.org/mozilla-central/rev/f932163ae6a0 - new patch
http://hg.mozilla.org/mozilla-central/rev/247da71f616e - breakpad patch mentioned in comment 17
http://hg.mozilla.org/mozilla-central/rev/bd51bbe88214 - tests

Comment 22

[timeless]

ted the indentation here is off:

+    if (GetFileVersionInfoW(L"dbghelp.dll",
+                           0,

Comment 23

[(not currently active) Ted Mielczarek]

Fixed whitespace:
http://hg.mozilla.org/mozilla-central/rev/a39147a05a1e