1418535 - Investigate RealPlayer add-on use of accessibility services

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Jim Mathies [:jimm]]

Based on bug 1418290, comment 13, there are reports on twitter of RealPlayer consuming accessibility services, and potentially slowing down the browser.

Comment 1

[Jim Mathies [:jimm]]

This is pretty bad on low end systems. They inject into our running browser instance, instantiate a11y, and add a chrome widget on our tab bar. 

Performance of 57 with this running is really bad on single core core systems.

No client info unfortunately.

Activated 	true
Prevent Accessibility 	0
Accessible Handler Used 	true
Accessibility Instantiator 	UIAUTOMATION|

Comment 2

[Jim Mathies [:jimm]]

57 profile:

https://perfht.ml/2yWhFPt

Aaron, can you take a look? Wondering if there's something we can do to help here.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

This is UIA caching through and through!

We would either need to get aggressive with blocking UIA or somehow change our lazy instantiation code to be more conservative.

This is definitely the same stuff that is interfering with JAWS, through.

I think we'd need to discuss consequences of blocking UIA with davidb and team. I'm happy to do it (and even have a half-baked patch sitting around from last week), but obviously we don't want to overcorrect.

Comment 4

[Jim Mathies [:jimm]]

Hey Liz, I think it might be appropriate to add a rel note about this for 57. The note should suggest the user disable a11y to disable the "RealPlayer add-on" to improve performance.

Comment 5

[Jim Mathies [:jimm]]

