Closed Bug 1289473 Opened 3 years ago Closed 3 years ago

Accessibility+e10s restart prompt sometimes doesn't display

Categories

(Toolkit :: Notifications and Alerts, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox47 --- unaffected
firefox48 --- wontfix
firefox49 - verified
firefox50 --- verified

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(1 file, 2 obsolete files)

Darnit, I introduced this when I did a little code reorg in bug 1198459 (dd880f72d470).

If there's a client running, getting the prompt is pretty random. Sometimes it works, sometimes it doesn't. Totally depends on how quickly the client requests a11y interfaces from us and how quickly we create the first browser window.

Working on a patch that should make this reliable.
Assignee: nobody → jmathies
Attached patch patch (obsolete) — Splinter Review
This patch makes a few changes - 

1) Have E10SAccessibilityCheck start listening for the a11y observer events earlier.
2) If E10SAccessibilityCheck doesn't have a window to prompt with, it sets a flag indicating it needs to in the future.
3) Add a new callback on E10SAccessibilityCheck for window opened which checks the prompt flag and prompts if necessary.
Attachment #8774842 - Flags: review?(felipc)
Comment on attachment 8774842 [details] [diff] [review]
patch

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

::: browser/components/nsBrowserGlue.js
@@ +3027,5 @@
>  
>    _showE10sAccessibilityWarning: function() {
>      // We don't prompt about a11y incompat if e10s is off.
> +    if (!Services.appinfo.browserTabsRemoteAutostart &&
> +        !this._wantsPrompt) {

why this extra check here? in which case would e10s be off but we still want to prompt users?
(In reply to :Felipe Gomes (needinfo me!) from comment #2)
> Comment on attachment 8774842 [details] [diff] [review]
> patch
> 
> Review of attachment 8774842 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/nsBrowserGlue.js
> @@ +3027,5 @@
> >  
> >    _showE10sAccessibilityWarning: function() {
> >      // We don't prompt about a11y incompat if e10s is off.
> > +    if (!Services.appinfo.browserTabsRemoteAutostart &&
> > +        !this._wantsPrompt) {
> 
> why this extra check here? in which case would e10s be off but we still want
> to prompt users?

If we get the a11y observer notification before a window is created, we will try to prompt again in the onWindowRestored handler. At that point Services.appinfo.browserTabsRemoteAutostart returns false due to the accessibility pref that gets set.

To get into a state where _wantsPrompt is true, browserTabsRemoteAutostart has to be true at some earlier point since we set it down below this check. If browserTabsRemoteAutostart has always been false (e10s disabled from start) _wantsPrompt will also be false.
Attached patch patch v.2 (obsolete) — Splinter Review
removing that second _wantsPrompt check since browserTabsRemoteAutostart is cached from the first call.
Attachment #8774842 - Attachment is obsolete: true
Attachment #8774842 - Flags: review?(felipc)
Attachment #8774917 - Flags: review?(felipc)
Comment on attachment 8774917 [details] [diff] [review]
patch v.2

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

nit: please change the new function name from onWindowRestored to onWindowsRestored
Attachment #8774917 - Flags: review?(felipc) → review+
Attached patch patch v.3Splinter Review
updated per comments.
Attachment #8774917 - Attachment is obsolete: true
Attachment #8775144 - Flags: review+
[Tracking Requested - why for this release]:
get this uplifted to 49 once it's baked a bit on nightly.
Component: General → Notifications and Alerts
Product: Firefox → Toolkit
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5172702dcd32
Ensure e10s+accessibility notice displays for a11y clients that constantly run. r=felipe
Keywords: checkin-needed
Pushed by jmathies@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/855b04e6a374
Accessibility+e10s prompt - follow up to add missing review nit changes. r=felipe
(In reply to Pulsebot from comment #9)
> Pushed by jmathies@mozilla.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/855b04e6a374
> Accessibility+e10s prompt - follow up to add missing review nit changes.
> r=felipe

^ review nits, missed a qref.
https://hg.mozilla.org/mozilla-central/rev/5172702dcd32
https://hg.mozilla.org/mozilla-central/rev/855b04e6a374
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment on attachment 8775144 [details] [diff] [review]
patch v.3

Approval Request Comment
[Feature/regressing bug #]:
bug 1198459
[User impact if declined]:
flaky e10s+accessibility prompt
[Describe test coverage new/current, TreeHerder]:
On nightly and aurora for a while
[Risks and why]: 
low, no bugs reported
[String/UUID change made/needed]:
none
Attachment #8775144 - Flags: approval-mozilla-beta?
Comment on attachment 8775144 [details] [diff] [review]
patch v.3

On  m-c without problems, sounds like a good idea for a11y users. This should make it into beta 4 next week.
Attachment #8775144 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Confirming this fix using a Microsoft Surface Pro 2 device running latest Windows 10 x64 (Anniversary release).

Testing was performed using latest 50.0a2 Aurora (build ID 20160906004000) and Fx 49.0 RC (build ID 20160905130425).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: cornel.ionce
You need to log in before you can comment on or make changes to this bug.