Closed Bug 1418535 Opened 2 years ago Closed 2 years ago

Investigate RealPlayer add-on use of accessibility services

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 + fixed
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: jimm, Assigned: aklotz)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(3 files)

Based on bug 1418290, comment 13, there are reports on twitter of RealPlayer consuming accessibility services, and potentially slowing down the browser.
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|
57 profile:

https://perfht.ml/2yWhFPt

Aaron, can you take a look? Wondering if there's something we can do to help here.
Flags: needinfo?(aklotz)
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.
Flags: needinfo?(aklotz)
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.
Flags: needinfo?(lhenry)
(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?
Flags: needinfo?(aklotz)
Attached file module list.txt
Should a11y users be disabling RealPlayer or RealPlayer users disable a11y?  Or both?
Flags: needinfo?(lhenry)
Keywords: access
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?
How do you feel about going with a whitelist on Windows until we fix perf?
(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?
Flags: needinfo?(aklotz) → needinfo?(yzenevich)
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?
Flags: needinfo?(yzenevich)
Flags: needinfo?(aklotz)
Taking. We're gonna try and block from within the LazyInstantiator.
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Flags: needinfo?(aklotz)
Please try with your RealPlayer install
Attachment #8930217 - Flags: review?(jmathies)
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
NI Adam Stevenson for partner awareness.
Flags: needinfo?(astevenson)
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?
Attachment #8930217 - Flags: review?(jmathies) → review+
Duplicate of this bug: 1418794
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
Connected with RealNetworks by email, added in Jim and David.
Flags: needinfo?(astevenson)
(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.
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
Another profile - 

https://perfht.ml/2zcviKs
Attached file log-Content-1100.log
mscom content process logs loading a google search page and scrolling a little. The parent log was 0 bytes.
Tested on beta, seems like there's a slight improvement but the browse is still heavily janked doing basic things.
Also tested with 'accessibility.uia.enable' set to true, it didn't seem to help.
https://hg.mozilla.org/mozilla-central/rev/1516807b05d3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(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...
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."
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 .
Just tested change from "on page load for 7 seconds' to "mouse over' , and as suspected there was NO CHANGE in performance .
(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 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.
Attachment #8930217 - Flags: approval-mozilla-beta?
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.
Attachment #8930217 - Flags: approval-mozilla-release?
Blocks: 1421010
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+
Attachment #8930217 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8930217 - Flags: approval-mozilla-release? → approval-mozilla-release+
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 .
RealNetworks/RealDownloader is gone again on 57 . Turning on accessibility still causes performance issues.
You need to log in before you can comment on or make changes to this bug.