Closed Bug 1305120 Opened 3 years ago Closed 3 years ago

Add a crash report annotation containing the microcode version of the CPU

Categories

(Toolkit :: Crash Reporting, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla52
Tracking Status
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: marco, Assigned: milan)

References

Details

Attachments

(2 files)

This might be useful in some cases, e.g. bug 1296630.
Would you just add it to the CPU description, or have a separate field?
Separate field maybe? So we have more flexibility when using SuperSearch.
The CPU description is generated by Breakpad from some structs in the minidump itself (which we can't change because they're defined by Microsoft), so I think it'd be better to put it in a separate field.
I was hoping we'd be able to extend Breakpad to show this extra information, but sounds like that is not an option.
Blocks: 1305888
Most of the code got reviewed in https://bugzilla.mozilla.org/attachment.cgi?id=8790490&action=edit, but I want to make sure this is the correct location for this kind of a functionality.
Assignee: nobody → milan
Comment on attachment 8795455 [details]
Bug 1305120: CPU microcode as a separate field in the crash report. .mielczarek

https://reviewboard.mozilla.org/r/81496/#review81400

::: toolkit/xre/nsAppRunner.cpp:3451
(Diff revision 2)
> +                           &len) == ERROR_SUCCESS &&
> +          vtype == REG_BINARY && len == sizeof(updateRevision)) {
> +
> +        // The first word is unused
> +        cpuUpdateRevision = static_cast<int>(updateRevision[1]);
> +      } else if (RegQueryValueExW(key, L"Update Revision",

Seems like you'd be better served writing this as a loop and breaking on the first successful read.

::: toolkit/xre/nsAppRunner.cpp:3463
(Diff revision 2)
> +        cpuUpdateRevision = static_cast<int>(updateRevision[1]);
> +      }
> +    }
> +
> +    if (cpuUpdateRevision > 0) {
> +      CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("Microcode"),

Can we give this a slightly more informative name, like maybe "CPU_Microcode_Revision"?
Attachment #8795455 - Flags: review?(ted) → review+
Milan, did you want to ask for review again or carry r+ and land the patch?
Flags: needinfo?(milan)
(In reply to Marco Castelluccio [:marco] from comment #10)
> Milan, did you want to ask for review again or carry r+ and land the patch?

Once it passes try, I will carry r+ and land it.
Flags: needinfo?(milan)
Pushed by msreckovic@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d382e432aa08
CPU microcode as a separate field in the crash report. r=ted.mielczarek
https://hg.mozilla.org/mozilla-central/rev/d382e432aa08
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1308385
Comment on attachment 8795455 [details]
Bug 1305120: CPU microcode as a separate field in the crash report. .mielczarek

Approval Request Comment
Need the extra information for bug 1296630.  Because we need the edit made in bug 1308385, I will attach a aurora/beta rolled up patch here.
Flags: needinfo?(milan)
Attachment #8795455 - Flags: approval-mozilla-beta?
Attachment #8795455 - Flags: approval-mozilla-aurora?
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #16)
> I'm a little worried that it's not showing up in the Metadata tab of any of
> today's crash reports:
> https://crash-stats.mozilla.com/search/
> ?build_id=20161007030207&release_channel=nightly&product=Firefox&platform=Win
> dows&date=%3E%3D2016-10-07T00%3A00%3A00.000Z&date=%3C2016-10-07T16%3A19%3A00.
> 000Z&_sort=-
> date&_facets=signature&_columns=date&_columns=signature&_columns=product&_col
> umns=version&_columns=build_id&_columns=platform&_columns=process_type#crash-
> reports
> 
> Should I be expecting it to be there?
> 
> 
> If this is expected until bug 1305888 happens, though, could you request
> uplift to aurora and beta for debugging of bug 1296630?

It is showing up for me, e.g. https://crash-stats.mozilla.com/report/index/2c4bc882-96b2-4456-95ea-61d8a2161007#tab-metadata
has CPUMicrocodeVersion: 0x1b.

Perhaps you need to be logged-in in order to see it.
Status: RESOLVED → VERIFIED
(In reply to Marco Castelluccio [:marco] from comment #19)
> Perhaps you need to be logged-in in order to see it.

Yes, that was the problem.
Comment on attachment 8795455 [details]
Bug 1305120: CPU microcode as a separate field in the crash report. .mielczarek

We need this to fix the Intel crashes, Aurora51+, Beta50+
Attachment #8795455 - Flags: approval-mozilla-beta?
Attachment #8795455 - Flags: approval-mozilla-beta+
Attachment #8795455 - Flags: approval-mozilla-aurora?
Attachment #8795455 - Flags: approval-mozilla-aurora+
Depends on: 1315213
See Also: → 1320921
You need to log in before you can comment on or make changes to this bug.