1437417 - Unblock newer versions of RealPlayer with accessibility performance fixes

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[James Teh [:Jamie] (away returning 14 Apr)]

RealPlayer's use of accessibility was blocked in bug 1418535 due to severe performance problems caused by its use of accessibility. It was frequently using UIA to walk the entire tree to find the address bar. We've been working with the RealNetworks engineers and they've now switched to using NAVRELATION_EMBEDS and accValue to retrieve the address of the page. While it still uses accessibility, it should no longer cause performance problems. As per their request, we will unblock newer versions of RealPlayer with this change implemented.

Note that while RealNetworks confirm the performance problems are gone with this change, we haven't yet confirmed this ourselves.

The version with the fix will be 18.1.11.xxxx. The last version without the fix was 18.1.10.xxxx.

Comment 1

[James Teh [:Jamie] (away returning 14 Apr)]

Jimm confirmed that the upcoming version of RealPlayer does not cause performance problems.

Comment 2

[James Teh [:Jamie] (away returning 14 Apr)]

Attachment: Bug 1437417 part 1: a11y: Fix some issues in IsModuleVersionLessThan and make it reusable.

1. Move IsModuleVersionLessThan into the Compatibility class and export it in the header file.

2. The function previously referred to the third component of the version as the minor version; i.e. it was testing major.bbbb.minor.dddd.
This is incorrect and might confuse people using this in future code.
The minor version is the second component; i.e. major.minor.cccc.dddd.
cccc and dddd are often named build and revision, but the naming here is less consistent.

3. Rather than accepting separate version components, the function now accepts a single 64 bit value.
This makes comparison easier and also allows for comparison against magic values in other code; e.g. a value meaning "all versions".
This value can be created from separate components using the MAKE_FILE_VERSION macro.

4. Previously, it was assumed that a dll path could not be longer than MAX_PATH, but it can actually be longer.
The function now handles this.

5. The function previously didn't do any error checking, which could have led to null pointer dereferences and possibly other pain.
This was fine when it was only being used for JAWS, which we know always has version info, but this could be problematic for other callers.
We return true if there is a failure, assuming that no version info implies an earlier version.

6. The code now uses smart pointers instead of raw pointers, making memory management simpler.

7. Updated the JAWS version check accordingly.

Review commit: https://reviewboard.mozilla.org/r/219774/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219774/

Comment 3

[James Teh [:Jamie] (away returning 14 Apr)]

Attachment: Bug 1437417 part 2: Don't block accessibility instantiation for newer RealPlayer builds with accessibility performance fixes.

RealPlayer's use of accessibility was blocked in bug 1418535 due to severe performance problems caused by its use of accessibility.
This is fixed in newer builds, so we want to allow those while still blocking older builds.

Review commit: https://reviewboard.mozilla.org/r/219776/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219776/

Comment 4

[James Teh [:Jamie] (away returning 14 Apr)]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39a7c5be0863

Comment 5

[Marco Zehe (:MarcoZ)]

Comment on attachment 8950494 [details]
Bug 1437417 part 1: a11y: Fix some issues in IsModuleVersionLessThan and make it reusable.

https://reviewboard.mozilla.org/r/219774/#review225474

This is cool, thanks!

Comment 6

[Marco Zehe (:MarcoZ)]

Comment on attachment 8950495 [details]
Bug 1437417 part 2: Don't block accessibility instantiation for newer RealPlayer builds with accessibility performance fixes.

https://reviewboard.mozilla.org/r/219776/#review225480

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/799a9c8323fe
part 1: a11y: Fix some issues in IsModuleVersionLessThan and make it reusable. r=MarcoZ
https://hg.mozilla.org/integration/autoland/rev/ec4ad0ab47e9
part 2: Don't block accessibility instantiation for newer RealPlayer builds with accessibility performance fixes. r=MarcoZ

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/799a9c8323fe
https://hg.mozilla.org/mozilla-central/rev/ec4ad0ab47e9

Comment 9

[Jim Mathies [:jimm]]

Hey Jamie, lets get these two patches and any dependents marked for request for uplift to 59. We can try to get these into a point release within the next few weeks.

Comment 10

[Jim Mathies [:jimm]]

Testing with old 57 Firefox and the latest Firefox beta:

1) old version of RealPlayer still experiencing perf problems in 57
2) old version of RealPlayer perf improves with 58 and the block in place.
3) Testing with latest beta and ther old RealPlayer, performance isn't great but it's on par with 58 and the blocking on.
4) Testing with an updated RealPlayer and beta, performance of the browser is in much better shape.

So generally confirming fixes on both sides. If we have the opportunity to ship these in a point release lets do it. Otherwise we'll roll these changes out in 60 on 05-07.

Comment 11

[James Teh [:Jamie] (away returning 14 Apr)]

Comment on attachment 8950494 [details]
Bug 1437417 part 1: a11y: Fix some issues in IsModuleVersionLessThan and make it reusable.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1418535.
[User impact if declined]: Users cannot use newer versions of RealPlayer with performance fixes related to Firefox.
[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.
[List of other uplifts needed for the feature/fix]: Other patch from this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Only updates dll version check code used by accessibility. Has been on nightly for weeks and beta for a week without any issues.
[String changes made/needed]: None.

Comment 12

[James Teh [:Jamie] (away returning 14 Apr)]

Comment on attachment 8950495 [details]
Bug 1437417 part 2: Don't block accessibility instantiation for newer RealPlayer builds with accessibility performance fixes.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1418535.
[User impact if declined]: Users cannot use newer versions of RealPlayer with performance fixes related to Firefox.
[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.
[List of other uplifts needed for the feature/fix]: Other patch from this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Just updates dll block code to allow newer versions of RealPlayer. Has been on nightly for weeks and beta for a week without any issues.
[String changes made/needed]: None.

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 8950494 [details]
Bug 1437417 part 1: a11y: Fix some issues in IsModuleVersionLessThan and make it reusable.

60 is on release now, too late for 59.