1323069 - Identification of remote a11y clients

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

In Hawaii, Jamie and I were discussing a way to determine whether we can collect information about who is attempting to instantiate a11y.

We figured out that we could implement IMessageFilter for incoming requests to the chrome process's main thread STA. If we returned a dummy IAccessible that was essentially a factory, we could use the IMessageFilter to figure out who was calling a method on that factory and respond accodingly.

This would allow us to collect telemetry on which processes affect a11y, as well as allow us to potentially avoid instantiating full a11y services for clients whose use cases we know to not require them.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

My WIP patch converts thread id to the leaf name of the executable. Keeping privacy considerations in mind, are there any other details that we might want? Version number, perhaps?

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Aaron Klotz [:aklotz] from comment #1)
> My WIP patch converts thread id to the leaf name of the executable. Keeping
> privacy considerations in mind, are there any other details that we might
> want? Version number, perhaps?

It would be nice to add version info later if we need it.

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

This might really help us if we need to do a staged rollout as more AT vendors come onboard -- get properly tested.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to David Bolter [:davidb] from comment #2)
> (In reply to Aaron Klotz [:aklotz] from comment #1)
> > My WIP patch converts thread id to the leaf name of the executable. Keeping
> > privacy considerations in mind, are there any other details that we might
> > want? Version number, perhaps?
> 
> It would be nice to add version info later if we need it.

Who am I kidding... we'll need it.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1: mscom changes to facilitate resolution of COM remote client thread ID

These changes insert a message filter on the main thread. The message filter is just a passthrough, but it records the incoming thread ID which is then exposed by mscom::MainThreadRuntime::GetClientThreadId().

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Declare A11Y_INSTANTIATORS telemetry probe

We'd like to add an opt-out probe that records the leaf name and version number of the remote executable that is responsible for instantiating a11y in the current session.

The format consists of the leaf name of the executable, with its version number concatenated to it in the format "|a.b.c.d" (if that information is available).

Examples:

"foo.exe|3.1.0.1234"
"bar.exe"

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 3: Add the ability to lazily instantiate a11y, gather telemetry for remote clients, and potentially block unwanted a11y instantiations

LazyInstantiator acts on its own until the client does something that requires a11y to start. Once that happens, LazyInstantiator retrieves the root accessible and then aggregates the root so that the client doesn't realize that anything has changed.

LazyInstantiator needs to do a lot of funky stuff to be able to transparently wrap itself around the root accessible once a11y has been started. This is partially due to the quirks of COM aggregation, but also due to the fact that accessibles are cycle-collected.

I have tried to comment the things that I expect will raise questions, but I am sure that there will probably need to be further explanation! ;)

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 4: Widget changes to facilitate returning of a11y::LazyInstantiator in reponse to WM_GETOBJECT

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 8878163 [details] [diff] [review]
Part 1: mscom changes to facilitate resolution of COM remote client thread ID

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

