Closed Bug 1384560 Opened 3 years ago Closed 3 years ago

Annotate crash reports with accessibility client information

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: jimm, Assigned: jimm)

Details

(Whiteboard: aes+)

Attachments

(1 file, 4 obsolete files)

We ship oop clients to telemetry here[1], and in-proc clients here [2]. We should add this information to crash report annotations (non public) so we don't have to keep asking users what it is they run.

[1] http://searchfox.org/mozilla-central/source/accessible/windows/msaa/LazyInstantiator.cpp#351
[2] http://searchfox.org/mozilla-central/source/accessible/windows/msaa/Compatibility.cpp#134
Depends on: 1384672
Assignee: nobody → jmathies
Attached patch wip (obsolete) — Splinter Review
Attachment #8890568 - Attachment is obsolete: true
Attachment #8890842 - Attachment is obsolete: true
No longer depends on: 1384672
Attached patch patch (obsolete) — Splinter Review
Attachment #8890845 - Flags: review?(aklotz)
Comment on attachment 8890845 [details] [diff] [review]
patch

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

I just noticed I'm including the exception handler header twice in both files, will address in a follow up.
Attached patch patch v.2 (obsolete) — Splinter Review
Went ahead and updated.
Attachment #8890845 - Attachment is obsolete: true
Attachment #8890845 - Flags: review?(aklotz)
Attachment #8890851 - Flags: review?(aklotz)
Comment on attachment 8890851 [details] [diff] [review]
patch v.2

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

You are going to need to fix the following preprocessor defines around LazyInstantiator::AccumulateTelemetry to make the conditional defined(MOZ_TELEMETRY_REPORTING) || defined(MOZ_CRASHREPORTER)

http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/accessible/windows/msaa/LazyInstantiator.cpp#278
http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/accessible/windows/msaa/LazyInstantiator.cpp#247
http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/accessible/windows/msaa/LazyInstantiator.h#89

Then you'll want to make the call to Telemetry::ScalarSet conditional on defined(MOZ_TELEMETRY_REPORTING) only.

r- because I'd like to take another look before landing.
Attachment #8890851 - Flags: review?(aklotz) → review-
Attached patch patch v.3Splinter Review
updated per comments.
Attachment #8890851 - Attachment is obsolete: true
Attachment #8892923 - Flags: review?(aklotz)
Attachment #8892923 - Flags: review?(aklotz) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fb848f123a8
Annotate crash reports with accessibility client information. r=aklotz
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8fb848f123a8
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8892923 [details] [diff] [review]
patch v.3

Approval Request Comment
[Feature/Bug causing the regression]:
useful debugging tool - changes add annotations about a11y client to crash reports.
[User impact if declined]:
none
[Is this code covered by automated tests?]:
no
[Has the fix been verified in Nightly?]:
yes
[Needs manual test from QE? If yes, steps to reproduce]: 
no
[List of other uplifts needed for the feature/fix]:
none
[Is the change risky?]:
no
[Why is the change risky/not risky?]:
new crash annotations
[String changes made/needed]:
none
Attachment #8892923 - Flags: approval-mozilla-beta?
Comment on attachment 8892923 [details] [diff] [review]
patch v.3

Looks useful for crash-stats info, let's uplift this for beta 1.
Attachment #8892923 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Jim Mathies [:jimm] from comment #12)
> [Is this code covered by automated tests?]:
> no
> [Has the fix been verified in Nightly?]:
> yes
> [Needs manual test from QE? If yes, steps to reproduce]: 
> no

Setting qe-verify- based on Jim's assessment on manual testing needs.
Flags: qe-verify-
https://hg.mozilla.org/projects/date/rev/8fb848f123a8f1b22a61bcbb57a1583f40e64b36
Bug 1384560 - Annotate crash reports with accessibility client information. r=aklotz
You need to log in before you can comment on or make changes to this bug.