Disable e10s for a11y users in FF41

RESOLVED FIXED in Firefox 41

Status

()

Core
Disability Access APIs
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: davidb, Assigned: tbsaunde)

Tracking

unspecified
mozilla41
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(e10s+, firefox41 fixed, firefox42 wontfix)

Details

Attachments

(2 attachments, 1 obsolete attachment)

We're not ready.

Updated

3 years ago
tracking-e10s: --- → ?
(Assignee)

Updated

3 years ago
QA Contact: tbsaunde+mozbugs

Comment 1

3 years ago
We're going to leave this in triage and work on these crasher bugs, we'll revisit this next tuesday.
(Assignee)

Comment 2

3 years ago
Created attachment 8632228 [details] [diff] [review]
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.
Attachment #8632228 - Flags: review?(dbolter)
I'll see how triage goes today.
tracking-e10s: ? → +

Comment 4

3 years ago
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?
(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.
(Assignee)

Comment 6

3 years ago
(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.
Trevor, Jim, to show the correct prompt is it as simple as having IsBlacklistedForE10S always return true?
Flags: needinfo?(jmathies)
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.
Attachment #8632228 - Flags: review?(dbolter) → review+

Comment 9

3 years ago
(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.
Flags: needinfo?(jmathies)

Updated

3 years ago
Assignee: nobody → tbsaunde+mozbugs
(Assignee)

Comment 10

3 years ago
Created attachment 8636048 [details] [diff] [review]
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.
Attachment #8632228 - Attachment is obsolete: true
(Assignee)

Comment 11

3 years ago
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]:
Attachment #8636048 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 12

3 years ago
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
Trevor, while reviewing the aurora-uplift request, I noticed that the said patch has not been r+'d? Or has it?
Flags: needinfo?(tbsaunde+mozbugs)
(Assignee)

Comment 14

3 years ago
(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.
Flags: needinfo?(tbsaunde+mozbugs)
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.
Attachment #8636048 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox41: --- → affected
status-firefox42: --- → wontfix

Updated

3 years ago
Blocks: 1188605
https://hg.mozilla.org/releases/mozilla-aurora/rev/d2b1d8febf2c
status-firefox41: affected → fixed
Flags: in-testsuite-
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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"
Flags: needinfo?(tbsaunde+mozbugs)
We should probably check GetAccService() before returning true.

Comment 19

3 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Created attachment 8641249 [details] [diff] [review]
fix accessibilityIsBlacklistedForE10S flag

I think this is the simplest change from what dbolter suggested
Attachment #8641249 - Flags: review?(dbolter)
Comment on attachment 8641249 [details] [diff] [review]
fix accessibilityIsBlacklistedForE10S flag

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

Yep thanks, I want Trevor to review.
Attachment #8641249 - Flags: review?(tbsaunde+mozbugs)
Attachment #8641249 - Flags: review?(dbolter)
Attachment #8641249 - Flags: feedback+
(Assignee)

Comment 23

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

yeah, I'm an idiot and read the comments here and didn't think :(
Flags: needinfo?(tbsaunde+mozbugs)
Attachment #8641249 - Flags: review?(tbsaunde+mozbugs) → review+
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)
(Assignee)

Comment 25

3 years ago
(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 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]: -
Attachment #8641249 - Flags: approval-mozilla-aurora?
status-firefox41: fixed → affected
Depends on: 1189732
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!
Flags: needinfo?(felipc)
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.
Flags: needinfo?(felipc)
Comment on attachment 8641249 [details] [diff] [review]
fix accessibilityIsBlacklistedForE10S flag

Approved based on confirmation from Felipe that this patch does what is intended.
Attachment #8641249 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/39c6017f9e67
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.