telemetry for injected screen reader dll's

RESOLVED FIXED in mozilla12

Status

()

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

People

(Reporter: tbsaunde, Assigned: davidb)

Tracking

unspecified
mozilla12
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
wE SHOULD PROBABLY DO THIS FOR ATLEAST JAWS AND WE RIGHT?  wE HAVE  SOME CODE IN THE TREE TO GET THE VERSION OF A DLL, CURRENTLY ONLY USED TO GET THE JAWS VERSION, BUT SHOULD WORK AS WELL FOR OTHER DLL'S WE KNOW THE NAME OF i BELIVE.  tHE QUESTIONS ARE, AREWE INTERESTED IN MORE THAN THE MAJOR VERSION, THE HIGH 16 BITS OF MS i GUES IS, OR MORE? IF SO, DO WE HAVE IDEAS ON ENCODING THIS INFORMATION TO REPORT AS A SINGAL INTEGER TO TELEETRY?
(Reporter)

Comment 1

6 years ago
Taras, we'd like to get information about number of users using firefox with different screen readers, and which versions are in use.  Telemetry seems like a reasonable tool to use  for this right?  These screen readers are detected  because they inject a dll into the firefox process, see accessible/src/msaa/nsAccessNodeWrap.cpp line 671 for some code we already use to sniff the version of one of these dll's screen readers.  when we do this we get 2 32 bit integers worth of version information.  I twould appear that those two integers are best looked at as a set of 4 16 bit numbers, where each 16 bit word has one version number part. so for example the version number 7.1.1122 would have the highest order word be 7 then the next be 1 the third  1122 and the forth 0.  Have you considered at all how you might report data like this? I mean it would be nice for the histogram of this data to be easy to read instead of having to go through the raw data.  we also need to figure how much of such a version number we actually care about, I would think only  major versions that aren't free upgrades for the comercial screen readers.
Blocks: 648121
Summary: teleetry for injected screen reader dll's → telemetry for injected screen reader dll's
(Reporter)

Updated

6 years ago
OS: Linux → Windows 7
Hardware: x86_64 → x86

Comment 2

6 years ago
Not sure what to do about turning the dll info into strings on the client-side.

Currently we report addons which are a freeform string(which i think is structured as <id>, <id>...). Can do the same with accessibility.

See https://hg.mozilla.org/mozilla-central/file/fb919f4fa210/toolkit/components/telemetry/TelemetryPing.js#l267 for an example of how to attach a non-histogram .addons to the telemetry packet.
(Reporter)

Comment 3

6 years ago
> See
> https://hg.mozilla.org/mozilla-central/file/fb919f4fa210/toolkit/components/
> telemetry/TelemetryPing.js#l267 for an example of how to attach a
> non-histogram .addons to the telemetry packet.

It looks like all the strings are handled in js?  getting  the strings we want to report in js seems tricky, since we don't want to get the  accessibility service if it isn't already running.  In c++ we can get the accessibility service but not create it if it isn't running already, but I'm not sure that's something we can or want to expose to js.  So I think we'd want to add some machinary to report strings in js, does that seem reasonable?

Comment 4

