This is a temporary solution, but we'll need to do it if e10s a11y isn't ready by the time we want to enable e10s on Nightly.

How do we plan to detect this?

:tbsaunde who pointed to NS_GetAccessibilityService i.e. suggested that we rephrase this bug as "disable e10s if accessibility is enabled", not just "a screen reader" which is just one of multiple accessibility methods.

Ok, so just an FYI, people running Win8+ with touch screens have accessibility enabled. So we'll lose those testers on Nightly if we don't provide a way for them to manually override whatever we land here to disable e10s. We have about 30K users on Nightly running Win8+, and about 10% (3000) of them have touch input capabilities.

Maybe before proceeding further we need to understand more clearly what exactly is currently incompatible with e10s. Is it all of accessibility, or specifically screen readers, or other accessibility methods as well?

(In reply to Jim Mathies [:jimm] from comment #3) > Ok, so just an FYI, people running Win8+ with touch screens have > accessibility enabled. So we'll lose those testers on Nightly if we don't > provide a way for them to manually override whatever we land here to disable > e10s. err, yeah, I'm not really sure if that usage of accessibility works with e10ns today or not, but they can atleast disable accessibility with accessibility.force_disabled pref. (In reply to Benoit Jacob [:bjacob] from comment #4) > Maybe before proceeding further we need to understand more clearly what > exactly is currently incompatible with e10s. Is it all of accessibility, or > specifically screen readers, or other accessibility methods as well? the full answer is its complicated, but its atleast kind of sort of broken for everything.

So I looked at this on both Windows and Linux. On Windows, what triggers the initial creation of the accessibility service, is a WM_GETOBJECT event. The call stack for that is xul.dll!NS_GetAccessibilityService(nsIAccessibilityService * * aResult) Line 1658 C++ xul.dll!CreateA11yService(nsISupports * aOuter, const nsID & aIID, void * * aResult) Line 631 C++ xul.dll!mozilla::GenericFactory::CreateInstance(nsISupports * aOuter, const nsID & aIID, void * * aResult) Line 17 C++ xul.dll!nsComponentManagerImpl::CreateInstanceByContractID(const char * aContractID, nsISupports * aDelegate, const nsID & aIID, void * * aResult) Line 1190 C++ xul.dll!nsComponentManagerImpl::GetServiceByContractID(const char * aContractID, const nsID & aIID, void * * aResult) Line 1552 C++ xul.dll!CallGetService(const char * aContractID, const nsID & aIID, void * * aResult) Line 70 C++ xul.dll!nsGetServiceByContractID::operator()(const nsID & aIID, void * * aInstancePtr) Line 280 C++ xul.dll!nsCOMPtr<nsIAccessibilityService>::assign_from_gs_contractid(const nsGetServiceByContractID aGS, const nsID & aIID) Line 1216 C++ xul.dll!nsCOMPtr<nsIAccessibilityService>::nsCOMPtr<nsIAccessibilityService>(const nsGetServiceByContractID aGS) Line 646 C++ xul.dll!mozilla::services::GetAccessibilityService() Line 8 C++ xul.dll!nsBaseWidget::GetRootAccessible() Line 1464 C++ xul.dll!nsWindow::GetAccessible() Line 6916 C++ > xul.dll!nsWindow::ProcessMessage(unsigned int msg, unsigned int & wParam, long & lParam, long * aRetValue) Line 5226 C++ xul.dll!nsWindow::WindowProcInternal(HWND__ * hWnd, unsigned int msg, unsigned int wParam, long lParam) Line 4396 C++ xul.dll!CallWindowProcCrashProtected(long (HWND__ *, unsigned int, unsigned int, long) * aWndProc, HWND__ * aHWnd, unsigned int aMsg, unsigned int aWParam, long aLParam) Line 35 C++ xul.dll!nsWindow::WindowProc(HWND__ * hWnd, unsigned int msg, unsigned int wParam, long lParam) Line 4348 C++ [External Code] xul.dll!mozilla::HangMonitor::IsUIMessageWaiting() Line 322 C++ xul.dll!mozilla::HangMonitor::NotifyActivity(mozilla::HangMonitor::ActivityType aActivityType) Line 340 C++ xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 821 C++ xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 265 C++ xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate) Line 99 C++ xul.dll!MessageLoop::RunInternal() Line 230 C++ xul.dll!MessageLoop::RunHandler() Line 223 C++ xul.dll!MessageLoop::Run() Line 197 C++ Since we don't control when we'll receive a WM_GETOBJECT event, I don't see what we can do on startup, on Windows. On Linux, the situation is different. We actively query the system for whether accessibility is in use, using D-Bus. The problem is that on my Ubuntu 14.04 LTS system, the system replies "yes". My system should be in a pretty default state, the Orca screen reader is not running, and all accessibility options are disabled in the Ubuntu control center. So it seems that if we disable e10s on Linux systems that say they have accessibility turned on, we'll be disabling e10s on Linux altogether.

