telemetry for injected screen reader dll's

RESOLVED FIXED in mozilla12

Status

()

defect
RESOLVED FIXED
8 years ago
8 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

8 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

8 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.
Summary: teleetry for injected screen reader dll's → telemetry for injected screen reader dll's
Reporter

Updated

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

Comment 2

8 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

8 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?
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

8 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.
Taras are we going to be collecting telemetry on injected dlls generally? Do you know of any related work happening?

Comment 7

8 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
(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

8 years ago
Yes, expose a string via some nsInterface and then TelemetryPing.js can stick them into the ping
This makes sense to me. Alexander, how about we create an nsIAccessibleTelemetry for this and similar future cases?
Assignee: trev.saunders → bolterbugz
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?
Posted patch WIP (obsolete) — Splinter Review
Note this is not finished since I need to be able to differentiate potential conflicts. (E.g. 01 + 10 vs 11)
(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.
Posted patch patch1 (simple approach) (obsolete) — Splinter Review
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?
(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
Jamie confirmed the current approach is good (#3).
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

8 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+
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+
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?
(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.
(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.)
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.
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
Posted patch patch 2Splinter Review
Attachment #589872 - Flags: review?(trev.saunders)
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+
Thanks Nathan, corrected locally.
Reporter

Updated

8 years ago
Attachment #589872 - Flags: review?(trev.saunders) → review+
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
Thanks all.

https://hg.mozilla.org/integration/mozilla-inbound/rev/df1276b99406
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Reopening pending merge to central.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/df1276b99406
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.