6 years ago
All we should do is to make accessibility to notify observers about "ATDetected" notification (like it's done for addons https://hg.mozilla.org/mozilla-central/file/fb919f4fa210/toolkit/components/telemetry/TelemetryPing.js#l445). Since observer service is available for trusted content only then perhaps it's ok to reveal this kind of information with script (keep in mind, Google proposed technically similar things - AT detection for web services). So that should be ok.

But this looks like a hacky approach, I'd like to see a common way (especially because a11y is not unique consumer).
(Reporter)

Comment 5

6 years ago
Yeah, I'm ok with  letting chrome code know if there's a screen reader running, but this feels like a pretty heavy weight hacky solution.
(Assignee)

Comment 6

6 years ago
Taras are we going to be collecting telemetry on injected dlls generally? Do you know of any related work happening?

Comment 7

6 years ago
(In reply to David Bolter [:davidb] from comment #6)
> Taras are we going to be collecting telemetry on injected dlls generally? Do
> you know of any related work happening?

you guys are the first to actually file a bug and start planning an implementation. Some people are considering doing something similar for detecting AV software, etc
(Assignee)

Comment 8

6 years ago
(In reply to alexander surkov from comment #4)
> All we should do is to make accessibility to notify observers about
> "ATDetected" notification (like it's done for addons
> https://hg.mozilla.org/mozilla-central/file/fb919f4fa210/toolkit/components/
> telemetry/TelemetryPing.js#l445). Since observer service is available for
> trusted content only then perhaps it's ok to reveal this kind of information
> with script (keep in mind, Google proposed technically similar things - AT
> detection for web services). So that should be ok.
> 
> But this looks like a hacky approach, I'd like to see a common way
> (especially because a11y is not unique consumer).

Am I right that we want C++ API for adding string data to telemetry?

I have only ever seen histograms via about:telemetry - is there another representation we are expecting to use here?

Comment 9

6 years ago
Yes, expose a string via some nsInterface and then TelemetryPing.js can stick them into the ping
(Assignee)

Comment 10

6 years ago
This makes sense to me. Alexander, how about we create an nsIAccessibleTelemetry for this and similar future cases?
(Assignee)

Updated

6 years ago
Assignee: trev.saunders → bolterbugz
(Assignee)

Updated

6 years ago
Depends on: 704852
(Assignee)

Comment 11

6 years ago
OK, given the recent work on msaa/Compatibility.* how about this plan:

1. Add compatibility modes for other known modules.
2. expose sMode to Telemetry.

Alexander, Trevor objections?
(Assignee)

Comment 12

6 years ago
Created attachment 589196 [details] [diff] [review]
WIP
(Assignee)

Comment 13

6 years ago
Created attachment 589200 [details] [diff] [review]
WIP (part 2, added STS and NVDA)
(Assignee)

Comment 14

6 years ago
Note this is not finished since I need to be able to differentiate potential conflicts. (E.g. 01 + 10 vs 11)
(Assignee)

Comment 15

6 years ago
(In reply to David Bolter [:davidb] from comment #14)
> Note this is not finished since I need to be able to differentiate potential
> conflicts. (E.g. 01 + 10 vs 11)

Ignore this rubbish.
(Assignee)

Comment 16

6 years ago
Created attachment 589225 [details] [diff] [review]
patch1 (simple approach)

This should do what we need. I tested with NVDA, Jaws, and both at the same time.
Attachment #589196 - Attachment is obsolete: true
Attachment #589200 - Attachment is obsolete: true
Attachment #589225 - Flags: review?(trev.saunders)
Attachment #589225 - Flags: review?(marco.zehe)
Comment on attachment 589225 [details] [diff] [review]
patch1 (simple approach)

>+  if (::GetModuleHandleW(L"nvdaHelperRemote"))
>+    sMode |= NVDAMode;

Hm, I thought this was some sort of vBufbackend module that NVDA injects, but that may have changed. Did you find this by sniffing or by info from Jamie?
(Assignee)

Comment 18

6 years ago
(In reply to Marco Zehe (:MarcoZ) from comment #17)
> >+  if (::GetModuleHandleW(L"nvdaHelperRemote"))
> 
> Hm, I thought this was some sort of vBufbackend module that NVDA injects,
> but that may have changed. Did you find this by sniffing or by info from
> Jamie?

Good memory. I emailed him this morning to advise which of the following would be best:
1. IAccessible2Proxy.dll (clashes with accprobe)
2. minHook.dll
3. nvdaHelperRemote.dll
4. VBufBackend_gecko_ia2.dll
(Assignee)

Comment 19

6 years ago
Jamie confirmed the current approach is good (#3).
(Assignee)

Comment 20

6 years ago
Created attachment 589280 [details] [diff] [review]
patch 1.1 (adjusted max as per IRC)
Attachment #589225 - Attachment is obsolete: true
Attachment #589225 - Flags: review?(trev.saunders)
Attachment #589225 - Flags: review?(marco.zehe)
Attachment #589280 - Flags: superreview?(marco.zehe)
(Reporter)

Comment 21

6 years ago
Comment on attachment 589280 [details] [diff] [review]
patch 1.1 (adjusted max as per IRC)

> 
> #include "Compatibility.h"
>-
> #include "nsWinUtils.h"
>+#include "Statistics.h"

I'd prefer you left that blank line between this files header and the other a11y ones.

otherwise if your fine with interpreting  the histogram into the bitfield this seems fine.

btw when did Marco make it onto the sr? list?
Attachment #589280 - Flags: review+
(Assignee)

Comment 22

6 years ago
Comment on attachment 589280 [details] [diff] [review]
patch 1.1 (adjusted max as per IRC)

heehee
Attachment #589280 - Flags: superreview?(marco.zehe) → review?(marco.zehe)
Comment on attachment 589280 [details] [diff] [review]
patch 1.1 (adjusted max as per IRC)

r=me with the right NVDA module included, depending on Jamie's reply.
Attachment #589280 - Flags: review?(marco.zehe) → review+
Err, just now saw comment#19, so unconditional r=me.

Comment 25

6 years ago
Comment on attachment 589280 [details] [diff] [review]
patch 1.1 (adjusted max as per IRC)

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

::: accessible/src/msaa/Compatibility.cpp
@@ +108,5 @@
>      if (!Preferences::HasUserValue("browser.ctrlTab.disallowForScreenReaders"))
>        Preferences::SetBool("browser.ctrlTab.disallowForScreenReaders", true);
>    }
> +
> +  statistics::A11yConsumers(sMode); // Telemetry

you expose IA2OffMode as consumer, sounds a little bit weird

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +55,5 @@
>  /**
>   * a11y telemetry
>   */
>  HISTOGRAM_BOOLEAN(A11Y_INSTANTIATED, "has accessibility support been instantiated")
> +HISTOGRAM(A11Y_CONSUMERS, 0, 32, 33, LINEAR, "Current known consumers (bitfield) of accessibility")

unreadable on user side, any plans/ideas to make it more userfriendly?
(Assignee)

Comment 26

6 years ago
(In reply to alexander :surkov from comment #25)

> > +  statistics::A11yConsumers(sMode); // Telemetry
> 
> you expose IA2OffMode as consumer, sounds a little bit weird

I agree, but it isn't particularly harmful.

> > +HISTOGRAM(A11Y_CONSUMERS, 0, 32, 33, LINEAR, "Current known consumers (bitfield) of accessibility")
> 
> unreadable on user side, any plans/ideas to make it more userfriendly?

The data will be there. I think we'll have ways to make it more easily readable but really this bug is about collecting the data.

Comment 27

6 years ago
(In reply to David Bolter [:davidb] from comment #26)
> (In reply to alexander :surkov from comment #25)
> 
> > > +  statistics::A11yConsumers(sMode); // Telemetry
> > 
> > you expose IA2OffMode as consumer, sounds a little bit weird
> 
> I agree, but it isn't particularly harmful.

it is but weird anyway, you could unset this bit easily if it makes sense. Otherwise you could rename it to "a11y_modes" or something like that.

> > > +HISTOGRAM(A11Y_CONSUMERS, 0, 32, 33, LINEAR, "Current known consumers (bitfield) of accessibility")
> > 
> > unreadable on user side, any plans/ideas to make it more userfriendly?
> 
> The data will be there. I think we'll have ways to make it more easily
> readable but really this bug is about collecting the data.

true but the question is not answered actually
(In reply to David Bolter [:davidb] from comment #26)
> (In reply to alexander :surkov from comment #25)
> > > +HISTOGRAM(A11Y_CONSUMERS, 0, 32, 33, LINEAR, "Current known consumers (bitfield) of accessibility")
> > 
> > unreadable on user side, any plans/ideas to make it more userfriendly?
> 
> The data will be there. I think we'll have ways to make it more easily
> readable but really this bug is about collecting the data.

Why not just have N buckets for all the defined consumers, and then accumulate into multiple buckets?  So you have a bucket for Dolphin, a bucket for JAWS, and so forth.  That way you avoid exponential blowup as (or if) more consumers are added, and the buckets are somewhat more straightforward to understand.  (Telemetry doesn't have any means for providing labels for buckets currently, though.)
(Assignee)

Comment 29

6 years ago
I thought buckets shown were automatically reduced/minimized based on data?

Maybe I'm not understanding you though.
The telemetry ping sent in doesn't include zero buckets, if that's what you are referring to by automatically reduced.  But the zero buckets are still stored in memory on the C++ side.

Please do individual buckets.
(Assignee)

Comment 31

6 years ago
Comment on attachment 589280 [details] [diff] [review]
patch 1.1 (adjusted max as per IRC)

I chatted with Nathan on IRC (thanks!)

I'll work up a new patch.
Attachment #589280 - Attachment is obsolete: true
(Assignee)

Comment 32

6 years ago
Created attachment 589872 [details] [diff] [review]
patch 2
Attachment #589872 - Flags: review?(trev.saunders)
(Assignee)

Updated

6 years ago
Attachment #589872 - Flags: review?(nfroyd)
Comment on attachment 589872 [details] [diff] [review]
patch 2

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

Looks great, thanks for redoing it this way.  r+ with the bits about histogram definition addressed.

::: accessible/src/msaa/Compatibility.h
@@ +107,5 @@
> +    WE = 3,
> +    DOLPHIN = 4,
> +    SEROTEK = 5
> +  };
> +

Drive-by comment: You may want to add a comment about the need to update TelemetryHistograms.h if this list changes.  Not my code, though, so your call.

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +55,5 @@
>  /**
>   * a11y telemetry
>   */
>  HISTOGRAM_BOOLEAN(A11Y_INSTANTIATED, "has accessibility support been instantiated")
> +HISTOGRAM(A11Y_CONSUMERS, 1, 6, 6, LINEAR, "Current known consumers (bitfield) of accessibility")

This should be:

HISTOGRAM(A11Y_CONSUMERS, 1, 5, 6, LINEAR, ...)

since there's an implicit bucket for 0, which you'll want for NVDA.

Comment also needs to be corrected.
Attachment #589872 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 34

6 years ago
Thanks Nathan, corrected locally.
(Reporter)

Updated

6 years ago
Attachment #589872 - Flags: review?(trev.saunders) → review+

Comment 35

6 years ago
Comment on attachment 589872 [details] [diff] [review]
patch 2

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

::: accessible/src/base/Statistics.h
@@ +48,5 @@
>  
>    inline void A11yInitialized()
>      { Telemetry::Accumulate(Telemetry::A11Y_INSTANTIATED, true); }
>  
> +  inline void A11yConsumers(PRUint32 consumer)

nit: consumer -> aConsumer
(Assignee)

Comment 36

6 years ago
Thanks all.

https://hg.mozilla.org/integration/mozilla-inbound/rev/df1276b99406
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
(Assignee)

Comment 37

6 years ago
Reopening pending merge to central.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/df1276b99406
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.