(In reply to Benoit Jacob [:bjacob] from comment #6) > So I looked at this on both Windows and Linux. > > On Windows, what triggers the initial creation of the accessibility service, > is a WM_GETOBJECT event. The call stack for that is > > xul.dll!NS_GetAccessibilityService(nsIAccessibilityService * * aResult) > Line 1658 C++ > xul.dll!CreateA11yService(nsISupports * aOuter, const nsID & aIID, void * > * aResult) Line 631 C++ > xul.dll!mozilla::GenericFactory::CreateInstance(nsISupports * aOuter, > const nsID & aIID, void * * aResult) Line 17 C++ > xul.dll!nsComponentManagerImpl::CreateInstanceByContractID(const char * > aContractID, nsISupports * aDelegate, const nsID & aIID, void * * aResult) > Line 1190 C++ > xul.dll!nsComponentManagerImpl::GetServiceByContractID(const char * > aContractID, const nsID & aIID, void * * aResult) Line 1552 C++ > xul.dll!CallGetService(const char * aContractID, const nsID & aIID, void * > * aResult) Line 70 C++ > xul.dll!nsGetServiceByContractID::operator()(const nsID & aIID, void * * > aInstancePtr) Line 280 C++ > xul.dll!nsCOMPtr<nsIAccessibilityService>::assign_from_gs_contractid(const > nsGetServiceByContractID aGS, const nsID & aIID) Line 1216 C++ > > xul.dll!nsCOMPtr<nsIAccessibilityService>:: > nsCOMPtr<nsIAccessibilityService>(const nsGetServiceByContractID aGS) Line > 646 C++ > xul.dll!mozilla::services::GetAccessibilityService() Line 8 C++ > xul.dll!nsBaseWidget::GetRootAccessible() Line 1464 C++ > xul.dll!nsWindow::GetAccessible() Line 6916 C++ > > xul.dll!nsWindow::ProcessMessage(unsigned int msg, unsigned int & wParam, long & lParam, long * aRetValue) Line 5226 C++ > xul.dll!nsWindow::WindowProcInternal(HWND__ * hWnd, unsigned int msg, > unsigned int wParam, long lParam) Line 4396 C++ > xul.dll!CallWindowProcCrashProtected(long (HWND__ *, unsigned int, > unsigned int, long) * aWndProc, HWND__ * aHWnd, unsigned int aMsg, unsigned > int aWParam, long aLParam) Line 35 C++ > xul.dll!nsWindow::WindowProc(HWND__ * hWnd, unsigned int msg, unsigned int > wParam, long lParam) Line 4348 C++ > [External Code] > xul.dll!mozilla::HangMonitor::IsUIMessageWaiting() Line 322 C++ > > xul.dll!mozilla::HangMonitor::NotifyActivity(mozilla::HangMonitor:: > ActivityType aActivityType) Line 340 C++ > xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 821 > C++ > xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 265 > C++ > xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * > aDelegate) Line 99 C++ > xul.dll!MessageLoop::RunInternal() Line 230 C++ > xul.dll!MessageLoop::RunHandler() Line 223 C++ > xul.dll!MessageLoop::Run() Line 197 C++ > > > Since we don't control when we'll receive a WM_GETOBJECT event, I don't see > what we can do on startup, on Windows. ultimitely whatever we do it'll be out of our control to some digree, but maybe we get WM_GETOBJECT early enough, if not the only other thing you can try is looking at DLLs loaded when we need to make a decision about e10s. The DLL we know about are at http://mxr.mozilla.org/mozilla-central/source/accessible/windows/msaa/Compatibility.cpp#57 > On Linux, the situation is different. We actively query the system for > whether accessibility is in use, using D-Bus. The problem is that on my > Ubuntu 14.04 LTS system, the system replies "yes". My system should be in a > pretty default state, the Orca screen reader is not running, and all > accessibility options are disabled in the Ubuntu control center. So it seems > that if we disable e10s on Linux systems that say they have accessibility > turned on, we'll be disabling e10s on Linux altogether. yeah, were pretty screwed here, I guess we could go for absolutely terrible and exec ps?

(In reply to Trevor Saunders (:tbsaunde) from comment #7) > ultimitely whatever we do it'll be out of our control to some digree, but > maybe we get WM_GETOBJECT early enough, Unfortunately, that is not the case. "Early enough" would be right on loading browser.js, where it queries for "useRemoteTabs" when initializing the gMultiProcessBrowser global variable: http://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#43 At this point, we do not yet have a window. So in particular, this is much earlier than when we get WM_GETOBJECT messages sent to our window. I verified this locally.

(In reply to Trevor Saunders (:tbsaunde) from comment #7) > if not the only other thing you can > try is looking at DLLs loaded when we need to make a decision about e10s. > The DLL we know about are at > http://mxr.mozilla.org/mozilla-central/source/accessible/windows/msaa/ > Compatibility.cpp#57 That looks like it could work: I broke just before the point in browser.js where we query for useRemoteTabs, and at this point, nvdaHelperRemote.dll was already injected into my process.

This won't work if: 1. The AT doesn't hit Firefox until *after* you start Firefox. This is a totally valid use case; e.g. I'm starting NVDA to participate in something my wife is doing on her PC. 2. The AT injects later than you expect. This might happen if, for example, you start Firefox and alt+tab away while it is loading. In that case, Firefox mightn't fire a foreground event, so NVDA won't inject. 3. You're using an AT that isn't known to Firefox; e.g. Windows 8 Narrator. In fairness, Narrator doesn't work too well with Firefox, but no access would still be a regression. More interestingly, I'm not sure Narrator injects an in-process dll at all.

(In reply to James Teh [:Jamie] from comment #10) > This won't work if: oh sure, there are all sorts of things wrong with this, *but* its arguably less wrong than telling people to manually disable e10s.

Thanks James and Trevor. I understand that the approach to check for injected DLLs will not work great, but because of bug 1063680 I need to work on whatever we can do, even if that's imperfect. James, also note that people are only talking of enabling e10s on the Nightly channel at this point. There is no plan to regress anything on other channels at this point. An update on where we stand. I hope to have the DLL-injection-checking-on-startup patch ready by monday. What made it nontrivial is that we have a bunch of places, both in C++ and JS code, that directly query the browser.tabs.remote.autostart pref, and we need to change them to calling a function instead, and initially we thought that nsDOMWindowUtils would be a good trivially have a DOMWindow work with. Doesn't sound like more than a silly roadblock, but it costed me some time today.

Oops, the second paragraph in comment 12 is garbled, sorry. I meant this: An update on where we stand. I hope to have the DLL-injection-checking-on-startup patch ready by monday. What made it nontrivial is that we have a bunch of places, both in C++ and JS code, that directly query the browser.tabs.remote.autostart pref, and we need to change them to calling a function instead, and initially we thought that nsDOMWindowUtils would be a good home for such a function, but at least one of the call sites is in a .jsm module where we don't trivially have a DOMWindow to work with. Doesn't sound like more than a silly roadblock, but it costed me some time today.

This bug should be fixed for the e10s M2 milestone.

Work-in-progress / debugging patch. Should work, if not for the startup/injection timing issue described below. Builds on Felipe's patches in bug 1063848 which found a great place to put the single 'check if e10s' function to unify all the places where we were querying the pref manually. Problem: Unfortunately, it turns out that this is queried more times than I knew before, and some of the earlier times are earlier than when the DLL is injected in our process. See next attachment.

Here are the successive stacks that I get, to when we query this. Each time, "Loaded" indicates whether the DLL is already injected in our process. As you can see, the earlier calls, in particular around where we create a hidden top window, are happening before the DLL is injected. This is a problem: for this TabsRemoteAutostart() function to return something dependable, it should always return the same thing. So if we call it too early, we can't change our minds later.

on the browser.js call, can you dump what's window.location.href of the window that's accessing the getter? And DumpJSStack() on the other calls before the load happens? When browser.js is running there should definitely be some window object of some sorts, at least from our backend. It's possible that an OS widget window has not been created yet..

The call from JustCreateTopLevelWindow should be avoidable by checking the aIsHiddenWindow param. But the call to RegisterExtensionInterpositions, which is the earliest one, is probably not avoidable because that's what register the compatibility shims for the add-ons etc. But I think that if that gets activated it will probably be harmless for a browser not running e10s. I'm not sure about the 3rd call there, but i'm guessing it's the call from browser.js from the hidden window. But it needs more investigation. So it's possible that you could switch the value of gBrowserTabsRemoteAutostart during runtime without a lot of harm if a load to an a11y .dll happens. Of course it's not great, but the most important is that the safeMode check behaves correctly so that if it breaks something, a user can enter safe-mode and toggle the autostart pref to false.

As the approach discussed above was very imperfect and hard to make to work, Brad suggested this other approach instead: just restart Nightly if we detect a11y at any point. With session-restore, that should be an acceptable user experience as far as Nightly is concerned. The present patch does that, in a11y::Compatibility::Init(). That does exactly what is expected when a screen reader is launched before the browser. On the other hand, perhaps because that function is run only once, it does not detect when a screen reader is launched after the browser is already up and running.

This new patch correctly reacts to a screen reader being launched after Firefox is launched, by putting that logic in NS_GetAccessibilityService as Trevor originally suggested. As a bonus, it also isn't Windows-specific anymore, and it does correctly restore the browser session on restart. The last thing that bugs me is that restarting the browser is something that we normally inform the user about and give him a chance to opt out from, with some kind of dialog box, such as what we do after an update has been installed. Could we do the same thing here?

> This new patch correctly reacts to a screen reader being launched after > Firefox is launched, by putting that logic in NS_GetAccessibilityService as > Trevor originally suggested. As a bonus, it also isn't Windows-specific > anymore, and it does correctly restore the browser session on restart. seems pretty reasonable. > The last thing that bugs me is that restarting the browser is something that > we normally inform the user about and give him a chance to opt out from, > with some kind of dialog box, such as what we do after an update has been > installed. Could we do the same thing here? yeah, presumably with a timeout after which we just restart the browser?

Bug Bug 1064886 and Bug 1064885 are adding simple notices about restarting the browser for the e10s pref change to take effect. Perhaps you can use the same code here (dispatch a notification/event and call the same JS code?)

(In reply to Benoit Jacob [:bjacob] from comment #20) > The last thing that bugs me is that restarting the browser is something that > we normally inform the user about and give him a chance to opt out from, > with some kind of dialog box, such as what we do after an update has been > installed. Could we do the same thing here? Does it disable e10s permanently or just for the next session? If only for the next session, this would get pretty annoying for accessibility users. Every time they start Firefox, they'll see this notification and will need to respond to it (or wait for it if there's a timeout). Could there be a check box in this notification to permanently disable e10s for those of us who *always* use accessibiliy? Would this notification actually be accessible or is a11y completely broken at this point? I assume a11y is only broken for document tabs.

(In reply to James Teh [:Jamie] from comment #23) > (In reply to Benoit Jacob [:bjacob] from comment #20) > > The last thing that bugs me is that restarting the browser is something that > > we normally inform the user about and give him a chance to opt out from, > > with some kind of dialog box, such as what we do after an update has been > > installed. Could we do the same thing here? > Does it disable e10s permanently or just for the it adds a new pref and disables e10s as long as we care about that pref. Of course that's not a final decision, but seems reasonable enough. > Would this notification actually be accessible or is a11y completely broken > at this point? I assume a11y is only broken for document tabs. I suspect it would be accessible, but it depends on internals I don't know off hand.

Comment on attachment 8487279 [details] [diff] [review] Restart the browser in non-e10s mode whenever the accessibility service is used. This is definitely sucky. If the question is "is the UI good enough for nightly users", that's really a question for e10s drivers. I presume that we plan on fixing accessibility before letting this ride the trains. I can guarantee that we wouldn't want this code to be active for release or even beta users.

There is no intent at this point to use this on any other channel than Nightly. (In reply to James Teh [:Jamie] from comment #23) > Would this notification actually be accessible or is a11y completely broken > at this point? I assume a11y is only broken for document tabs. Yes, a11y is only broken for content coming from child processes. The browser UI, which is driven directly by the parent process, is still accessible. I verified this on Windows with NVDA. So this confirmation dialog should be accessible just fine.

This patch adds the UI part. If accessibility is used, we pop up a notification saying that accessibility won't work in e10s and asks the user to restart. Trevor, would you mind testing this? It relies on code that merged to m-c today, so you'll need a recent build. I'm mainly concerned about how easy it is to read the notification with a screen reader. Will the user be immediately aware of the popup, or will they have to move around to find it? If this doesn't work, we could open a whole new window.

This was missing a check.

Comment on attachment 8488304 [details] [diff] [review] disable-e10s-if-a11y v3 Bill, I was talking with bjacob about this bug and he said that, in the cases where the a11y tools are already running before FF starts, it's possible that this getter will run too early before there's a window where we can display the notification. So we need to handle that case somehow. I think in that case it's fine to just turn e10s off and quickly restart Firefox

Note that testing this yourselves is super easy, as on each OS there is a screen reader that we support and that is super easy to install and launch either before or after launching Firefox. On Windows, try NVDA. On Linux, try Orca. Regarding comment 29, indeed in the case where the screen reader is started before Firefox, on Windows, I've seen the DLL injection happen more or less at the time of creating the main Firefox window, not sure about the fine ordering there, but there is a chance that it would happen just a little too early to have a main window already opened, in which case you might fail to create a dialog box, but that should be a non-issue as in that case it's acceptable to just restart the browser without a confirmation (the user in that case didn't have time yet to do anything nontrivial with the browser, anyway).

(In reply to :Felipe Gomes from comment #29) > Bill, I was talking with bjacob about this bug and he said that, in the > cases where the a11y tools are already running before FF starts, it's > possible that this getter will run too early before there's a window where > we can display the notification. So we need to handle that case somehow. I > think in that case it's fine to just turn e10s off and quickly restart > Firefox Felipe: if the a11y checks in Bill's v3 patch are correct (for the cases being checked), could your proposed check for already-running a11y tools land in a follow-up patch?

Comment on attachment 8488304 [details] [diff] [review] disable-e10s-if-a11y v3 Review of attachment 8488304 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/nsBrowserGlue.js @@ +2272,5 @@ > if (!activationNoticeShown) { > this._showE10sActivatedNotice(); > } > + > + Services.obs.addObserver(this, "a11y-enabled-in-e10s", true); I thought the Observer service would reject adding a weak observers if it doesn't implement nsISupportsWeakRef. Can you check if this is actually working or if you need to define the QI in this object? @@ +2382,5 @@ > + let mainAction = { > + label: "Disable and Restart", > + accessKey: "R", > + callback: function () { > + Services.prefs.setBoolPref("browser.tabs.remote.autostart", true); Leftover? I don't think we need to set this pref

(In reply to Chris Peterson (:cpeterson) from comment #31) > (In reply to :Felipe Gomes from comment #29) > > Bill, I was talking with bjacob about this bug and he said that, in the > > cases where the a11y tools are already running before FF starts, it's > > possible that this getter will run too early before there's a window where > > we can display the notification. So we need to handle that case somehow. I > > think in that case it's fine to just turn e10s off and quickly restart > > Firefox > > Felipe: if the a11y checks in Bill's v3 patch are correct (for the cases > being checked), could your proposed check for already-running a11y tools > land in a follow-up patch? Yeah, it can be done in a follow-up patch. I guess the importance will depend on Trevor's feedback to see if this is the common case or the non-common case for the standard a11y tools.

Comment on attachment 8488304 [details] [diff] [review] disable-e10s-if-a11y v3 >+ // bug 1047076: accessibility is not yet e10s-ified, so at this point >+ // if we are in e10s we must restart the browser in non-e10s. >+ if (BrowserTabsRemoteAutostart()) { >+ nsCOMPtr<nsIObserverService> observerService = >+ mozilla::services::GetObserverService(); >+ if (!observerService) >+ return NS_ERROR_FAILURE; >+ >+ observerService->NotifyObservers(nullptr, "a11y-enabled-in-e10s", nullptr); what's wrong with the a11y-init-or-shutdown notification we already have?

Comment on attachment 8488304 [details] [diff] [review] disable-e10s-if-a11y v3 so, it looks like this doesn't work on linux we turn accessibility on to early. I suspect the same will be true for Mac, iirc there too we check if we should start accessibility when the first native window is created. It shouldn't be terribly hard to check if accessibility is on when you go to register with the observer service, you'll just have to stick a method on some handy xpcom thing that calls GetAccessibilityService() which won't try and create the service if it doesn't already exist.

Thanks for all the help on this. I think this fixes all the feedback. I pushed a Windows build to try and I'll test it when it's done: https://tbpl.mozilla.org/?tree=Try&rev=0ca16f871c10 It seems to work okay on Linux, although it looks like a11y only works if you start up with it on Linux. Maybe Windows will be different.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0529fb43bd5f I made a few changes after the review (okayed by Felipe). There's now a "Don't Disable" option. I also ensured that we don't display the e10s prompt if a11y is enabled at startup. I also found some remaining problems with this patch, but I think it's better to land it now. We can follow up in bug 1066901.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/92de4e82f011 - remains to be seen whether it's all debug, or all OS X, or only OS X debug (the only one of those which has finished yet), but OS X debug certainly crashes, https://tbpl.mozilla.org/php/getParsedLog.php?id=48011154&tree=Mozilla-Inbound

philor notes on TBPL that this patch should also rev the UUID.

Got a green try push after changing the UUID, so I think that was the only problem. https://hg.mozilla.org/integration/mozilla-inbound/rev/161056025760

I hope this doesn't get backed out again. Here was my try push (with green windows builds): https://tbpl.mozilla.org/?tree=Try&rev=4c839e44ddb8

https://hg.mozilla.org/mozilla-central/rev/161056025760 https://hg.mozilla.org/mozilla-central/rev/c3fb7d39e22c

Is there anyway to get around this on Linux?