Last Comment Bug 769302 - Please add about:support accessibility status to crash reports
: Please add about:support accessibility status to crash reports
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Arun [:arunsl]
:
:
Mentors:
Depends on: 759158 771574
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-28 08:49 PDT by David Bolter [:davidb]
Modified: 2012-07-20 21:03 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (1.33 KB, patch)
2012-06-29 15:22 PDT, Mark Capella [:capella]
dbolter: feedback+
Details | Diff | Splinter Review
Patch (v2) (1.67 KB, patch)
2012-07-01 11:58 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch v3, base mozilla-central (1.38 KB, patch)
2012-07-12 00:27 PDT, Arun [:arunsl]
tbsaunde+mozbugs: review+
dbolter: feedback+
Details | Diff | Splinter Review
Patch v4 (1.15 KB, patch)
2012-07-16 20:17 PDT, Arun [:arunsl]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description David Bolter [:davidb] 2012-06-28 08:49:20 PDT
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?
Comment 1 Trevor Saunders (:tbsaunde) 2012-06-28 13:08:35 PDT
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()
Comment 2 Ted Mielczarek [:ted.mielczarek] 2012-06-29 05:46:37 PDT
(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.
Comment 3 Mark Capella [:capella] 2012-06-29 15:22:07 PDT
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
Comment 4 David Bolter [:davidb] 2012-06-29 16:41:14 PDT
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.
Comment 5 Mark Capella [:capella] 2012-06-29 17:01:07 PDT
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 Robert Kaiser 2012-07-01 08:02:13 PDT
(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 7 Trevor Saunders (:tbsaunde) 2012-07-01 11:50:43 PDT
Comment on attachment 638029 [details] [diff] [review]
Patch (v1)

>+#if defined(MOZ_CRASHREPORTER)

nit, ifdef

>+#if defined(MOZ_CRASHREPORTER)

same
Comment 8 Trevor Saunders (:tbsaunde) 2012-07-01 11:51:40 PDT
(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
Comment 9 Mark Capella [:capella] 2012-07-01 11:58:24 PDT
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 ...
Comment 10 Mark Capella [:capella] 2012-07-05 21:44:46 PDT
Ok, Here's what I was missing...
https://wiki.mozilla.org/Breakpad
https://wiki.mozilla.org/Breakpad/Design
Comment 11 Mark Capella [:capella] 2012-07-10 06:37:21 PDT
Fyi, sneethling@mozilla.com from the Socorro team will be helping on their side.
Comment 12 Trevor Saunders (:tbsaunde) 2012-07-11 23:06:52 PDT
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
Comment 13 Arun [:arunsl] 2012-07-12 00:27:55 PDT
Created attachment 641384 [details] [diff] [review]
Patch v3, base mozilla-central

Patch with the comments incorporated. tested with base mozilla-central in mercurial.
Comment 14 David Bolter [:davidb] 2012-07-12 10:53:57 PDT
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!
Comment 15 Trevor Saunders (:tbsaunde) 2012-07-16 08:04:51 PDT
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.
Comment 16 Arun [:arunsl] 2012-07-16 20:17:42 PDT
Created attachment 642848 [details] [diff] [review]
Patch v4

review comments are incorporated
Comment 17 Trevor Saunders (:tbsaunde) 2012-07-20 11:59:19 PDT
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/a746aaa32b22
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-07-20 21:03:48 PDT
https://hg.mozilla.org/mozilla-central/rev/a746aaa32b22

Note You need to log in before you can comment on or make changes to this bug.