1182097 - Disable e10s for a11y users in FF41

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

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

We're not ready.

Comment 1

[Jim Mathies [:jimm]]

We're going to leave this in triage and work on these crasher bugs, we'll revisit this next tuesday.

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: disable ipc accessibility on the firefox 41 branch

IPC accessibility is not nearly ready for use and causes problems.  Its not
worth backporting all the patches that would be needed to fix the usability so
there's no real point in fixing the crashes it causes.  Instead just disable
the IPC code.

Comment 3

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

I'll see how triage goes today.

Comment 4

[Jim Mathies [:jimm]]

If we're going to do this, shouldn't we bring back the disable prompt instead? With this patch, what happens if the user turns e10s on and they are using a11y?

Comment 5

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

(In reply to Jim Mathies [:jimm] from comment #4)
> If we're going to do this, shouldn't we bring back the disable prompt
> instead?

Yes I think so.

> With this patch, what happens if the user turns e10s on and they
> are using a11y?

Accessible content would be broken.

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to David Bolter [:davidb] from comment #5)
> (In reply to Jim Mathies [:jimm] from comment #4)
> > If we're going to do this, shouldn't we bring back the disable prompt
> > instead?
> 
> Yes I think so.

In principal sure, I'm not sure its worth the bother though, its only aurora after all.

Comment 7

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

Trevor, Jim, to show the correct prompt is it as simple as having IsBlacklistedForE10S always return true?

Comment 8

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

Comment on attachment 8632228 [details] [diff] [review]
disable ipc accessibility on the firefox 41 branch

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

I think we need to decide on the prompt before landing this.

Comment 9

[Jim Mathies [:jimm]]

(In reply to David Bolter [:davidb] from comment #8)
> Comment on attachment 8632228 [details] [diff] [review]
> disable ipc accessibility on the firefox 41 branch
> 
> Review of attachment 8632228 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we need to decide on the prompt before landing this.

Yeah that looks like it would work.

Comment 10

[Trevor Saunders (:tbsaunde)]

Attachment: disable ipc accessibility on the firefox 41 branch

IPC accessibility is not nearly ready for use and causes problems.  Its not
worth backporting all the patches that would be needed to fix the usability so
there's no real point in fixing the crashes it causes.  Instead just disable
the IPC code.

Comment 11

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8636048 [details] [diff] [review]
disable ipc accessibility on the firefox 41 branch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Comment 12

[Trevor Saunders (:tbsaunde)]

forgot to fill out form first time.

> Approval Request Comment
> [Feature/regressing bug #]:
> [User impact if declined]:a11y crashes when e10s is enabled
> [Describe test coverage new/current, TreeHerder]:
> [Risks and why]:  low just disables code that only runs with e10s on
> [String/UUID change made/needed]: none

Comment 13

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

Trevor, while reviewing the aurora-uplift request, I noticed that the said patch has not been r+'d? Or has it?

Comment 14

[Trevor Saunders (:tbsaunde)]

(In reply to Ritu Kothari (:ritu) from comment #13)
> Trevor, while reviewing the aurora-uplift request, I noticed that the said
> patch has not been r+'d? Or has it?

it was in the previous attachment.

Comment 15

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

Comment on attachment 8636048 [details] [diff] [review]
disable ipc accessibility on the firefox 41 branch

Approving for uplift to Aurora. Seems to be a safe patch.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d2b1d8febf2c

Comment 17

[:Felipe Gomes (needinfo for replies!)]

With the forcing of accessibilityIsBlacklistedForE10S to true, this means that every user on Aurora will get a popup warning about e10s not being compatible with e10s, and will have e10s disabled as the default operation of that popup.

Is that part of the change really needed? That flag is used as "a11y is blocking e10s, so deactivate e10s", rather than "..., so deactivate a11y"

Comment 18

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

We should probably check GetAccService() before returning true.

Comment 19

[Jim Mathies [:jimm]]

Doesn't appear to be hurting our E10S_AUTOSTART telemetry numbers, fwiw. Definitely needs a fix though, this isn't an excuse to keep this.

Comment 20

[:Felipe Gomes (needinfo for replies!)]

(In reply to Jim Mathies [:jimm] from comment #19)
> Doesn't appear to be hurting our E10S_AUTOSTART telemetry numbers, fwiw.
> Definitely needs a fix though, this isn't an excuse to keep this.

This is because there hasn't been an Aurora update to users with this patch yet, aiui.

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Attachment: fix accessibilityIsBlacklistedForE10S flag

I think this is the simplest change from what dbolter suggested

Comment 22

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

Comment on attachment 8641249 [details] [diff] [review]
fix accessibilityIsBlacklistedForE10S flag

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

Yep thanks, I want Trevor to review.

Comment 23

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8641249 [details] [diff] [review]
fix accessibilityIsBlacklistedForE10S flag

yeah, I'm an idiot and read the comments here and didn't think :(

Comment 24

[:Felipe Gomes (needinfo for replies!)]

Ah don't worry. Trevor, quick question: this just landed on Aurora, right? (i.e. there was no m-c version in some other bug for example)

Comment 25

[Trevor Saunders (:tbsaunde)]

(In reply to :Felipe Gomes from comment #24)
> Ah don't worry. Trevor, quick question: this just landed on Aurora, right?
> (i.e. there was no m-c version in some other bug for example)

correct, it never landed on m-c

Comment 26

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8641249 [details] [diff] [review]
fix accessibilityIsBlacklistedForE10S flag

Approval Request Comment
[Feature/regressing bug #]: follow-up to the previous patch on this bug
[User impact if declined]: users on Aurora will see a warning saying that e10s will be disabled because of a11y
[Describe test coverage new/current, TreeHerder]: I don't think there are tests for this
[Risks and why]: this code is only used to warn if e10s should be force-disabled when a11y is enabled. Given that a11y is being disabled for e10s, it should not have any big impact.
[String/UUID change made/needed]: -

Comment 27

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

Felipe, could you please confirm (by manually testing on a local build) that after applying the second patch that e10s is disabled for accessibility users? And that e10s prompts for a opt-in at startup like before. 

In your aurora uplift request you did not mention whether you tested it manually. I would like to get a confirmation from you so we know that we have the right fix this time. Thanks!

Comment 28

[:Felipe Gomes (needinfo for replies!)]

I just tested manually by doing the following:

On Mac, I enabled VoiceOver and started an Aurora build with e10s previously enabled. The popup saying that a11y is incompatible with e10s showed up as expected and e10s was disabled after a restart.

This patch doesn't touch anything around the opt-in e10s prompt, but I double checked and it is appearing correctly.

Comment 29

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

Comment on attachment 8641249 [details] [diff] [review]
fix accessibilityIsBlacklistedForE10S flag

Approved based on confirmation from Felipe that this patch does what is intended.

Comment 30

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/39c6017f9e67