921609 - Annotate system manufacturer in crash reports

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[(Away)]

AMD has requested a breakdown of bug 772330 crashes by OEM.

On Windows this information is available through WMI. I propose logging these WMI values:
Win32_BIOS.Manufacturer
Win32_ComputerSystemProduct.Vendor
Win32_ComputerSystemProduct.Name

The BIOS manufacturer is probably the most relevant one for the AMD investigation, but as long as we are calling those APIs, the make and model of the machine might be useful to have.

I have some proof of concept code but it needs cleanup.

Comment 1

[(Away)]

Attachment: Proof of concept

Prototype code based on MSDN sample.

Comment 2

[(Away)]

Attachment: Proof of concept

Prototype code based on MSDN sample.
Removed trailing whitespace from previous patch

Comment 3

[(Away)]

Comment on attachment 814531 [details] [diff] [review]
Proof of concept

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

::: toolkit/xre/nsAppRunner.cpp
@@ +241,5 @@
>  #include "BinaryPath.h"
>  
> +#include <comdef.h>
> +#include <Wbemidl.h>
> +#pragma comment(lib, "wbemuuid.lib")

Is this the right way to pull in a system lib? Or do I need to integrate with the Mozilla build system? (And yeah I need some platform ifdefs)

@@ +2812,5 @@
> +int WMI_ProofOfConcept()
> +{
> +    HRESULT hres;
> +
> +    hres =  CoInitializeEx(0, COINIT_MULTITHREADED);

It doesn't feel right for this dinky piece of code to set the CoInit settings for the main thread. Should I be doing this in some other thread? By the way, I checked, and we haven't already CoInitialized at this point.

@@ +2830,5 @@
> +        IID_IWbemLocator, (LPVOID *) &pLoc);
> +
> +    if (FAILED(hres))
> +    {
> +        CoUninitialize();

This error handling gets more and more clunky as the function progresses. Are there any RAII helpers that could simplify?

@@ +2841,5 @@
> +    // current user and obtain pointer pSvc
> +    // to make IWbemServices calls.
> +
> +    hres = pLoc->ConnectServer(
> +        _bstr_t(L"ROOT\\CIMV2"), // WMI namespace

Is _bstr_t considered hax? Back in my day, we had to SysAllocString for 15 miles in the snow.

@@ +2860,5 @@
> +    }
> +
> +    // Set the IWbemServices proxy so that impersonation
> +    // of the user (client) occurs.
> +    hres = CoSetProxyBlanket(

Cargo cult alert. I'm not familiar with this function, other than that the rest of the code fails without it. Am I using it correctly?

@@ +2886,5 @@
> +
> +    // For example, query for all the running processes
> +    IEnumWbemClassObject* pEnumerator = NULL;
> +    hres = pSvc->ExecQuery(
> +        bstr_t("WQL"),

Wait, what? Both _bstr_t and bstr_t? Thanks, MSDN.

@@ +3110,5 @@
>           CrashReporter::SetExceptionHandler(mAppData->xreDirectory))) {
>      if (mAppData->crashReporterURL)
>        CrashReporter::SetServerURL(nsDependentCString(mAppData->crashReporterURL));
>  
> +    WMI_ProofOfConcept();

Assuming we get the code cleaned up, is this the right place to call it?

Comment 4

[Jim Mathies [:jimm]]

Comment on attachment 814531 [details] [diff] [review]
Proof of concept

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

Note if you've pulled this out of sample code you should wash it. Microsoft's sample code licensing isn't compat with our own.

::: toolkit/xre/nsAppRunner.cpp
@@ +241,5 @@
>  #include "BinaryPath.h"
>  
> +#include <comdef.h>
> +#include <Wbemidl.h>
> +#pragma comment(lib, "wbemuuid.lib")

No it's not, you can add it to the libraries we link xul.dll with down in toolkit/library.

@@ +2812,5 @@
> +int WMI_ProofOfConcept()
> +{
> +    HRESULT hres;
> +
> +    hres =  CoInitializeEx(0, COINIT_MULTITHREADED);

If this is called on the main thread it'll fail since we've already initialized as STA.

@@ +2813,5 @@
> +{
> +    HRESULT hres;
> +
> +    hres =  CoInitializeEx(0, COINIT_MULTITHREADED);
> +    if (FAILED(hres))

nit -

if (x) {
}

Also you might want to use our assertion / return macros that check results.

@@ +2820,5 @@
> +    }
> +
> +    // Obtain the initial locator to Windows Management
> +    // on a particular host computer.
> +    IWbemLocator *pLoc = 0;

wrap these in a nsRefPtr<>'s, so you don't have to release them manually.

@@ +2841,5 @@
> +    // current user and obtain pointer pSvc
> +    // to make IWbemServices calls.
> +
> +    hres = pLoc->ConnectServer(
> +        _bstr_t(L"ROOT\\CIMV2"), // WMI namespace

I think using these classes is ok as long as it builds / links properly. They have automatic  dealocation which is nice.

@@ +2860,5 @@
> +    }
> +
> +    // Set the IWbemServices proxy so that impersonation
> +    // of the user (client) occurs.
> +    hres = CoSetProxyBlanket(

If you don't know what it does, might I suggest you figure it out before using it. :)

Comment 5

[Jim Mathies [:jimm]]

> @@ +2812,5 @@
> > +int WMI_ProofOfConcept()
> > +{
> > +    HRESULT hres;
> > +
> > +    hres =  CoInitializeEx(0, COINIT_MULTITHREADED);
> 
> If this is called on the main thread it'll fail since we've already
> initialized as STA.

Also if by some chance you call this before everyone else, you're going to break a lot of stuff. I don't think that's going to happen though. For a long time I've been wanting to centralize our com init/uninit calls but have never gotten around to it. :/ I think the first real call we make to init com happens down in some random file picker init code.
 
> @@ +2860,5 @@
> > +    }
> > +
> > +    // Set the IWbemServices proxy so that impersonation
> > +    // of the user (client) occurs.
> > +    hres = CoSetProxyBlanket(
> 
> If you don't know what it does, might I suggest you figure it out before
> using it. :)

Comment 6

[Jim Mathies [:jimm]]

Any sense of what the overhead is for this? We might want to delay it if it takes "a lot of time". You could even do it on a delayed, separate thread if need be. If there's no overhead, no big deal.

Comment 7

[(Away)]

(In reply to Jim Mathies [:jimm] from comment #5)
> I think the first real call we make to init com happens down in some random file picker init code.

You're right. Heh, and I was really close, too! Had I called WMI_ProofOfConcept only a few lines later, COM would have been initialized by that file code.

Comment 8

[(Away)]

(In reply to David Major [:dmajor] from comment #7)
> Heh, and I was really close, too! 

...actually, no, I was looking at some transient initializations from shell32. The first long-term CoInitialize comes from some ShortcutResolver stuff later on. I guess I'll have wait until we've passed that point before I do my calls.

Comment 9

[Jim Mathies [:jimm]]

(In reply to David Major [:dmajor] from comment #8)
> (In reply to David Major [:dmajor] from comment #7)
> > Heh, and I was really close, too! 
> 
> ...actually, no, I was looking at some transient initializations from
> shell32. The first long-term CoInitialize comes from some ShortcutResolver
> stuff later on. I guess I'll have wait until we've passed that point before
> I do my calls.

Nothing wrong with you doing the init here if it works right. I think there is quite a bit of added overhead when you initialize COM as MTA. Is that needed? Also initializing as MTA means a lot of our paired coinit/counint will be mismatched due to failed coinit calls that don't get tracked. This needs to be avoided.

http://mxr.mozilla.org/mozilla-central/search?string=CoInitialize
http://mxr.mozilla.org/mozilla-central/search?string=CoInitializeEx

You could also do this in a separate thread.

Comment 10

[Jim Mathies [:jimm]]

One other thought, since this is associated with crash reporting, maybe move the code into that module? There might be an mta thread there you can use.

Comment 11

[(not currently active) Ted Mielczarek]

The crash reporter only has one other thread, and it's not setup to let you run random code on it (it's the Breakpad thread that listens for events from the crashing thread).

Comment 12

[(Away)]

Attachment: Attempt 2

Here's another attempt. Restyled. Moved to an already-CoInit point in time.

Some quick-and-dirty QPC measurement shows around 17ms. That's not exactly free :(

Comment 13

[Jim Mathies [:jimm]]

Comment on attachment 818756 [details] [diff] [review]
Attempt 2

Looks great. I'd suggest a try talos push to see how the numbers look. If the perf hit shows up you should look at moving this to delayed startup somehow. 

If we do that startup crashes won't get this info, which sucks. An option there might be to annotate on startup for nightlies only and delay for everything else.  Others cc'd in here might have some different ideas.

Comment 14

[Benjamin Smedberg]

I don't particular care if very early startup crashes don't get this info, if it costs in terms of perf. Most of our startup crashes aren't that early in startup anyway. Doing this from a custom thread sounds pretty reasonable to me.

Comment 15

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 818756 [details] [diff] [review]
Attempt 2

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

I didn't comment on this because it looked good and jimm had already provided the f+. However, that didn't make it disappear from my review queue so... :)

::: toolkit/xre/nsAppRunner.cpp
@@ +3218,5 @@
> +    HRESULT hr = CoCreateInstance(CLSID_WbemLocator, nullptr, CLSCTX_INPROC_SERVER,
> +                                  IID_IWbemLocator, getter_AddRefs(locator));
> +
> +    if (FAILED(hr))
> +      return;

nit: I prefer all if blocks to have braces

Comment 16

[(Away)]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #15)
> nit: I prefer all if blocks to have braces

I do too! :) I thought the style guide insisted on not bracing single lines, but on second look I'm not seeing that in there.

Given the perf cost, I'm going to try moving this work to its own thread.

Comment 17

[Jim Mathies [:jimm]]

(In reply to David Major [:dmajor] from comment #16)
> (In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #15)
> > nit: I prefer all if blocks to have braces
> 
> I do too! :) I thought the style guide insisted on not bracing single lines,
> but on second look I'm not seeing that in there.
> 
> Given the perf cost, I'm going to try moving this work to its own thread.

Over the last couple of years reviewers have been enforcing that more. I try to too although I admit sometimes I forget / don't do it in my own code.

Comment 18

[(Away)]

Attachment: Attempt 3 - Moved to its own thread

I think this is ready for real review. I moved it off the main thread. Stepped through to make sure that the crash reporter doesn't mind.

Comment 19

[Jim Mathies [:jimm]]

Comment on attachment 822457 [details] [diff] [review]
Attempt 3 - Moved to its own thread

Looks like AnnotateCrashReport is thread safe, and the code here looks good. My only concern is that we're doing this right at startup without a delay so we might take a hit from that. I'd suggest throwing this at try for a talos run to look for regressions. If nothing shows up I think your good to go.

Comment 20

[(Away)]

I sent this through Try last night, not just for perf but also to make sure I didn't mess up on the platform ifdefs. As far as I can tell the Talos numbers seem okay. https://tbpl.mozilla.org/?tree=Try&rev=aa2253d01cc4

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/875fb3abaa10

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/875fb3abaa10

Comment 23

[Robert Kaiser]

You should probably file a followup bug (I guess in Socorro :: Webapp) to whitelist any added annotations in Socorro, which will make them available in UI (at least in the places where we already use the whitelist - will be used more and more in the future).