CPU microcode detection is not working with AMD CPUs

VERIFIED FIXED in Firefox 52

Status

()

defect
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: marco, Assigned: marco)

Tracking

Trunk
mozilla53
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

OS: Windows 7 x64 SP1 + all updates
Firefox: 53.0a1 - Nightly (2016-11-26) - tested Win32 and Win64 versions


I get this error in JS console:

> 12:55:05.663 Error getting CPU revision. Exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWindowsRegKey.readBinaryValue]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///C:/Program%20Files/Mozilla/Firefox/53.0a1/browser/features/aushelper@mozilla.org.xpi!/bootstrap.js :: startup :: line 67"  data: no] 1 bootstrap.js:89
>	startup jar:file:///C:/Program%20Files/Mozilla/Firefox/53.0a1/browser/features/aushelper@mozilla.org.xpi!/bootstrap.js:89:11
>	this.XPIProvider.callBootstrapMethod resource://gre/modules/addons/XPIProvider.jsm:4975:9
>	this.XPIProvider.startup resource://gre/modules/addons/XPIProvider.jsm:2880:13
>	callProvider resource://gre/modules/AddonManager.jsm:235:12
>	_startProvider resource://gre/modules/AddonManager.jsm:788:5
>	AddonManagerInternal.startup resource://gre/modules/AddonManager.jsm:974:9
>	this.AddonManagerPrivate.startup resource://gre/modules/AddonManager.jsm:3026:5
>	amManager.prototype.observe resource://gre/components/addonManager.js:65:9


The offending code seems to be this:

