Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Please add about:support accessibility status to crash reports

RESOLVED FIXED in mozilla17

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: davidb, Assigned: arunsl)

Tracking

unspecified
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
We landed about:support info for accessibility yesterday in trunk. It would help us triage bugs to know if accessibility was activated. Can we include this info in crash reports?
(Reporter)

Updated

5 years ago
Depends on: 759158
I think you can just call do_GetService() to get the nsICrashreporter, and then call AnnottateCrashreport() on it with something like Accessibility-active" or so in nsAccessibilityService::Init()
Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++]
Component: General → Breakpad Integration
Product: Socorro → Toolkit
QA Contact: general → breakpad.integration
(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> I think you can just call do_GetService() to get the nsICrashreporter, and
> then call AnnottateCrashreport() on it with something like
> Accessibility-active" or so in nsAccessibilityService::Init()

This sounds about right, you'll want to do AnnotateCrashReport("Accessibility", "1") or something like that.

Generally, bugs for new crash report annotations are filed under the component in whose code we'd add the annotation.
Component: Breakpad Integration → Disability Access APIs
Product: Toolkit → Core
QA Contact: breakpad.integration → accessibility-apis
Created attachment 638029 [details] [diff] [review]
Patch (v1)

Hacked around with this for a little bit, before finding this simple solution. The relevent information appears / or not in the "App Notes" section...

Example crash reports:

Forced crash with accessibility not activated yet:
https://crash-stats.mozilla.com/report/index/bp-fb87e2b9-8000-47f3-bd8c-ebdc72120629

Forced crash with accessibility activated by DOMInspector:
https://crash-stats.mozilla.com/report/index/bp-6ed7baf5-5652-41be-a21e-3fd8a2120629
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #638029 - Flags: feedback?(dbolter)
(Reporter)

Comment 4

5 years ago
Comment on attachment 638029 [details] [diff] [review]
Patch (v1)

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

f=me. Thanks! It is a little unfortunate we are sandwiched between gfx adapter info and gfx webgl stuff. I'm not sure we can do much about that.
Attachment #638029 - Flags: feedback?(dbolter) → feedback+
Attachment #638029 - Flags: review?(trev.saunders)
It appears to be a function of code execution order: https://crash-stats.mozilla.com/report/index/bp-32f668bf-9add-4015-bc5a-e1cef2120629

Comment 6

5 years ago
(In reply to David Bolter [:davidb] from comment #4)
> It is a little unfortunate we are sandwiched between gfx
> adapter info and gfx webgl stuff. I'm not sure we can do much about that.

Probably not if you want to be in app notes. If in the future you want any statistics on this, you should make it a separate annotation like ted proposed in comment #2, though, and you'll want another bug filed about exposing this on Socorro.
Comment on attachment 638029 [details] [diff] [review]
Patch (v1)

>+#if defined(MOZ_CRASHREPORTER)

nit, ifdef

>+#if defined(MOZ_CRASHREPORTER)

same
Attachment #638029 - Flags: review?(trev.saunders)
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #6)
> (In reply to David Bolter [:davidb] from comment #4)
> > It is a little unfortunate we are sandwiched between gfx
> > adapter info and gfx webgl stuff. I'm not sure we can do much about that.
> 
> Probably not if you want to be in app notes. If in the future you want any
> statistics on this, you should make it a separate annotation like ted
> proposed in comment #2, though, and you'll want another bug filed about
> exposing this on Socorro.

Mark please do this, we may well want to collect statistics
Created attachment 638209 [details] [diff] [review]
Patch (v2)

The #ifdef seems to be done either of two ways, not sure it matters one way or the other ... (what the difference is?)

for example:
http://mxr.mozilla.org/mozilla-central/source/ipc/glue/GeckoChildProcessHost.cpp#526

As for the AnnottateCrashreport() approach, I've tried a couple ways (see attached) but must be missing something as I can't locate anything in the crash reports resembling a line item ...
Ok, Here's what I was missing...
https://wiki.mozilla.org/Breakpad
https://wiki.mozilla.org/Breakpad/Design
Depends on: 771574
Fyi, sneethling@mozilla.com from the Socorro team will be helping on their side.
Assignee: markcapella → nobody
Status: ASSIGNED → NEW
Comment on attachment 638209 [details] [diff] [review]
Patch (v2)

b/accessible/src/base/nsAccessibilityService.cpp
>--- a/accessible/src/base/nsAccessibilityService.cpp
>+++ b/accessible/src/base/nsAccessibilityService.cpp
>@@ -37,16 +37,20 @@
>+#if defined(MOZ_CRASHREPORTER)
>+#include "nsExceptionHandler.h"
>+#endif

nit, ifdef please

> 
> #ifdef DEBUG
>   logging::CheckEnv();
> #endif
> 
>   // Initialize accessibility.
>   nsAccessNodeWrap::InitAccessibility();
> 
>+#if defined(MOZ_CRASHREPORTER)

ifdef here too

>+  CrashReporter::
>+    AppendAppNotesToCrashReport(NS_LITERAL_CSTRING("Accessibility is Active.\n"));

I don't think we need the note and the annotation, just keep the annotation, and get rid of the note above.

>+  CrashReporter::
>+    AnnotateCrashReport(NS_LITERAL_CSTRING("Accessibility"),
>+                        NS_LITERAL_CSTRING("Active"));
>+
>+  nsCString aStatus("Active II");
>+  nsCOMPtr<nsICrashReporter> cr =
>+     do_GetService("@mozilla.org/toolkit/crash-reporter;1");
>+  cr->AnnotateCrashReport(NS_LITERAL_CSTRING("Accessibility II"), aStatus);

only one annotation is needed.

>+#endif
(Assignee)

Comment 13

5 years ago
Created attachment 641384 [details] [diff] [review]
Patch v3, base mozilla-central

Patch with the comments incorporated. tested with base mozilla-central in mercurial.
(Reporter)

Comment 14

5 years ago
Comment on attachment 641384 [details] [diff] [review]
Patch v3, base mozilla-central

Might as well request review from Trevor.
https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch#Getting_reviews

Thanks!
Attachment #641384 - Flags: feedback+
(Assignee)

Updated

5 years ago
Attachment #641384 - Flags: review?(trev.saunders)

Updated

5 years ago
Assignee: nobody → arun.sukumaran.latha
Comment on attachment 641384 [details] [diff] [review]
Patch v3, base mozilla-central

>+#include "nsICrashReporter.h"

nit, you don't need that

>+  nsCString aStatus("Active II");
>+  nsCOMPtr<nsICrashReporter> cr =
>+     do_GetService("@mozilla.org/toolkit/crash-reporter;1");
>+  cr->AnnotateCrashReport(NS_LITERAL_CSTRING("Accessibility II"), aStatus);

all of this is redundant with the function call above so just rm it.

r=tsaunde without that stuff.
Attachment #641384 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 16

5 years ago
Created attachment 642848 [details] [diff] [review]
Patch v4

review comments are incorporated
Attachment #641384 - Attachment is obsolete: true
Attachment #642848 - Flags: review?(trev.saunders)
Attachment #642848 - Flags: review?(trev.saunders) → review+
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/a746aaa32b22
https://hg.mozilla.org/mozilla-central/rev/a746aaa32b22
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.