Fix breakpad to cope with minidumps generated in Windows 10

RESOLVED FIXED

Status

()

Toolkit
Crash Reporting
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: gsvelto, Assigned: gsvelto)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fce-active])

(Assignee)

Description

a year ago
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.
This is still broken upstream? I'm pretty sure I've looked at Windows 10 minidumps with Breakpad.

Comment 2

a year ago
All my crashes on win10 appear to be processed correctly on crash-stats (win64).
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.
(Assignee)

Comment 4

a year ago
(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.
(Assignee)

Comment 5

a year ago
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.
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.
(Assignee)

Comment 7

a year ago
(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!
(Assignee)

Comment 8

a year ago
I've sent the patch that fixes this issue upstream, the relevant issue is this one: https://codereview.chromium.org/2109063004
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
(Assignee)

Updated

a year ago
Blocks: 1280469
No longer depends on: 1280469
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.
(Assignee)

Comment 10

a year ago
The fix for this will land as part of bug 1264367.
Depends on: 1264367
(Assignee)

Comment 11

a year ago
This is now in mozilla-central.
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.