Closed Bug 620974 Opened 10 years ago Closed 10 years ago

Add memory mapping info to minidumps

Categories

(Toolkit :: Crash Reporting, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b10

People

(Reporter: ted, Assigned: ted)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
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.
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.
Attachment #499531 - Flags: review?(timeless)
Attachment #499531 - Flags: review?(timeless) → review+
Attachment #499531 - Flags: approval2.0?
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.
Attachment #499531 - Flags: approval2.0? → approval2.0+
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/f7a6e3530d99
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 623462
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.
Attachment #499531 - Attachment is obsolete: true
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.
Attachment #501758 - Flags: review+
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.
Attachment #501758 - Attachment is obsolete: true
Ok, finished patch. This one actually does free the module handle in UnsetExceptionHandler.
Status: REOPENED → ASSIGNED
Attachment #501960 - Flags: review?(timeless)
I pushed to try, and this patch was green on Win2k3.
Attachment #501960 - Flags: review?(timeless) → review+
Attachment #501960 - Flags: approval2.0+
Pushed to m-c again:
http://hg.mozilla.org/mozilla-central/rev/cc4c72425f0e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b9
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. :-/
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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 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.
Attachment #502847 - Flags: review?(timeless)
Status: REOPENED → ASSIGNED
Target Milestone: mozilla2.0b9 → ---
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 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
Attachment #503228 - Flags: review?(benjamin)
Attachment #503228 - Attachment is obsolete: true
Attachment #503228 - Flags: review?(benjamin)
Same patch, just rebased on top of bsmedberg's patch from bug 624835.
Attachment #503462 - Flags: review?(benjamin)
Blocks: 583591
Attachment #503462 - Flags: review?(benjamin) → review+
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.
Attachment #502847 - Flags: review?(timeless) → review?(benjamin)
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.
Attachment #501960 - Attachment is obsolete: true
Attachment #502847 - Flags: review?(benjamin) → review+
Attachment #502847 - Flags: approval2.0+
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
Depends on: 626169
ted the indentation here is off:

+    if (GetFileVersionInfoW(L"dbghelp.dll",
+                           0,
You need to log in before you can comment on or make changes to this bug.