1280577 - Fix breakpad to cope with minidumps generated in Windows 10

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

Technically not a gecko bug but still needs to be tracked to ensure we can properly analyze stacks on Win32/64. The minidump file format was changed in Windows 10 and extra fields were added to the MiscInfo structure. This causes breakpad's internal checks to fail when processing such file as the structure appears to be of an unexpected size. I'll file (and fix) a bug in breakpad upstream and close this one once we've pulled in the fixes.

Comment 1

[(not currently active) Ted Mielczarek]

This is still broken upstream? I'm pretty sure I've looked at Windows 10 minidumps with Breakpad.

Comment 2

[Benjamin Smedberg]

All my crashes on win10 appear to be processed correctly on crash-stats (win64).

Comment 3

[(not currently active) Ted Mielczarek]

Presumably this happens if you have a newer DbgHelp.dll or something? The Breakpad code here is kind of silly:
https://chromium.googlesource.com/breakpad/breakpad/+/240ed57ee1ac6a87b91526b8331717d494801826/src/processor/minidump.cc#3492

It should allow anything there that's >= MD_MISCINFO_SIZE. That patch should be easy to upstream.

Comment 4

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3)
> Presumably this happens if you have a newer DbgHelp.dll or something? The
> Breakpad code here is kind of silly:
> https://chromium.googlesource.com/breakpad/breakpad/+/
> 240ed57ee1ac6a87b91526b8331717d494801826/src/processor/minidump.cc#3492
> 
> It should allow anything there that's >= MD_MISCINFO_SIZE. That patch should
> be easy to upstream.

Yeah, that's what's causing it to fail because they've added a fifth part to the misc info structure and it's another 532 bytes in size. I could just add a placeholder MD_MISCINFO05_* structure with defines and all without populating it so that it would at least accept the files and then we could implement proper reading of the fields at a later time. BTW rust-minidump runs into the same problem; I've discovered it there first.

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

Version 5 of the _MINIDUMP_MISC_INFO structure in minidumpapiset.h contains two additional fields:

XSTATE_CONFIG_FEATURE_MSC_INFO XStateData;    
ULONG32 ProcessCookie;

In turn XSTATE_CONFIG_FEATURE_MSC_INFO is a structure made of the following fields (with MAXIMUM_XSTATE_FEATURES being defined to 64):

ULONG32 SizeOfInfo;
ULONG32 ContextSize;
ULONG64 EnabledFeatures; 	           
XSTATE_FEATURE Features[MAXIMUM_XSTATE_FEATURES];

Finally XSTATE_FEATURE is again a structure holding only two fields:

DWORD Offset;
DWORD Size;

Adding this bits to the equivalent breakpad structures plus the relevant machinery to load them should fix the issue.

Comment 6

[(not currently active) Ted Mielczarek]

Okay. I think it'd be sufficient to just handle anything >= MD_MISCINFO_SIZE, so it's future-proof. (I think you should make that change regardless, so we don't break the next time Microsoft does this.) Feel free to add the new structs as well, I don't know that we get much value out of them.

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> Okay. I think it'd be sufficient to just handle anything >=
> MD_MISCINFO_SIZE, so it's future-proof. (I think you should make that change
> regardless, so we don't break the next time Microsoft does this.)

+1, I'll do that!

Comment 8

[Gabriele Svelto [:gsvelto] (PTO)]

I've sent the patch that fixes this issue upstream, the relevant issue is this one: https://codereview.chromium.org/2109063004

Comment 9

[(not currently active) Ted Mielczarek]

I landed that patch upstream:
https://chromium.googlesource.com/breakpad/breakpad/+/c9f80bf1a8d8060c812e8cedbbd0129ec2d1fb21

Feel free to cherry-pick it into mozilla-central if we don't get the Breakpad update landed.

Comment 10

[Gabriele Svelto [:gsvelto] (PTO)]

The fix for this will land as part of bug 1264367.

Comment 11

[Gabriele Svelto [:gsvelto] (PTO)]

This is now in mozilla-central.