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]
:
: alexander :surkov
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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Arun [:arunsl] 2012-07-16 20:17:42 PDT
Created attachment 642848 [details] [diff] [review]
Patch v4

review comments are incorporated
Comment 17 User image Trevor Saunders (:tbsaunde) 2012-07-20 11:59:19 PDT
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/a746aaa32b22
Comment 18 User image 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.