1419886 - Find a way to determine UIA client instantiator

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Okay, I've figured out a really hacky way to do this. I have optimized it to take way less time than it initially did, however it still needs ~25ms to run on my machine, and it needs to be done synchronously on the main thread. -_-

My WIP is able to pull the instantiator and dump it to the debugger console. I still need to hook it into the various reporting mechanisms that we would want for this.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1: Add CallWndProc hook for UIA detection to nsAppShell

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Add UIA detection

This patch adds UIA detection. Furthermore, it also centralizes the code for recording the a11y instantiator so that we can share it between the LazyInstantiator stuff and the UIA stuff.

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 3: Add telemetry probe to measure how long UIA detection takes

Since the UIA detection code is slow, I'd like to add a telemetry probe to measure how long this detection code actually takes in the wild.

We'll need to know this in order to evaluate whether to allow this code to ride the trains beyond Nightly.

r? rweiss for privacy review.

Comment 5

[James Teh [:Jamie]]

Comment on attachment 8934364 [details] [diff] [review]
Part 2: Add UIA detection

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

Wow! Crazy stuff! Respect! :) I think this might need a bit of tweaking, though.

::: accessible/base/Platform.h
@@ +50,5 @@
>  bool IsHandlerRegistered();
>  
>  /*
>   * Name of platform service that instantiated accessibility
> + * @return nsIFile containing instantiator's process name

nit: This comment is a bit confusing because the Set function takes a pid. It'd be good if this could be clarified. This might involve splitting this into two comments (one for Set and one for Get).

::: accessible/windows/msaa/CompatibilityUIA.cpp
@@ +37,5 @@
> +{
> +  aLocal.reset();
> +
> +  if (!aProcess) {
> +    HANDLE rawProcess = ::OpenProcess(PROCESS_DUP_HANDLE, FALSE, aSrcPid);

I'm pretty sure this is going to fail with access denied on processes with the uiAccess privilege, since uiAccess processes get a higher integrity level:

> Applications that are launched with UIAccess rights for a standard user are assigned a slightly higher integrity level value in the access token. The access token integrity level for the UIAccess application for a standard user is the value of medium integrity level, plus an increment of 0x10. The higher integrity level for UIAccess applications prevents other processes on the same desktop at the medium integrity level from opening the UIAccess process object.
(https://msdn.microsoft.com/en-us/library/bb625963.aspx)

Most assistive technology products will have this privilege. NVDA, JAWS and Microsoft's tools (Narrator, Magnifier, etc.) certainly all do. I just tried this locally and my test process was unable to OpenProcess with NVDA's pid. So you won't be able to detect real AT like this.

Having said that, the clients that are most important to detect aren't actual AT products. They probably don't have this privilege, but we can't rule out that possibility.

I don't think there's anything we can really do about this. I raise it more as something to be aware of.

@@ +260,5 @@
> +    if (curHandle.mPid == ourPid) {
> +      // Our own process also has a handle to the section of interest. While we
> +      // don't want our own pid, this *does* give us an opportunity to speed up
> +      // future iterations by examining each handle for its kernel object (which
> +      // is unique across all processes) instead of searching by name.

nit: "unique across all processes" is totally valid language, but it initially confused me; I kept reading it as "unique in all processes", which is very different. Consider changing to "the same for all processes" to be super clear.

::: accessible/windows/msaa/LazyInstantiator.cpp
@@ +287,5 @@
> +  if (uiaPid) {
> +    return uiaPid.value();
> +  }
> +
> +  return mscom::MainThreadRuntime::GetClientThreadId();

This returns a process id for UIA and a *thread* id for MSAA.
ShouldInstantiate takes a thread id, not a process id.
So, I think this will fail for UIA.
But... UIA has already set the instantiator anyway in Compatibility::OnUIAMessage, so does it really need to set it in ShouldInstantiate as well?
For that matter, will ShouldInstantiate even get called for UIA? Have we blocked UIA before that point anyway?
If we do want ShouldInstantiate to get called for UIA, that also means we'd need to check the UIA block list in two places.

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 8934361 [details] [diff] [review]
Part 1: Add CallWndProc hook for UIA detection to nsAppShell

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

::: widget/windows/nsAppShell.cpp
@@ +211,5 @@
> +    }
> +  }
> +
> +  if (!gUiaMsg) {
> +    gUiaMsg = ::RegisterWindowMessageW(L"HOOKUTIL_MSG");

a comment explaining what this message is would be helpful.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1: Add CallWndProc hook for UIA detection to nsAppShell

Addressed comment, carrying forward r+

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Add UIA detection (r2)

(In reply to James Teh [:Jamie] from comment #5)
> Comment on attachment 8934364 [details] [diff] [review]
> Part 2: Add UIA detection
> 
> Review of attachment 8934364 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/base/Platform.h
> @@ +50,5 @@
> >  bool IsHandlerRegistered();
> >  
> >  /*
> >   * Name of platform service that instantiated accessibility
> > + * @return nsIFile containing instantiator's process name
> 
> nit: This comment is a bit confusing because the Set function takes a pid.
> It'd be good if this could be clarified. This might involve splitting this
> into two comments (one for Set and one for Get).

I decided to overhaul these functions so that now SetInstantiator returns void and GetInstantiator outputs a nsIFile. Internally we now store the nsIFile itself instead of just the path.

> 
> ::: accessible/windows/msaa/CompatibilityUIA.cpp
> @@ +37,5 @@
> > +{
> > +  aLocal.reset();
> > +
> > +  if (!aProcess) {
> > +    HANDLE rawProcess = ::OpenProcess(PROCESS_DUP_HANDLE, FALSE, aSrcPid);
> 
> I'm pretty sure this is going to fail with access denied on processes with
> the uiAccess privilege, since uiAccess processes get a higher integrity
> level:
> 
> > Applications that are launched with UIAccess rights for a standard user are assigned a slightly higher integrity level value in the access token. The access token integrity level for the UIAccess application for a standard user is the value of medium integrity level, plus an increment of 0x10. The higher integrity level for UIAccess applications prevents other processes on the same desktop at the medium integrity level from opening the UIAccess process object.
> (https://msdn.microsoft.com/en-us/library/bb625963.aspx)
> 
> Most assistive technology products will have this privilege. NVDA, JAWS and
> Microsoft's tools (Narrator, Magnifier, etc.) certainly all do. I just tried
> this locally and my test process was unable to OpenProcess with NVDA's pid.
> So you won't be able to detect real AT like this.
> 
> Having said that, the clients that are most important to detect aren't
> actual AT products. They probably don't have this privilege, but we can't
> rule out that possibility.
> 
> I don't think there's anything we can really do about this. I raise it more
> as something to be aware of.

Interesting. You're right, I expect that this will still work for the common case of programs that aren't requesting uiAccess.

I think we should land this as-is for now. I have realized that we *can* still make this work even for uiAccess processes by comparing the kernel object pointers, but that would require a second pass over the handle list in the worst case. I propose that we consider that to be a future enhancement if necessary.

> 
> @@ +260,5 @@
> > +    if (curHandle.mPid == ourPid) {
> > +      // Our own process also has a handle to the section of interest. While we
> > +      // don't want our own pid, this *does* give us an opportunity to speed up
> > +      // future iterations by examining each handle for its kernel object (which
> > +      // is unique across all processes) instead of searching by name.
> 
> nit: "unique across all processes" is totally valid language, but it
> initially confused me; I kept reading it as "unique in all processes", which
> is very different. Consider changing to "the same for all processes" to be
> super clear.

Sure, I fixed the wording.

> 
> ::: accessible/windows/msaa/LazyInstantiator.cpp
> @@ +287,5 @@
> > +  if (uiaPid) {
> > +    return uiaPid.value();
> > +  }
> > +
> > +  return mscom::MainThreadRuntime::GetClientThreadId();
> 
> This returns a process id for UIA and a *thread* id for MSAA.
> ShouldInstantiate takes a thread id, not a process id.
> So, I think this will fail for UIA.
> But... UIA has already set the instantiator anyway in
> Compatibility::OnUIAMessage, so does it really need to set it in
> ShouldInstantiate as well?
> For that matter, will ShouldInstantiate even get called for UIA? Have we
> blocked UIA before that point anyway?
> If we do want ShouldInstantiate to get called for UIA, that also means we'd
> need to check the UIA block list in two places.

Yeah, this is kind of vestigial work from an earlier revision. Originally I was hoping that we could funnel UIA detection through the LazyInstantiator, but due to various timing concerns I could not make that work. I just removed this offending code for now.

Comment 9

[James Teh [:Jamie]]

Comment on attachment 8934667 [details] [diff] [review]
Part 2: Add UIA detection (r2)

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

Thanks! r+ with nit fixed.

::: accessible/windows/msaa/CompatibilityUIA.cpp
@@ +278,5 @@
> +
> +  a11y::SetInstantiator(remotePid.value());
> +
> +  /* This is where we could block UIA stuff
> +  nsCOMPtr<nsIFile> instantiator = a11y::SetInstantiator(remotePid.value());

nit: Needs to be updated now that SetInstantiator returns void.

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Add UIA detection (r3)

Patch with comment revision. Carrying forward r+.

I'm obsoleting part 3 in favour of landing that in a follow-up bug.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3606176ddf2fc9de85e6932af7380cb2142349cc
Bug 1419886: Part 1 - Add hooks for UIA detection to nsAppShell; r=jimm

https://hg.mozilla.org/integration/mozilla-inbound/rev/da4106e06989ab988a2ed112920aaad5aec0e670
Bug 1419886: Part 2 - Add UIA detection to a11y and centralize a11y instantiator telemetry under a11y::SetInstantiator function; r=Jamie

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/3606176ddf2f
https://hg.mozilla.org/mozilla-central/rev/da4106e06989