(In reply to Aaron Klotz [:aklotz] from comment #3)
> This is UIA caching through and through!
> 
> We would either need to get aggressive with blocking UIA or somehow change
> our lazy instantiation code to be more conservative.

How about shutting a11y down if we detect the dll they inject? I see 'dtvhooks.dll' in the modules list which corresponds to the "RealDownloader".

Also, do you know of any change in 58 we might be able to uplift that would improve this?

Comment 6

[Jim Mathies [:jimm]]

Attachment: module list.txt

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Should a11y users be disabling RealPlayer or RealPlayer users disable a11y?  Or both?

Comment 8

[James Teh [:Jamie]]

Aaron, as far as I know, UIA subtree caching has to be explicitly enabled. Why would they be enabling subtree caching? I'm concerned we might be missing something here. Is UIA actually walking descendants here?

Comment 9

[alexander :surkov (:asurkov)]

How do you feel about going with a whitelist on Windows until we fix perf?

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Jim Mathies [:jimm] from comment #5)
> How about shutting a11y down if we detect the dll they inject? I see
> 'dtvhooks.dll' in the modules list which corresponds to the "RealDownloader".
> 
> Also, do you know of any change in 58 we might be able to uplift that would
> improve this?

Possibly. We'd need to write a bit of new code here for the DLL detection side.

Yura, would we need to uplift anything from bug 1401980 for this to work correctly?

Comment 11

[Yura Zenevich [:yzen] (please use "needinfo")]

If we want to disable a11y with a pref then yes, a patch from the bug with possible 1 follow up that i m working on right now. And also, Aaron, you flagged a good point about cleaning everything up on shutdown. Would that be critical?

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

Taking. We're gonna try and block from within the LazyInstantiator.

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Block known bad dlls when they're the only ones present

Please try with your RealPlayer install

Comment 14

[[:philipp]]

please note that the realplayer hook can also come in the form of "dtvhooks64.dll" - a sample is in the crash reports linked from https://support.mozilla.org/questions/1186424

Comment 15

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

NI Adam Stevenson for partner awareness.

Comment 16

[Jim Mathies [:jimm]]

Comment on attachment 8930217 [details] [diff] [review]
Block known bad dlls when they're the only ones present

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

::: accessible/windows/msaa/LazyInstantiator.cpp
@@ +294,2 @@
>    /*
>    if (ClientShouldBeBlocked(clientExe)) {

Don't we need another call to IsBlockedInjection() in here somewhere?

Comment 18

[Jim Mathies [:jimm]]

Comment on attachment 8930217 [details] [diff] [review]
Block known bad dlls when they're the only ones present

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

::: accessible/windows/msaa/LazyInstantiator.cpp
@@ +234,5 @@
>  /**
> + * This is the blocklist for known "bad" DLLs that instantiate a11y.
> + */
> +static const wchar_t* gBlockedInprocDlls[] = {
> +  L"dtvhooks.dll"   // RealPlayer, bug 1418535

we need the 64 bit version too - dtvhooks64.dll

Comment 19

[Adam Stevenson [:adamopenweb]]

Connected with RealNetworks by email, added in Jim and David.

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Jim Mathies [:jimm] from comment #16)
> Comment on attachment 8930217 [details] [diff] [review]
> Block known bad dlls when they're the only ones present
> 
> Review of attachment 8930217 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/windows/msaa/LazyInstantiator.cpp
> @@ +294,2 @@
> >    /*
> >    if (ClientShouldBeBlocked(clientExe)) {
> 
> Don't we need another call to IsBlockedInjection() in here somewhere?

* Since UIA is being used, the external thread id will be 0.
* If I added it here, it would block any and all further a11y requests from other external programs using MSAA once that DLL was in place.

(In reply to Jim Mathies [:jimm] from comment #18)
> Comment on attachment 8930217 [details] [diff] [review]
> Block known bad dlls when they're the only ones present
> 
> Review of attachment 8930217 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/windows/msaa/LazyInstantiator.cpp
> @@ +234,5 @@
> >  /**
> > + * This is the blocklist for known "bad" DLLs that instantiate a11y.
> > + */
> > +static const wchar_t* gBlockedInprocDlls[] = {
> > +  L"dtvhooks.dll"   // RealPlayer, bug 1418535
> 
> we need the 64 bit version too - dtvhooks64.dll

Will add.

Comment 21

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1516807b05d3f6084b58d54b12032ccce5cbdcfc
Bug 1418535: Block a11y instntiation if no known ATs are present and known bad DLLs are; r=jimm

Comment 22

[Jim Mathies [:jimm]]

Another profile - 

https://perfht.ml/2zcviKs

Comment 23

[Jim Mathies [:jimm]]

Attachment: log-Content-1100.log

mscom content process logs loading a google search page and scrolling a little. The parent log was 0 bytes.

Comment 24

[Jim Mathies [:jimm]]

Tested on beta, seems like there's a slight improvement but the browse is still heavily janked doing basic things.

Comment 25

[Jim Mathies [:jimm]]

Also tested with 'accessibility.uia.enable' set to true, it didn't seem to help.

Comment 26

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/1516807b05d3

Comment 27

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Jim Mathies [:jimm] from comment #24)
> Tested on beta, seems like there's a slight improvement but the browse is
> still heavily janked doing basic things.

Got a new profile? Maybe the fix isn't working...

Comment 28

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

from https://support.mozilla.org/en-US/questions/1185287#answer-1031548
"I had the same problem. I have Realplayer installed and I suspected it may be the problem. So I checked its settings. It was set to "on page load 7 seconds". I changed it to" mouse over" and it seems to have corrected the problem. I'll be setting Firefox to default browser and if it's fixed I'll be back her to confirm."

Comment 29

[DDM]

I don't believe changing from "on page load for 7 seconds" to "mouse over " will have any effect . It pertains to the "Down Load This" window for Internet Explorer browser .

Comment 30

[DDM]

Just tested change from "on page load for 7 seconds' to "mouse over' , and as suspected there was NO CHANGE in performance .

Comment 31

[Jim Mathies [:jimm]]

(In reply to Aaron Klotz [:aklotz] from comment #27)
> (In reply to Jim Mathies [:jimm] from comment #24)
> > Tested on beta, seems like there's a slight improvement but the browse is
> > still heavily janked doing basic things.
> 
> Got a new profile? Maybe the fix isn't working...

Which fix? I don' think we have anything up on beta right now aside from any general a11y perf fixes from the team.

Comment 32

[Jim Mathies [:jimm]]

Comment on attachment 8930217 [details] [diff] [review]
Block known bad dlls when they're the only ones present

Approval Request Comment
[Feature/Bug causing the regression]:
57 accessibility support
[User impact if declined]:
janky browser with RealNetworks free RealPLayer injected into browser browser space.
[Is this code covered by automated tests?]:
no.
[Has the fix been verified in Nightly?]:
yes.
[Needs manual test from QE? If yes, steps to reproduce]: 
no, a11y team will test on beta once uplifted.
[List of other uplifts needed for the feature/fix]:
none.
[Is the change risky?]:
no.
[Why is the change risky/not risky?]:
well understood change by the author.
[String changes made/needed]:
none.

Comment 33

[Jim Mathies [:jimm]]

Comment on attachment 8930217 [details] [diff] [review]
Block known bad dlls when they're the only ones present

Approval Request Comment
[Feature/Bug causing the regression]:
57 accessibility support
[User impact if declined]:
janky browser with RealNetworks free RealPLayer injected into browser browser space.
[Is this code covered by automated tests?]:
no.
[Has the fix been verified in Nightly?]:
yes.
[Needs manual test from QE? If yes, steps to reproduce]: 
no, a11y team will test on beta once uplifted.
[List of other uplifts needed for the feature/fix]:
none.
[Is the change risky?]:
no.
[Why is the change risky/not risky?]:
well understood change by the author.
[String changes made/needed]:
none.

Comment 34

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8930217 [details] [diff] [review]
Block known bad dlls when they're the only ones present

Jimm suggested we include this as a ride-along in 57.0.1 due to the a11y perf issues, Release57+

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0b758da65a95

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/86498cd76359

Comment 37

[DDM]

Good news , RealNetworks / RealDownloader is back on in 57 with no issues . Accessibility is still set to off .
Unknown if this is update to 57 , or update to RealNetworks/RealDownloader .

Comment 38

[DDM]

RealNetworks/RealDownloader is gone again on 57 . Turning on accessibility still causes performance issues.