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.

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?

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

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

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

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().

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"

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 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 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 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 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.

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

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.

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

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

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

Woohoooooooo! \o/