918386 - Put AdapterSubsysID and AdapterDriverVersion into separate crash annotations

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Robert Kaiser]

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.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

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

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

How does one test this, outside of crashing?

Comment 3

[Robert Kaiser]

(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.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: 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.)

Comment 5

[Sylvestre Ledru [:Sylvestre]]

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

I will leave Kairo answer here. I don't have an opinion on this.

Comment 6

[Robert Kaiser]

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?

Comment 7

[Benjamin Smedberg]

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.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Expose adapter subsys to webidl, make it available together with driver version as crash annotations.

Subsys on windows only, extra annotation on OS X.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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?

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: 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?

Comment 11

[Robert Kaiser]

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).

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

I'll move the "about:support" part to another bug to avoid confusion about alpha+beta, etc.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: 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.

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Right, forgot to put the link in the bug: https://tbpl.mozilla.org/?tree=Try&rev=e1b4d75c1c77

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f1c312a85ee

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

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?

Comment 21

[Carsten Book [:Tomcat]]

checked in as https://hg.mozilla.org/integration/mozilla-inbound/rev/9f4b2ff029e3

Comment 22

[Carsten Book [:Tomcat]]

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 ?

Comment 23

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: 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.

Comment 24

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 25

[Carsten Book [:Tomcat]]

relanded again as https://hg.mozilla.org/integration/mozilla-inbound/rev/0519b4b09abb

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0519b4b09abb

Comment 27

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: 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?

Comment 28

[Robert Kaiser]

(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).

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f6670cdc8b9a

Comment 30

[Robert Kaiser]

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)