Closed Bug 769302 Opened 12 years ago Closed 12 years ago

Please add about:support accessibility status to crash reports

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: davidb, Assigned: arunsl)

References

Details

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

Attachments

(3 files, 1 obsolete file)

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?
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
Attached patch Patch (v1)Splinter Review
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)
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)
(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
Attached patch Patch (v2)Splinter Review
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 ...
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
Attached patch Patch v3, base mozilla-central (obsolete) — Splinter Review
Patch with the comments incorporated. tested with base mozilla-central in mercurial.
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+
Attachment #641384 - Flags: review?(trev.saunders)
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+
Attached patch Patch v4Splinter Review
review comments are incorporated
Attachment #641384 - Attachment is obsolete: true
Attachment #642848 - Flags: review?(trev.saunders)
Attachment #642848 - Flags: review?(trev.saunders) → review+
https://hg.mozilla.org/mozilla-central/rev/a746aaa32b22
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: