1420276 - Investigate blocking UIA

Status: RESOLVED WONTFIX

(Core :: Disability Access APIs, defect, P3)

Description

[(No longer employed by Mozilla) Aaron Klotz]

UIA is being injected by user32!_ClientLoadLibrary, which tells me that it's coming in via a window hook set on our parent main thread.

The simplest way to block this is probably to just add it to our DLL blocklist machinery.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 2

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

Marco can you give the try build in comment 1 a spin? (I will as well)

Comment 3

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

I tested Microsoft Narrator and Magnifier, these are some findings:

57 Release (20171112125346) + Magnifier
Accessibility
Activated 	true
Prevent Accessibility 	0
Accessible Handler Used 	true
Accessibility Instantiator 	UIAUTOMATION|

57 Release (20171112125346) + Narrator
Accessibility
Activated 	true
Prevent Accessibility 	0
Accessible Handler Used 	true
Accessibility Instantiator 	UIAUTOMATION|

Try build + Magnifier
Activated 	true
Prevent Accessibility 	0
Accessible Handler Used 	false
Accessibility Instantiator 	UNKNOWN|

Try build + Narrator:
Accessibility
Activated 	true
Prevent Accessibility 	0
Accessible Handler Used 	false
Accessibility Instantiator 	UNKNOWN|

Good news: so far I couldn't find any way that either tools was broken via UIA blocking.

I set magnifier to follow the keyboard focus but that was similarly broken in all scenarios. I also tested on Nightly (without UIA blocking) and the main difference was that it seemed more performant (I never got a 'wait' in Narrator).

Comment 4

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

Adding a few more folks in case they can help test soon -- please.

Comment 5

[:Gijs (he/him)]

I wonder if this will break touch stuff on win10? IIRC that uses UIA for some things? Maybe I'm confused. I'll try to test on my tablet later tonight.

Comment 6

[:Gijs (he/him)]

The test build seems to work fine on my surface with touch keyboard input.

Comment 7

[James Teh [:Jamie]]

(In reply to David Bolter [:davidb] (NeedInfo me for attention) from comment #3)
> I tested Microsoft Narrator and Magnifier, these are some findings:
> Try build + Narrator:
> Accessibility Instantiator 	UNKNOWN|

Odd that accessibility gets enabled, but the instantiator doesn't show an out-proc executable. I confirmed this locally too.

> I also tested on Nightly (without UIA blocking) and the
> main difference was that it seemed more performant (I never got a 'wait' in
> Narrator).

To clarify:
1. I assume you mean the try build was more performant?
2. What do you mean by a 'wait' in Narrator? I don't seem to see this even in Nightly.

My findings:
1. It seems that if UIAutomationCore is blocked, it falls back to out-proc MSAA. However, the out-proc executable name isn't picked up as the instantiator for Narrator (and Magnifier as well according to David's testing), which is odd.
2. My UIA tree walking/caching tests with the NVDA Python console (as outlined in bug 1419992) do work, so it seems UIA still provides this functionality even when it has to run out-proc. They are slightly faster with UIA blocked! The difference isn't hugely significant, maybe a second or so in each case. My guess is that this is because it doesn't bother doing a QI for IAccessibleEx, since AFAIK raw UIA interfaces can't be used out-proc.

If David is indeed seeing better perf with UIA blocked, my guess is that this is largely related to the Firefox parent process. Tree walking/caching is still going to be insanely slow (see my second finding above). However, with UIA in Firefox's parent process, that blocks the parent process, meaning the UI is blocked. In contrast, with UIA blocked, those calls are going to go straight to the MTA in the content process. That means the UI is not blocked (so scrolling and the like will be much less affected), but it will still have the same impact on content.

I think we really need to try this UIA blocking with a confirmed "problematic" test case such as RealPlayer. Unfortunately, I believe RealPlayer doesn't run against Nightly, so we'd need to fool it into thinking it's running against Firefox stable somehow.

Comment 8

[James Teh [:Jamie]]

(In reply to James Teh [:Jamie] from comment #7)
> Odd that accessibility gets enabled, but the instantiator doesn't show an
> out-proc executable.

IMessageFilter::HandleIncomingCall gets called, but we get 0 for aCallerTid. I don't see any mention of this in the documentation for IMessageFilter. However, CoGetCallerTID returns 0 if the calling thread is an MTA. I'm thinking the same applies to IMessageFilter. If that's the case, this means we can't get out-proc instantiators that call us from an MTA.

If I'm correct, this also makes me wonder whether we should just use CoGetCallerTID instead of IMessageFilter, unless IMessageFilter offers us something else.

Comment 9

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

(In reply to James Teh [:Jamie] from comment #7)
> (In reply to David Bolter [:davidb] (NeedInfo me for attention) from comment

> To clarify:
> 1. I assume you mean the try build was more performant?

Yes.

> 2. What do you mean by a 'wait' in Narrator? I don't seem to see this even
> in Nightly.

I managed to get Narrator to speak "wait" or something similar, with heavy pages. That didn't happen with the try build. I didn't test a lot though.

Comment 10

[Jim Mathies [:jimm]]

Aaron, can you comment on comment 7 and comment 8?

Can someone take JAWS 2018 for a spin with this try build?

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

It could very well be that UIA outproc goes through an MTA. I too had speculated about whether IMessageFilter works the same way as CoGetCallerTID but of course documentation was scarce.

We could replace IMessageFilter with CoGetCallerTID, though that's low priority, IMO. Though bug 1413009, if implemented, would still require IMessageFilter.


I'm not sure what we can do about detecting the instantiator when it's outproc UIA; that would require some more investigation. OTOH it's better to have UIA outproc than inproc as far as using Firefox is concerned.

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

I tried JAWS out with this build; it's still pretty slow. Blocking inproc UIA is not a panacea (but I never expected it to be), however it removes another source of problems.

Comment 13

[James Teh [:Jamie]]

(In reply to Aaron Klotz [:aklotz] from comment #11)
> We could replace IMessageFilter with CoGetCallerTID, though that's low
> priority, IMO.

Of course. That was more of a side note than a point of concern. Sorry for being unclear. :)

> OTOH it's better to
> have UIA outproc than inproc as far as using Firefox is concerned.

Perhaps. I still wonder whether my ideas in bug 1419992 might make UIA much faster in-proc, in which case we would be blocking content less. However, that will certainly take longer to implement... and whether testing that theory is a good use of my time is an open question.

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch

We only block on Dev Edition, Beta and Release. I'm leaving this off on local/Nightly builds so that we can continue to improve our UIA detection and blocking.

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8932165 [details] [diff] [review]
Patch

Redirecting to Jamie.

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to James Teh [:Jamie] from comment #13)
> Perhaps. I still wonder whether my ideas in bug 1419992 might make UIA much
> faster in-proc, in which case we would be blocking content less. However,
> that will certainly take longer to implement... and whether testing that
> theory is a good use of my time is an open question.

Oh yeah, we should totally see what we can do to improve this. That was more a comment on the current state of things than anything. :-)

Comment 17

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/244c97bc16e3f6b394c0915a4f7b62f97176e4d7
Bug 1420276: Add uiautomationcore.dll to blocklist; r=Jamie

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8932165 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Windows a11y+e10s
[User impact if declined]: This is currently one of the most commonly reported perf issues on 57 release. Heavy Jank when the user's computer is running third-party software that tries to interact with Firefox using Microsoft UI Automation. Note that the third-party software that causes this does *not* need to be an assistive technology to cause problems with Firefox. eg RealPlayer is a known cause.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: We have verified it using the try build in this bug. We have not verified it on Nightly because the fix is being intentionally disabled on Nightly so that we can continue to improve the performance issues.
[Needs manual test from QE? If yes, steps to reproduce]: No, the a11y team has done the manual tests already.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low.
[Why is the change risky/not risky?]: Simple DLL block. We have manually tested this with the relevant third-party software and the results have been positive.
[String changes made/needed]: None

Comment 19

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

Jim have you confirmed this helps the UIA top 15? Or was it only realplayer where you've been able to recreate the slowness?

Comment 20

[Marco Zehe (:MarcoZ)]

(In reply to James Teh [:Jamie] from comment #13)
> > OTOH it's better to
> > have UIA outproc than inproc as far as using Firefox is concerned.
> 
> Perhaps. I still wonder whether my ideas in bug 1419992 might make UIA much
> faster in-proc, in which case we would be blocking content less. However,
> that will certainly take longer to implement... and whether testing that
> theory is a good use of my time is an open question.

Based on my own testing of JAWS with that try build, we should definitely investigate this route. I basically don't see much of a difference with this blocking UIA stuff. It may *look* better for some users, since scrolling, which is done in the parent, is no longer affected, but as soon as interaction with content happens, that bug still hits. I'm definitely with Jamie on this question.

Comment 21

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/244c97bc16e3

Comment 22

[Jim Mathies [:jimm]]

Comment on attachment 8932165 [details] [diff] [review]
Patch

Want to chat with david about this uplift this morning. putting this on hold for now.

Comment 23

[Jim Mathies [:jimm]]

(In reply to David Bolter [:davidb] (NeedInfo me for attention) from comment #19)
> Jim have you confirmed this helps the UIA top 15? Or was it only realplayer
> where you've been able to recreate the slowness?

No not yet. I am trying to get a try build with official branding and real player blocking patch removed to test against real player in a vm. I probably won't have that for another day or so.

Comment 24

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #23)
> (In reply to David Bolter [:davidb] (NeedInfo me for attention) from comment
> #19)
> > Jim have you confirmed this helps the UIA top 15? Or was it only realplayer
> > where you've been able to recreate the slowness?
> 
> No not yet. I am trying to get a try build with official branding and real
> player blocking patch removed to test against real player in a vm. I
> probably won't have that for another day or so.

If we block uiautomationcore.dll and run realplayer we still get instantiated by:
 UNKNOWN|C:\Program Files (x86)\Real\RealDownloader\downloader2.exe
and performance is better. So I think UIA is definitely our current problem area.

Comment 25

[Julien Cristau [:jcristau]]

Was there a decision here re 58?

Comment 26

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7324518f32644f799a1d89dfc2c8e2a2d165802f
Bug 1420276: Backed out changeset 244c97bc16e3 since we don't want to ship this yet; r=backout

Comment 27

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/7324518f3264

Comment 28

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:aklotz, maybe it's time to close this bug?

Comment 29

[James Teh [:Jamie]]

This patch was landed but then backed out, so it was never "fixed" as such.

We are looking at a native UIA implementation in future, so a block for this would need to be removed at that point. Since RealPlayer fixed their implementation, we no longer have any specific UIA clients which we know to be causing severe performance problems. On the other hand, we do still see crashes which are very probably related to UIA; see bug 1461237, bug 1507102 and bug 1458238. We should consider whether we want to block this on releases until we do have a native implementation. (it's probable that the crashes are related to our use of cross-process COM for accessibility in remote documents.)

A few things would need to be checked:
1. We are pretty sure that UIA falls back to out-proc MSAA on Windows 10 if it can't load in-proc. This needs to be double checked.
2. Does it do this on Windows 7?
3. As per comment 8, we would lose telemetry for at least some (maybe all) UIA clients. Is this something we can live with in the near-term?

Comment 30

[James Teh [:Jamie]]

Now that Cache the World has shipped (thus unblocking a proper UIA implementation and fixing UIA related stability issues), going forward, we will want to support UIA better, not block it.