Annotate system manufacturer in crash reports

RESOLVED FIXED in mozilla27

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: dmajor, Assigned: dmajor)

Tracking

unspecified
mozilla27
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 814479 [details] [diff] [review]
Proof of concept

Prototype code based on MSDN sample.
(Assignee)

Comment 2

5 years ago
Created attachment 814531 [details] [diff] [review]
Proof of concept

Prototype code based on MSDN sample.
Removed trailing whitespace from previous patch
Attachment #814479 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
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

5 years ago
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

5 years ago
> @@ +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

5 years ago
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.
(Assignee)

Comment 7

5 years ago
(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.
(Assignee)

Comment 8

5 years ago
(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

5 years ago
(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

5 years ago
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.
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).
(Assignee)

Comment 12

5 years ago
Created attachment 818756 [details] [diff] [review]
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 :(
Attachment #814531 - Attachment is obsolete: true
Attachment #818756 - Flags: feedback?(tabraldes)
Attachment #818756 - Flags: feedback?(jmathies)

Comment 13

5 years ago
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.
Attachment #818756 - Flags: feedback?(jmathies) → feedback+

Comment 14

5 years ago
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 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
Attachment #818756 - Flags: feedback?(tabraldes) → feedback+
(Assignee)

Comment 16

5 years ago
(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

5 years ago
(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.
(Assignee)

Comment 18

5 years ago
Created attachment 822457 [details] [diff] [review]
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.
Attachment #818756 - Attachment is obsolete: true
Attachment #822457 - Flags: review?(jmathies)

Comment 19

5 years ago
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.
Attachment #822457 - Flags: review?(jmathies) → review+
(Assignee)

Comment 20

5 years ago
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/875fb3abaa10
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

Comment 23

5 years ago
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).
(Assignee)

Updated

5 years ago
Blocks: 932060

Updated

4 years ago
Depends on: 1070123
You need to log in before you can comment on or make changes to this bug.