Closed Bug 918386 Opened 11 years ago Closed 10 years ago

Put AdapterSubsysID and AdapterDriverVersion into separate crash annotations

Categories

(Core :: Graphics, defect)

26 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox31 --- wontfix
firefox32 --- verified
firefox33 --- verified

People

(Reporter: kairo, Assigned: milan)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

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.
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?
(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: → 918389
Flags: needinfo?(milan)
Attached patch WIP: Windows version. (obsolete) — Splinter Review
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)
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)
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.
Attachment #8440107 - Flags: feedback?(benjamin) → feedback+
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+
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)
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.
Minor formatting change with subsys id saved as string, but carry r=vlad.
Attachment #8448156 - Flags: review+
Attachment #8445321 - Attachment is obsolete: true
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
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 ?
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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
[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)
Attachment #8453803 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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)
Blocks: 1039461
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+]
You need to log in before you can comment on or make changes to this bug.