::: ipc/mscom/MainThreadClientInfo.cpp
@@ +14,5 @@
> +namespace mozilla {
> +namespace mscom {
> +
> +HRESULT
> +MainThreadClientInfo::Create(MainThreadClientInfo** aOutObj)

nit - add //static to this

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 8878176 [details] [diff] [review]
Part 4: Widget changes to facilitate returning of a11y::LazyInstantiator in reponse to WM_GETOBJECT

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

Can't wait to see what this digs up!

Comment 11

[Eitan Isaacson [:eeejay]]

Comment on attachment 8878172 [details] [diff] [review]
Part 3: Add the ability to lazily instantiate a11y, gather telemetry for remote clients, and potentially block unwanted a11y instantiations

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

Very cool!

::: accessible/windows/msaa/LazyInstantiator.cpp
@@ +43,5 @@
> +  // To track this, we set the kLazyInstantiatorProp on the HWND with a pointer
> +  // to an existing instance. We only create a new LazyInstatiator if that prop
> +  // has not already been set.
> +  LazyInstantiator* existingInstantiator =
> +    reinterpret_cast<LazyInstantiator*>(::GetProp(aHwnd, kLazyInstantiatorProp));

Silly question, this prop is stored server-side, right? So if a second AT comes along, it will get a cached version?

@@ +74,5 @@
> +  // unknown (which is not wrapped by the LazyInstantiator) and then querying
> +  // that for IID_IAccessible.
> +  a11y::RootAccessibleWrap* rootWrap =
> +    static_cast<a11y::RootAccessibleWrap*>(rootAcc);
> +  RefPtr<IUnknown> punk(rootWrap->GetInternalUnknown());

Good idea. Do bad things happen if we continue using the wrapped root accessible?

@@ +227,5 @@
> +
> +  auto verInfoBuf = MakeUnique<BYTE[]>(verInfoSize);
> +
> +  if (!::GetFileVersionInfo(fullPath.get(), 0, verInfoSize, verInfoBuf.get())) {
> +    return;

This is pretty cool. It works on most executables?

@@ +279,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (!aValue.IsEmpty()) {
> +    Telemetry::ScalarSet(Telemetry::ScalarID::A11Y_INSTANTIATORS, aValue);

I have only done this kind of this with a keyed histogram. Will we get an aggregated bucket count of a11y instantiators?

@@ +370,5 @@
> +
> +    return S_OK;
> +  }
> +
> +  // If we don't want a real root, let's resolve a fake one.

Tough love. Is this just a childless accessible with the top-window bounds?

@@ +445,5 @@
> +      // for more info).
> +      --result;
> +    }
> +  } else {
> +    result = --mRefCnt;

Can we end up in a mixed state, where both mRefCnt is > 0 and mRealRootUnk is not null? eg. this object is created, has a few refs before root accessible is resolved.

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbc6b0e38e73f310f274aa793427c11db4995aaf

Comment 13

[Benjamin Smedberg]

Comment on attachment 8878166 [details] [diff] [review]
Part 2: Declare A11Y_INSTANTIATORS telemetry probe

I have no problem with collecting this for 6 months.

If this really does need to be forever, I'm interested in talking through with you how this will provide long-term value and who's going to be responsible for monitoring/using the data. Is this correlational data?

data-r=me with expires: 61 or come back to me about permanent monitoring.

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #13)
> Comment on attachment 8878166 [details] [diff] [review]
> Part 2: Declare A11Y_INSTANTIATORS telemetry probe
> 
> I have no problem with collecting this for 6 months.
> 
> If this really does need to be forever, I'm interested in talking through
> with you how this will provide long-term value and who's going to be
> responsible for monitoring/using the data. Is this correlational data?
> 
> data-r=me with expires: 61 or come back to me about permanent monitoring.

I'll land with 61 and leave the rest up to a11y team to decide how they want to handle this longer-term.

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=87636d9b9ded2f169a846d72ed43ce9214c6f5fe

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 3: Add the ability to lazily instantiate a11y, gather telemetry for remote clients, and potentially block unwanted a11y instantiations (r2)

Sorry Eitan, my pre-landing try push had lots of failures and I noticed some other problems during testing, so I had to make some changes to the patch.

* LazyInstantiator has to implement IServiceProvider in order to avoid premature instantiation of a11y;

* RootAccessibleWrap was delegating its InternalQueryInterface, InternalAddRef, and InternalRelease to the wrong superclasses, which was messing up refcount logging and creating false positives in leakchecks;

* I had to introduce the mAllowBlindAggregation variable to LazyInstantiator to make its QueryInterface work properly. See the comments above LazyInstantiator::EnableBlindAggregation() for more info.

The changes are fairly small but significant enough that I wanted to run it by you again.

I'll answer your questions from the previous review:

(In reply to Eitan Isaacson [:eeejay] from comment #11)
> Comment on attachment 8878172 [details] [diff] [review]
> Part 3: Add the ability to lazily instantiate a11y, gather telemetry for
> remote clients, and potentially block unwanted a11y instantiations
> 
> Review of attachment 8878172 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Very cool!
> 
> ::: accessible/windows/msaa/LazyInstantiator.cpp
> @@ +43,5 @@
> > +  // To track this, we set the kLazyInstantiatorProp on the HWND with a pointer
> > +  // to an existing instance. We only create a new LazyInstatiator if that prop
> > +  // has not already been set.
> > +  LazyInstantiator* existingInstantiator =
> > +    reinterpret_cast<LazyInstantiator*>(::GetProp(aHwnd, kLazyInstantiatorProp));
> 
> Silly question, this prop is stored server-side, right? So if a second AT
> comes along, it will get a cached version?

That's correct. We can only have one LazyInstantiator wrapping the root at any given time, so we cache it server-side via the HWND.

> 
> @@ +74,5 @@
> > +  // unknown (which is not wrapped by the LazyInstantiator) and then querying
> > +  // that for IID_IAccessible.
> > +  a11y::RootAccessibleWrap* rootWrap =
> > +    static_cast<a11y::RootAccessibleWrap*>(rootAcc);
> > +  RefPtr<IUnknown> punk(rootWrap->GetInternalUnknown());
> 
> Good idea. Do bad things happen if we continue using the wrapped root
> accessible?

Not really, but once a11y is up, this just becomes an other layer of unnecessary indirection.

> 
> @@ +227,5 @@
> > +
> > +  auto verInfoBuf = MakeUnique<BYTE[]>(verInfoSize);
> > +
> > +  if (!::GetFileVersionInfo(fullPath.get(), 0, verInfoSize, verInfoBuf.get())) {
> > +    return;
> 
> This is pretty cool. It works on most executables?

It requires the executables to have embedded version information in their resources at compile time. Most commercial vendors do this (it is in their own interests for debugging purposes, after all), but not always: that's why this code is written to successfully write the probe even if we cannot extract the version info.

> 
> @@ +279,5 @@
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +
> > +  if (!aValue.IsEmpty()) {
> > +    Telemetry::ScalarSet(Telemetry::ScalarID::A11Y_INSTANTIATORS, aValue);
> 
> I have only done this kind of this with a keyed histogram. Will we get an
> aggregated bucket count of a11y instantiators?

Yes we will get aggregated buckets. One drawback of my approach is that we only record the first binary that instantiates a11y, so if there is more than one remote consumer, we don't know who the additional ones are. But in practice I don't think that's too important.

> 
> @@ +370,5 @@
> > +
> > +    return S_OK;
> > +  }
> > +
> > +  // If we don't want a real root, let's resolve a fake one.
> 
> Tough love. Is this just a childless accessible with the top-window bounds?

Honestly, from what I've seen, the system merely gives us a nullptr accessible in this particular case. But IMHO implementing things this way is the most future-proof should something change under Windows' hood.

> 
> @@ +445,5 @@
> > +      // for more info).
> > +      --result;
> > +    }
> > +  } else {
> > +    result = --mRefCnt;
> 
> Can we end up in a mixed state, where both mRefCnt is > 0 and mRealRootUnk
> is not null? eg. this object is created, has a few refs before root
> accessible is resolved.

We should be able to handle that case, as TransplantRefCnt will ensure that mRefCnt is converted into AddRef()s on mRealRootUnk once the root acc is resolved.

Comment 17

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f318bae76a85daf03d149e1cc26140118d9ba1ed

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ab86873e8fb40fc77a02421bcd1cf4a4457d5bd

Comment 19

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0b6c3ce83a44a21309f04699bcf11d4d3b3669e
Bug 1323069: mscom changes to facilitate resolution of remote client thread ID; r=jimm

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec02abc4bbc78cd7892b43068ed2ec44e01e71cf
Bug 1323069: Declare A11Y_INSTANTIATORS telemetry probe; r=bsmedberg

https://hg.mozilla.org/integration/mozilla-inbound/rev/94ba0b72c28ac6fbb6a680c52ab073f10503eb16
Bug 1323069: Add ability to detect and identify remote a11y clients, as well as lazily instantiate a11y; r=eeejay

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca7b176b568132efc80229303db32e3bad6974c8
Bug 1323069: Widget changes to facilitate returning a11y::LazyInstantiator in response to WM_GETOBJECT; r=jimm

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7bdf1aa4e51482e51c0104845dc249cdc027a809
Bug 1323069: Fix conflict between this bug and bug 1371274; r=bustage CLOSED TREE

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d0b6c3ce83a4
https://hg.mozilla.org/mozilla-central/rev/ec02abc4bbc7
https://hg.mozilla.org/mozilla-central/rev/94ba0b72c28a
https://hg.mozilla.org/mozilla-central/rev/ca7b176b5681
https://hg.mozilla.org/mozilla-central/rev/7bdf1aa4e514

Comment 22

[David Bolter [:davidb] (NeedInfo me for attention)]

Woohoooooooo! \o/