> In browser/features/aushelper@mozilla.org.xpi!/bootstrap.js
>
>  let microCodeVersions = [0xe, 0x11, 0x12, 0x13, 0x16, 0x18, 0x19];
>  let cpuRevMatch = null;
>  try {
>    let keyNames = ["Update Revision", "Update Signature"];
>    for (let i = 0; i < keyNames.length; ++i) {
>      try {
>        let regVal = wrk.readBinaryValue(keyNames[i]);

The problem is that there are no key names "Update Revision" or "Update Signature" in the registry ( HKEY_LOCAL_MACHINE\HARDWARE\DESCRIPTION\System\CentralProcessor\0 ) for my AMD CPU.  Are these specific to Intel CPUs?  Should this have a conditional or something?  Check that is it an Intel CPU before looking for Intel specific registry entries.  If there are other AMD CPUs that do have an "Update (Revision|Signature)", then check that we are on one of those specific CPUs before looking deeper?

The available registry entries are:

~MHz (REG_DWORD, CPU MHz )
Component Information (REG_BINARY, 16 bytes)
Configuration Data (REG_FULL_RESOURCE_DESCRIPTOR, 16 bytes)
CurrentPatchLevel (REG_DWORD)
FeatureSet (REG_DWORD)
Identifier (REG_SZ, AMD64 Family 21 Model 2 Stepping 0)
MicrocodeUpdateStatus (REG_SZ, Newer Patch Not Available)
PreferredPatchLevel (REG_DWORD)
PreviousPatchLevel (REG_DWORD, same as PreferredPatchLevel)
ProcessorNameString (REG_SZ, "AMD FX(tm)-8350 Eight-Core Processor           ")
VendorIdentifier (REG_SZ)
This was actually in 49.0 and 50.0.  The Mercurial repo says bootstrap.js has a 2016-10-08 creation date, so I am assuming somewhere in the 49.0a1 nightlies.

The bug seems to do nothing but print an annoying message on AMD systems.  Cosmetic?

Looking at the code again, it seems to check for a bunch of intel-specific registry and other values, and sets each of three flags to true, false, or null, and then checks them all at the end, after the catch block generates the caught error message noise.

But perhaps to make this quiet, the checks need to be done incrementally, after each flag is set.  Thus, after the cpuVendorIDMatch === false, we KNOW that BOTH cpuIDMatch AND cpuRevMatch WILL also === false, because they CANNOT be true for non-Intel CPUs, and we should not even test, because we know they WILL fail the try and trigger the catch and be set to === null, and trigger the error message erroneously.  Likewise, if cpuVendorIDMatch === true, but cpuIDMatch === false, then we KNOW the stepping will be irrelevant (=== false), as each family has their own stepping sequence.

Thus, there should be no need for the redundant checking "if-matrix" done at the end.

But anyways, the absolute minimal change possible would simply be an if-else around the 3rd try.

>  let microCodeVersions = [0xe, 0x11, 0x12, 0x13, 0x16, 0x18, 0x19];
>  let cpuRevMatch = null;
>  if ( ! cpuVendorIDMatch ) {
>    cpuRevMatch = false;
>  } else {
>    try {
>      let keyNames = ["Update Revision", "Update Signature"];
>      // ...
>    } catch // ... {
>  }
Posted patch patch (obsolete) — Splinter Review
This is just cosmetic. If any of the entries read doesn't match it sets the url to false so removing the reportError is all that needs to be done.
Attachment #8814741 - Flags: review?(mcastelluccio)
Component: Breakpad Integration → General
Product: Toolkit → Firefox
Attachment #8814741 - Flags: review?(mcastelluccio) → review?(felipc)
Attachment #8814741 - Flags: review?(felipc) → review+
Actually, I opened this bug to add the CPU microcode detection for AMD too.
Summary: CPU microcode detection is not working with AMD CPUs → CPU microcode detection reports error with AMD CPUs
Hmmm... I think I moved to fast with this in that Marco is reporting an error with breakpad and the additional comments are for the aushelper. I'll move this back to breakpad and file a new bug for the error message.
Summary: CPU microcode detection reports error with AMD CPUs → CPU microcode detection is not working with AMD CPUs
Component: General → Breakpad Integration
Product: Firefox → Toolkit
Removing dependency on bug 1311515 which is what made me think this had to do with the aushelper which was specifically requested to only detect GenuineIntel and not to include AMD. If AMD needs to be included in the aushelper a new bug along with the checks that need to be performed should be filed under Firefox -> General for Firefox's aushelper extension.
No longer blocks: 1311515
I think I know what's wrong. I'll write a patch.
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
Posted patch WIP patch (obsolete) — Splinter Review
This is my WIP patch, still untested.
hi marco, i've tested your second try build from https://treeherder.mozilla.org/#/jobs?repo=try&revision=36e3ddef50ebd2ac98cd86e351b7494d1052cc3c and it does seem to generate a CPUMicrocodeVersion field in a report from an AMD cpu now:
https://crash-stats.mozilla.com/report/index/2476a7cd-1b3b-44ca-8e36-18e982161130#tab-metadata
Posted patch PatchSplinter Review
Could any of you review this patch?

It's the same as the WIP one, just added a new possible value name ("CurrentPatchLevel") that appears to be used for some AMD CPUs.
Attachment #8815299 - Attachment is obsolete: true
Attachment #8815772 - Flags: review?(ted)
Attachment #8815772 - Flags: review?(milan)
Comment on attachment 8815772 [details] [diff] [review]
Patch

Review of attachment 8815772 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me - I'm wondering if there is value in recording which registry entry we ended up with, or if all of the numbers can be used the same way, so we don't really need it.  I guess we start with this, and if it turns out we need more, we can add it.
Attachment #8815772 - Flags: review?(milan) → review+
Comment on attachment 8815772 [details] [diff] [review]
Patch

Thanks Milan, that was a quick review!

I've pushed the patch to try, just to be sure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b5f03f461ab3e4caaf1e766ff7fa734afb42740.
Attachment #8815772 - Flags: review?(ted)
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb0dcd3dc893
Support reading microcode version from the registry when it is a REG_DWORD value (sometimes used for AMD CPUs). r=milan
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/46f6ea9600f0
Add "CurrentPatchLevel" to the list of possible values where the microcode version is stored. r=milan
https://hg.mozilla.org/mozilla-central/rev/bb0dcd3dc893
https://hg.mozilla.org/mozilla-central/rev/46f6ea9600f0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Marco, is that something that you were planning to uplift to 52?
Flags: needinfo?(mcastelluccio)
It's a simple patch and it would give us more information when making decisions. I guess we can safely uplift to 52.
I'll create an uplift request.
Flags: needinfo?(mcastelluccio)
Comment on attachment 8815772 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Not a regression.
[User impact if declined]: We won't have detailed information about CPU microcode for AMD CPUs in Socorro.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: I have verified that on Socorro we now have the cpu_microcode_version value also for AMD CPUs. IIRC philipp verified the patch on his AMD machine. So, I suppose additional verification isn't really needed.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's a simple change that is only adding a new possible Windows registry key (and type) to read the CPU microcode version.
[String changes made/needed]: None.
Attachment #8815772 - Flags: approval-mozilla-beta?
Comment on attachment 8815772 [details] [diff] [review]
Patch

get cpu microcode revision in crash reports for more cpus, beta52+ (both patches that landed on m-c)
Attachment #8815772 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.