1423999 - Improved UIA detection

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[(No longer employed by Mozilla) Aaron Klotz]

In https://bugzilla.mozilla.org/show_bug.cgi?id=1419886#c8 I speculated about being able to overcome the uiAccess permission by comparing the value of the kernel object.

A first cut of that patch ran in 15ms which is a new record.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: 2-pass approach using object pointer that eliminates handle duplication

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8935485 [details] [diff] [review]
2-pass approach using object pointer that eliminates handle duplication

Canceling review. More thought needs to go into this.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Detect all the things

This patch works and also fixes those PGO crashes.

In the comments I allude to a WOW64 quirk. More info can be found here: http://blog.rewolf.pl/blog/?p=1273

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Detect all the things (r2)

This one actually builds ;-)

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 7

[James Teh [:Jamie]]

Comment on attachment 8938072 [details] [diff] [review]
Detect all the things (r2)

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

Nice!

::: accessible/windows/msaa/CompatibilityUIA.cpp
@@ +97,5 @@
> +      return false;
> +    }
> +#else
> +    if (ntStatus == STATUS_BUFFER_TOO_SMALL) {
> +      // This case only occurs on 32-bit builds running atop WOW64.

1. I think it might be good to include a comment with the URL of the article about this which you linked in the bug. People could just look at the bug I guess, but because this is a bit obscure, it might be good to have the URL close at hand; I certainly don't think it does any harm.

2. How could this actually occur in our case? The article seems to suggest STATUS_BUFFER_TOO_SMALL only happens with WoW64 if the buffer was too small to fit even one structure:

		if ( !_number_of_32bit_structs )
		{
			WriteReturnLengthSilent(returnLength, _required_length);
			return STATUS_BUFFER_TOO_SMALL;
		}

In our case, we always start with a buffer big enough for 1024 structures, so we shouldn't hit this. Am I missing something?

@@ +236,2 @@
>        ULONG objTypeBufLen;
> +      ntStatus = NtQueryObject(handle, ObjectTypeInformation,

nit: Should this be :: prefixed (::NtQueryObject)? I'm not sure what the policy on this is, but I notice you do ::NtQuerySystemInformation earlier. Same question for other Nt* calls later.

::: widget/windows/nsAppShell.cpp
@@ +189,5 @@
>      Maybe<bool> shouldCallNextHook =
>        a11y::Compatibility::OnUIAMessage(cwp->wParam, cwp->lParam);
> +    if (shouldCallNextHook.isSome()) {
> +      // We've got an instantiator, disconnect this hook.
> +      if (::UnhookWindowsHookEx(gUiaHook)) {

1. Was this stuff meant to be included in this patch? I don't really mind, but it seems somewhat separate from the handle duplication elimination (and might need review from someone else as well).

2. We don't try to run the UIA detection once kMaxUiaAttempts has been reached. However, if I'm reading this correctly, we don't remove the hook in that case; we only remove the hook on a successful detection. Sure, it doesn't do much if we reached kMaxUiaAttempts, but should we still remove it anyway so we don't have a pointless hook hanging around?

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to James Teh [:Jamie] from comment #7)
> Comment on attachment 8938072 [details] [diff] [review]
> Detect all the things (r2)
> 
> Review of attachment 8938072 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice!
> 
> ::: accessible/windows/msaa/CompatibilityUIA.cpp
> @@ +97,5 @@
> > +      return false;
> > +    }
> > +#else
> > +    if (ntStatus == STATUS_BUFFER_TOO_SMALL) {
> > +      // This case only occurs on 32-bit builds running atop WOW64.
> 
> 1. I think it might be good to include a comment with the URL of the article
> about this which you linked in the bug. People could just look at the bug I
> guess, but because this is a bit obscure, it might be good to have the URL
> close at hand; I certainly don't think it does any harm.

As a point of style, I don't think we like to link directly to external websites in our code, however I did link to the comment in this bug.

> 
> 2. How could this actually occur in our case? The article seems to suggest
> STATUS_BUFFER_TOO_SMALL only happens with WoW64 if the buffer was too small
> to fit even one structure:
> 
> 		if ( !_number_of_32bit_structs )
> 		{
> 			WriteReturnLengthSilent(returnLength, _required_length);
> 			return STATUS_BUFFER_TOO_SMALL;
> 		}
> 
> In our case, we always start with a buffer big enough for 1024 structures,
> so we shouldn't hit this. Am I missing something?

My understanding is that the single structure case is the native case; under WOW64 it expects enough space for the entire list.

> 
> @@ +236,2 @@
> >        ULONG objTypeBufLen;
> > +      ntStatus = NtQueryObject(handle, ObjectTypeInformation,
> 
> nit: Should this be :: prefixed (::NtQueryObject)? I'm not sure what the
> policy on this is, but I notice you do ::NtQuerySystemInformation earlier.
> Same question for other Nt* calls later.

Ah, yes. This isn't a formal part of our style guide, it is more of a personal preference that I use. But yes, I should be consistent.

> 
> ::: widget/windows/nsAppShell.cpp
> @@ +189,5 @@
> >      Maybe<bool> shouldCallNextHook =
> >        a11y::Compatibility::OnUIAMessage(cwp->wParam, cwp->lParam);
> > +    if (shouldCallNextHook.isSome()) {
> > +      // We've got an instantiator, disconnect this hook.
> > +      if (::UnhookWindowsHookEx(gUiaHook)) {
> 
> 1. Was this stuff meant to be included in this patch? I don't really mind,
> but it seems somewhat separate from the handle duplication elimination (and
> might need review from someone else as well).

Yeah, I intentionally included this here. I felt that it was safe for you to review it. If not, I'm sure the Win32 widget owner (jimm) will let me know after the fact. ;-)

> 
> 2. We don't try to run the UIA detection once kMaxUiaAttempts has been
> reached. However, if I'm reading this correctly, we don't remove the hook in
> that case; we only remove the hook on a successful detection. Sure, it
> doesn't do much if we reached kMaxUiaAttempts, but should we still remove it
> anyway so we don't have a pointless hook hanging around?

Good point. I fixed that.

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Detect all the things (r3)

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Detect all the things (r4)

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/284f27f2bb610bbf416a4709b2afdf045f055a66
Bug 1423999: Improved UIA detection that eliminates handle duplication; r=Jamie

Comment 12

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/284f27f2bb61