Put AdapterSubsysID and AdapterDriverVersion into separate crash annotations

VERIFIED FIXED in Firefox 32

Status

()

VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: kairo, Assigned: milan)

Tracking

(Blocks: 1 bug)

26 Branch
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox31 wontfix, firefox32 verified, firefox33 verified)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
If you look at e.g. bp-ca89729a-19f7-4d66-a548-9b1712130918 you'll find info like this in the "App Notes" field:

AdapterVendorID: 0x1002, AdapterDeviceID: 0x9588, AdapterSubsysID: 09901462, AdapterDriverVersion: 8.970.100.7000


The AdapterVendorID and AdapterDeviceID are also available in separate annotations, but AdapterSubsysID and AdapterDriverVersion aren't, and if they were, we could make better analysis with the features that Socorro is adding like the API and search for all kinds of fields.
(Reporter)

Updated

5 years ago
OS: Linux → All
Hardware: x86_64 → All
Milan, this feature would help a lot for bug 768395. It would be nice if you could add to the TODO list of your team. Thanks
Flags: needinfo?(milan)
How does one test this, outside of crashing?
(Reporter)

Comment 3

4 years ago
(In reply to Milan Sreckovic [:milan] from comment #2)
> How does one test this, outside of crashing?

It's info in crash reports, you can only test it in a crash report, AFAIK (not sure if bsmedberg would know some other way). We have the add-on at https://code.google.com/p/crashme/ to infuse crash artificially, though.
Flags: needinfo?(milan)
See Also: → bug 918389
Flags: needinfo?(milan)
Created attachment 8440107 [details] [diff] [review]
WIP: Windows version.

KaiRo was mentioning in IRC that some other pieces need to be put in place on the server side; can somebody try this patch (Windows only) and let me know if the data is coming through, and if this is the right direction? (I do see it in the .extra file.)
Attachment #8440107 - Flags: feedback?(sledru)
Attachment #8440107 - Flags: feedback?(kairo)
Flags: needinfo?(milan)
Comment on attachment 8440107 [details] [diff] [review]
WIP: Windows version.

I will leave Kairo answer here. I don't have an opinion on this.
Attachment #8440107 - Flags: feedback?(sledru)
(Reporter)

Comment 6

4 years ago
Comment on attachment 8440107 [details] [diff] [review]
WIP: Windows version.

I don't know anything I can do that is better than to see if it's in the .extra file.
We will need to file an additional bug in Socorro once this landed to add the "AdapterDriverVersion" and "AdapterSubsysID" to the whitelist and to search, so we expose them in crash-stats UI and can search for them in "Super Search".

Benjamin, is there any other testing/checking than the .extra file we can/should do before this lands?
Attachment #8440107 - Flags: feedback?(kairo) → feedback?(benjamin)

Comment 7

4 years ago
I don't think automated testing is necessary for this, and would be a pain because we usually don't have a graphics subsystem in xpcshell tests, which is where most of these annotation tests currently live.

Updated

4 years ago
Attachment #8440107 - Flags: feedback?(benjamin) → feedback+
Created attachment 8445321 [details] [diff] [review]
Expose adapter subsys to webidl, make it available together with driver version as crash annotations.

Subsys on windows only, extra annotation on OS X.
Attachment #8445321 - Flags: review?(vladimir)
Comment on attachment 8445321 [details] [diff] [review]
Expose adapter subsys to webidl, make it available together with driver version as crash annotations.

Looks good, but add it to about:support as well?
Attachment #8445321 - Flags: review?(vladimir) → review+
Created attachment 8447365 [details] [diff] [review]
Expose adapter subsys to webidl, show it in about:support, make it available together with driver version as crash annotations.

Not sure if more things need to be done because this has L10N implications?
Attachment #8440107 - Attachment is obsolete: true
Attachment #8445321 - Attachment is obsolete: true
Attachment #8447365 - Flags: review?(vladimir)
(Reporter)

Comment 11

4 years ago
Comment on attachment 8447365 [details] [diff] [review]
Expose adapter subsys to webidl, show it in about:support, make it available together with driver version as crash annotations.

Well, the L10n changes can't land on Aurora and Beta, but for trunk, there are no issues with that.
It may make sense to (once this is reviewed) do a patch excluding the about:support changes and request uplift of that to aurora and beta, so we can have the crash annotations ASAP (and the about:support stuff will ride the trains).
Attachment #8445321 - Attachment is obsolete: false
Comment on attachment 8447365 [details] [diff] [review]
Expose adapter subsys to webidl, show it in about:support, make it available together with driver version as crash annotations.

Let me split the patch into the one that Vlad already reviewed, adds annotations, and the follow up for about:support.
Attachment #8447365 - Attachment is obsolete: true
Attachment #8447365 - Flags: review?(vladimir)
I'll move the "about:support" part to another bug to avoid confusion about alpha+beta, etc.
Created attachment 8448156 [details] [diff] [review]
Expose adapter subsys to webidl, show it in about:support, make it available together with driver version as crash annotations.

Minor formatting change with subsys id saved as string, but carry r=vlad.
Attachment #8448156 - Flags: review+
Attachment #8445321 - Attachment is obsolete: true
Keywords: checkin-needed
Hi Milan,
could you provide a Try link. Suggestions for what to run if you haven't
yet can be found here:
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f1c312a85ee
Assignee: nobody → milan
Keywords: checkin-needed
Backed out for crashes galore and other test failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/05e8235c3e20

https://tbpl.mozilla.org/php/getParsedLog.php?id=43055180&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=43054627&tree=Mozilla-Inbound

etc...

Please ensure that your Try pushes contain sufficient test coverage to catch potential issues that crop up.
Thanks for backing it out.  It isn't as bad as the run above shows (e.g., https://tbpl.mozilla.org/?tree=Try&rev=56867ebc1bbe), but there a failure in there that's real in B2G ICS Emulator Debug.
https://tbpl.mozilla.org/?tree=Try&rev=56867ebc1bbe and https://tbpl.mozilla.org/?tree=Try&rev=e4890a0af5dc seem to suggest that it was a temporary blip, they had the same patch as the catastrophically bad attempt above.  Does it make sense to try it again?
Keywords: checkin-needed
backed out again for breaking tests like https://tbpl.mozilla.org/php/getParsedLog.php?id=43425974&tree=Mozilla-Inbound 

maybe this need to be checked-in together with a clobber ?
Created attachment 8453208 [details] [diff] [review]
Expose adapter subsys to webidl, show it in about:support, make it available together with driver version as crash annotations.  Carry r=vvukicevic

And don't forget to update the UUID.
Attachment #8448156 - Attachment is obsolete: true
Attachment #8453208 - Flags: review+
One more try, with :ryanvm's pointer that we need to update UUID.  :kairo, if this goes through, we can see if we can uplift to Aurora.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0519b4b09abb
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Created attachment 8453803 [details] [diff] [review]
Aurora patch.  Expose adapter subsys to webidl, show it in about:support, make it available together with driver version as crash annotations.


[Feature/regressing bug #]: Part of meta bug 918102
[User impact if declined]: Improved Socorro experience waits until this rides out the trains.
[Describe test coverage new/current, TBPL]: No automated test coverage (see comment 7)
[Risks and why]: 
[String/UUID change made/needed]: UUID change to nsIGfxInfo in widget/nsIGfxInfo.idl

:kairo, anything to add on the user impact?
Attachment #8453803 - Flags: review+
Attachment #8453803 - Flags: approval-mozilla-aurora?
Flags: needinfo?(kairo)
status-firefox31: --- → wontfix
status-firefox32: --- → affected
status-firefox33: --- → fixed
Attachment #8453803 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Reporter)

Comment 28

4 years ago
(In reply to Milan Sreckovic [:milan] from comment #27)
> :kairo, anything to add on the user impact?

There is no direct user impact but we need this to analyze crashes better (esp. to determine which driver versions any crashes happen with).
Flags: needinfo?(kairo)
(Reporter)

Updated

4 years ago
Blocks: 1039461
(Reporter)

Comment 30

4 years ago
Here's a report from Aurora 32 that has those fields set: bp-c5983e09-5937-4a97-ae15-dd9652140712
And here's one in Nightly 33: bp-37ecefce-5ab4-4162-b5d5-0b3252140711

Thanks, seems to all work fine! :)


(I filed bug 1039461 to actually expose the fields in crsh-stats, but they are there in the metadata)
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+]
status-firefox32: fixed → verified
status-firefox33: fixed → verified
You need to log in before you can comment on or make changes to this bug.