Closed Bug 1419886 Opened 2 years ago Closed 2 years ago

Find a way to determine UIA client instantiator

Categories

(Core :: Disability Access APIs, enhancement)

Unspecified
Windows
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: aklotz, Assigned: aklotz)

References

Details

(Whiteboard: aes+)

Attachments

(2 files, 4 obsolete files)

No description provided.
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.
Depends on: 1422394
Attachment #8934361 - Flags: review?(jmathies)
Attached patch Part 2: Add UIA detection (obsolete) — Splinter Review
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.
Attachment #8934364 - Flags: review?(jteh)
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.
Attachment #8934365 - Flags: review?(rweiss)
Attachment #8934365 - Flags: review?(jteh)
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.
Attachment #8934365 - Flags: review?(jteh) → review+
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.
Attachment #8934361 - Flags: review?(jmathies) → review+
Addressed comment, carrying forward r+
Attachment #8934361 - Attachment is obsolete: true
Attachment #8934663 - Flags: review+
Attached patch Part 2: Add UIA detection (r2) (obsolete) — Splinter Review
(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.
Attachment #8934364 - Attachment is obsolete: true
Attachment #8934364 - Flags: review?(jteh)
Attachment #8934667 - Flags: review?(jteh)
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.
Attachment #8934667 - Flags: review?(jteh) → review+
Patch with comment revision. Carrying forward r+.

I'm obsoleting part 3 in favour of landing that in a follow-up bug.
Attachment #8934365 - Attachment is obsolete: true
Attachment #8934667 - Attachment is obsolete: true
Attachment #8934365 - Flags: review?(rweiss)
Attachment #8935251 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/3606176ddf2f
https://hg.mozilla.org/mozilla-central/rev/da4106e06989
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Blocks: 1423989
Blocks: 1423999
Blocks: 1424317
Depends on: 1433551
Depends on: 1446280
You need to log in before you can comment on or make changes to this bug.