Closed Bug 527003 Opened 15 years ago Closed 8 years ago

Need a way to shutdown accessibility (on/off)

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53

People

(Reporter: davidb, Assigned: yzen)

References

Details

(Keywords: access)

Attachments

(2 files, 7 obsolete files)

Bug 527003 - make sure a11y shutdown and start work with e10s. When accessibility service is requested, created, or shutdown, indicate if it is done by parent process, platform API or XPCOM.
58 bytes, text/x-review-board-request
surkov
: review+
tbsaunde
: review+
Details
Bug 527003 - adding tests for accessibility service shutdown (both e10s and non-e10s).
58 bytes, text/x-review-board-request
surkov
: review+
Details
This sounds scary I know. In the long run I think this will be good for accessibility. Here's why: 1. It will allow us to more easy do accessibility performance metrics 2. It will add a potential solution to performance degradation on systems where accessibility is only used by anti-virus software (like Kaspersky). The antivirus/antispy software could turn accessibility on only when needed (e.g. password entry). 3. It will give us an extra tool in doing quick triage with users who don't require accessibility. "Can you just turn accessibility off and try to recreate that bug?" Thoughts?
Which would be the default for the option? Will the option in UI? On Linux / Solaris, you can simply use "export GNOME_ACCESSIBILITY=0" to turn it off at startup. I think it would not be difficult to implement it.
Ginn, we want it to happen at runtime, so that single sign-on tools that use MSAA to find password fields can just use it, and then say they are done with it. It would be awesome if you could take this bug on. Probably easiest to add some UI for it for the first pass, so we can try it out easily.
There is no standard mechanism for requesting that accessibility be shut down dynamically on-demand. This would require some sort of new API. One possibility on Windows is to shut down accessibility within a certain time (a minute, perhaps?) of all accessible objects being released. It could then be re-enabled like it currently is; i.e. when WM_GETOBJECT is received. There would need to be a pref or environment variable if you want users to be able to forceably disable accessibility. Please do *not* use the Windows screen reader flag to make any such determinations. This whole concept really scares me. I don't want to see it break the brilliant accessibility story for Mozilla. :)
Jamie we'll make it work well, or not at all. I like your idea of detecting non-usage... hmmm... and yes I was originally thinking of new API, or at least abusing some existing API.
(In reply to comment #4) > Jamie we'll make it work well, or not at all. Good to hear. :) > I like your idea of detecting non-usage This is the simplest method to allow for performance optimisation once a11y objects are no longer required. It also requires the least change on the application side. > and yes I was originally > thinking of new API, or at least abusing some existing API. In Windows, you could register a custom window message which, when received, would shut down a11y. However, there are several issues with any such API: * What if there was more than one a11y client? Client A might shut down accessibility, but meanwhile, poor client B, which was still happily using a11y, is now screwed. The above approach solves this because it doesn't happen until all objects are released. * Do you want it to *stay* shut down or just until another WM_GETOBJECT is received? Having a message to really shut it down could be nice for testing, but would be extremely dangerous because once shut down by one client, no other clients could use a11y. Hell, someone could even invent a DoS attack for a11y users. * Those are just the issues that occurred to me in the space of a few seconds. There's probably more I haven't thought of yet...
(In reply to comment #5) > (In reply to comment #4) > * Do you want it to *stay* shut down or just until another WM_GETOBJECT is > received? The latter, at least for release builds. > Having a message to really shut it down could be nice for testing, > but would be extremely dangerous because once shut down by one client, no other > clients could use a11y. Hell, someone could even invent a DoS attack for a11y > users. Right, we can't let that happen. > * Those are just the issues that occurred to me in the space of a few seconds. > There's probably more I haven't thought of yet... I'm glad you didn't wait a whole minute :)
We (gok folks) killed ourselves trying to make sure GNOME couldn't lock out on-screen keyboard users; I'm sure not wanting to let that happen with FF -- believe me! :) I like the inactivity approach -- basically inactivity of calls coming in should do the trick. If MSAA or ATK or whatever clients give us the silent treatment, we can ignore them too :)
As I noted, i think it's important to define inactivity in two ways: 1. All objects must be released. 2. There must be a short time after (1) before shutting down a11y. It's feasible that something might release all of its objects and then grab another one in quick succession, so shutting down a11y after (1) alone would be nasty, not least because it halts events and ditches the object cache. You would also need to find out whether all ATs hold at least one a11y object at all times when they want to watch an application. If they don't, walking away from your computer might stop events from happening, which would really suck. NVDA does hold at least the application window and root document object around.
I'm listening. Your brain dump is extremely valuable.
(In reply to comment #8) > As I noted, i think it's important to define inactivity in two ways: > 1. All objects must be released. I think we could probably keep the application accessible alive.
Aaron had a neat idea today (over IRC). On windows we could track what windows have accessibility in use, and when/if all such windows are closed, we could flush accessibility (shutdown our engine). I'll try to code this up sometime in the next few days. So when a WM_GET_OBJECT comes in, record what window it was for... etc.
Close isn't good because AT could send this event to the top window just because it reacts on MSAA events we send. Since we received this event then we could wait for new events and watch on user input. If user is active, i.e. operates with the application but we don't receive new events then we can stop accessibility. The disadvantages of this approach is if AT would listen rare fired event and nothing else then we will turn on/off a11y.
(In reply to comment #12) > Close isn't good because AT could send this event to the top window just > because it reacts on MSAA events we send. Do you mean the AT (or antispyware etc) will probably send WM_GETOBJECT to the main application window? I think you are probably correct unfortunately.
So the idea is to run the timer and watch for the user activity and WM_GETOBJECT events. When timer triggers and if there was user input but no WM_GETOBJECT events during this time then we need to unload the a11y. Robert, do you have an idea how to watch for the user activity (mouse and keyboard usage)?
Ok, it sounds watching the user activity might be not enough. surkov: MarcoZ, do screen readers prevent keyboard events to process them? [02:29] MarcoZ: surkov: Yes they do as long as the user works in the virtual buffer. [02:29] MarcoZ: So if I read a long page (for example an wikipedia article) in JAWS for half an hour, the browser might not receive any events.
So I think we should monitor our accessibility events and WM_GETOBJECT events. So if we send events but AT doesn't respond then we can disable a11y. We could use accessible focus event as litmus event because any screen reader needs it. uncc'ing Robert to not bother him by a11y discussion.
It is worth trying out, there are a few ways we could monitor a11y API inactivity, but looking just after a focus event makes sense. Note some AT queue events and process them in an idle handler... so you might need a 100msec+ timer.
Or actually, if they want the accessible from the event, I guess that is sync.
(In reply to comment #8) > As I noted, i think it's important to define inactivity in two ways: > 1. All objects must be released. the problem we keep objects internally as well. We need to know how many addrefs we did on out side to know if objects were released by AT. It might be trick.
(In reply to comment #18) > Note some AT queue events and process them in an idle handler... so you might > need a 100msec+ timer. That's not problem I think. James, what will you say?
(In reply to comment #20) > > As I noted, i think it's important to define inactivity in two ways: > > 1. All objects must be released. > the problem we keep objects internally as well. We need to know how many > addrefs we did on out side to know if objects were released by AT. It might be > trick. I don't follow. Once the refcount on an object hits 0, it can be discarded, correct? And once all objects have been discarded, then (and only then) can a11y be considered potentially ready for shutdown. (In reply to comment #21) > > Note some AT queue events and process them in an idle handler... so you might > > need a 100msec+ timer. > That's not problem I think. NVDA does not currently handle events in-process, which means we handle them out-of-process (asynchronous handling). Therefore, there may be some short delay between the time you dispatch the event and the time you see a WM_GETOBJECT from us. If the system is highly loaded, this could indeed be 100ms or maybe longer. Normally, it'd probably only be a few ms.
(In reply to comment #22) > (In reply to comment #20) > > > As I noted, i think it's important to define inactivity in two ways: > > > 1. All objects must be released. > > the problem we keep objects internally as well. We need to know how many > > addrefs we did on out side to know if objects were released by AT. It might be > > trick. > I don't follow. Once the refcount on an object hits 0, it can be discarded, > correct? And once all objects have been discarded, then (and only then) can > a11y be considered potentially ready for shutdown. I mean this won't ever happen until we shutdown because we addref objects too. So that if AT gets addrefed object, use it and release it then object won't be destroyed because its reference count is not 0 since we keep it in own cache, it is kept by other objects for parent/child relashionship. > (In reply to comment #21) > > > Note some AT queue events and process them in an idle handler... so you might > > > need a 100msec+ timer. > > That's not problem I think. > NVDA does not currently handle events in-process, which means we handle them > out-of-process (asynchronous handling). Therefore, there may be some short > delay between the time you dispatch the event and the time you see a > WM_GETOBJECT from us. If the system is highly loaded, this could indeed be > 100ms or maybe longer. Normally, it'd probably only be a few ms. I wonder if 1 sec would be enough? This should be ok from the user point of view and I hope it should be ok for out-of-process AT.
Focus approach will break accessibility tools. If we could force Gecko-based tools to register theirself to prevent a11y unloading then we can't do this for platform tools. We could watch if AT call methods of our accessible objects but it should be done on platform specific part since we can easy trigger cross platform methods when we fire events. If Gecko-based tools will register itself and we will watch for platfrom specific methods then it could work. But it looks complex.
(In reply to comment #24) > If Gecko-based tools will register itself > and we will watch for platfrom specific methods then it could work. But it > looks complex. I think we should try it.
(In reply to comment #22) > NVDA does not currently handle events in-process Oops, not quite true. We handle events for virtual buffers in-process and we only process them every 300ms. We also hold the document accessible out-of-process which should stop a11y from dying, but I thought this was worth noting nevertheless. So, I think the timer should be at minimum 1 sec as suggested in comment #23.
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
--- accessible/base/nsAccessibilityService.cpp | 11 +--- accessible/base/nsAccessibilityService.h | 19 +++--- accessible/interfaces/moz.build | 5 -- accessible/interfaces/nsIAccessibilityService.h | 79 ------------------------ accessible/interfaces/nsIAccessibleRetrieval.idl | 6 +- dom/ipc/ContentChild.cpp | 5 +- layout/build/nsLayoutModule.cpp | 2 +- layout/inspector/inDOMView.cpp | 5 +- widget/gtk/nsWindow.cpp | 3 +- widget/nsBaseWidget.cpp | 3 +- widget/windows/nsWindow.cpp | 3 +- xpcom/build/ServiceList.h | 4 -- xpcom/build/Services.cpp | 3 - 13 files changed, 21 insertions(+), 127 deletions(-) delete mode 100644 accessible/interfaces/nsIAccessibilityService.h Review commit: https://reviewboard.mozilla.org/r/60074/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60074/
Attachment #8764037 - Flags: review?(surkov.alexander)
--- accessible/base/nsAccessibilityService.cpp | 8 +++---- accessible/base/nsAccessibilityService.h | 8 +++---- accessible/interfaces/moz.build | 2 +- ...leRetrieval.idl => nsIAccessibilityService.idl} | 2 +- accessible/jsat/AccessFu.jsm | 2 +- accessible/jsat/ContentControl.jsm | 6 ++--- accessible/jsat/EventManager.jsm | 4 ++-- accessible/jsat/OutputGenerator.jsm | 4 ++-- accessible/jsat/Traversal.jsm | 2 +- accessible/jsat/Utils.jsm | 24 +++++++++---------- accessible/jsat/content-script.js | 2 +- accessible/tests/crashtests/448064.xhtml | 4 ++-- accessible/tests/crashtests/471493.xul | 6 ++--- accessible/tests/mochitest/common.js | 28 +++++++++++----------- accessible/tests/mochitest/events/docload_wnd.html | 12 +++++----- .../tests/mochitest/events/test_docload.html | 4 ++-- accessible/tests/mochitest/pivot.js | 2 +- .../tests/mochitest/states/z_frames_update.html | 6 ++--- layout/build/nsLayoutModule.cpp | 1 - testing/marionette/accessibility.js | 10 ++++---- testing/talos/talos/tests/a11y/a11y.js | 18 +++++++------- 21 files changed, 77 insertions(+), 78 deletions(-) rename accessible/interfaces/{nsIAccessibleRetrieval.idl => nsIAccessibilityService.idl} (98%) Review commit: https://reviewboard.mozilla.org/r/60076/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60076/
--- accessible/base/moz.build | 1 + accessible/base/nsAccessibilityService.cpp | 285 +---------------- accessible/base/nsAccessibilityService.h | 45 ++- accessible/interfaces/nsIAccessibilityService.idl | 10 - accessible/xpcom/moz.build | 2 + accessible/xpcom/xpcAccessibilityService.cpp | 356 ++++++++++++++++++++++ accessible/xpcom/xpcAccessibilityService.h | 41 +++ dom/ipc/ContentChild.cpp | 2 +- layout/build/nsLayoutModule.cpp | 4 +- layout/inspector/inDOMView.cpp | 2 +- widget/gtk/nsWindow.cpp | 2 +- widget/nsBaseWidget.cpp | 2 +- widget/windows/nsWindow.cpp | 2 +- 13 files changed, 448 insertions(+), 306 deletions(-) create mode 100644 accessible/xpcom/xpcAccessibilityService.cpp create mode 100644 accessible/xpcom/xpcAccessibilityService.h Review commit: https://reviewboard.mozilla.org/r/60078/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60078/
--- accessible/base/DocManager.cpp | 25 +- accessible/base/nsAccessibilityService.cpp | 23 +- accessible/base/nsAccessibilityService.h | 19 +- accessible/moz.build | 5 +- accessible/tests/browser/browser.ini | 53 +--- accessible/tests/browser/browser_shutdown.js | 33 +++ accessible/tests/browser/{ => e10s}/browser.ini | 1 + .../{ => e10s}/browser_caching_attributes.js | 0 .../{ => e10s}/browser_caching_description.js | 0 .../browser/{ => e10s}/browser_caching_name.js | 0 .../{ => e10s}/browser_caching_relations.js | 0 .../browser/{ => e10s}/browser_caching_states.js | 0 .../browser/{ => e10s}/browser_caching_value.js | 0 .../browser/{ => e10s}/browser_events_caretmove.js | 0 .../browser/{ => e10s}/browser_events_hide.js | 0 .../browser/{ => e10s}/browser_events_show.js | 0 .../{ => e10s}/browser_events_statechange.js | 0 .../{ => e10s}/browser_events_textchange.js | 0 .../{ => e10s}/browser_treeupdate_ariadialog.js | 0 .../{ => e10s}/browser_treeupdate_ariaowns.js | 0 .../{ => e10s}/browser_treeupdate_canvas.js | 0 .../{ => e10s}/browser_treeupdate_cssoverflow.js | 0 .../browser/{ => e10s}/browser_treeupdate_doc.js | 0 .../{ => e10s}/browser_treeupdate_gencontent.js | 0 .../{ => e10s}/browser_treeupdate_hidden.js | 0 .../{ => e10s}/browser_treeupdate_imagemap.js | 0 .../browser/{ => e10s}/browser_treeupdate_list.js | 0 .../browser_treeupdate_list_editabledoc.js | 0 .../{ => e10s}/browser_treeupdate_listener.js | 0 .../{ => e10s}/browser_treeupdate_optgroup.js | 0 .../{ => e10s}/browser_treeupdate_removal.js | 0 .../browser/{ => e10s}/browser_treeupdate_table.js | 0 .../{ => e10s}/browser_treeupdate_textleaf.js | 0 .../{ => e10s}/browser_treeupdate_visibility.js | 0 .../{ => e10s}/browser_treeupdate_whitespace.js | 0 .../{ => e10s}/doc_treeupdate_ariadialog.html | 0 .../{ => e10s}/doc_treeupdate_ariaowns.html | 0 .../{ => e10s}/doc_treeupdate_imagemap.html | 0 .../{ => e10s}/doc_treeupdate_removal.xhtml | 0 .../{ => e10s}/doc_treeupdate_visibility.html | 0 .../{ => e10s}/doc_treeupdate_whitespace.html | 0 accessible/tests/browser/{ => e10s}/events.js | 0 accessible/tests/browser/e10s/head.js | 84 ++++++ accessible/tests/browser/head.js | 301 +-------------------- .../tests/browser/{head.js => shared-head.js} | 82 +----- accessible/xpcom/xpcAccessibilityService.cpp | 69 ++++- accessible/xpcom/xpcAccessibilityService.h | 16 +- 47 files changed, 254 insertions(+), 457 deletions(-) create mode 100644 accessible/tests/browser/browser_shutdown.js copy accessible/tests/browser/{ => e10s}/browser.ini (97%) rename accessible/tests/browser/{ => e10s}/browser_caching_attributes.js (100%) rename accessible/tests/browser/{ => e10s}/browser_caching_description.js (100%) rename accessible/tests/browser/{ => e10s}/browser_caching_name.js (100%) rename accessible/tests/browser/{ => e10s}/browser_caching_relations.js (100%) rename accessible/tests/browser/{ => e10s}/browser_caching_states.js (100%) rename accessible/tests/browser/{ => e10s}/browser_caching_value.js (100%) rename accessible/tests/browser/{ => e10s}/browser_events_caretmove.js (100%) rename accessible/tests/browser/{ => e10s}/browser_events_hide.js (100%) rename accessible/tests/browser/{ => e10s}/browser_events_show.js (100%) rename accessible/tests/browser/{ => e10s}/browser_events_statechange.js (100%) rename accessible/tests/browser/{ => e10s}/browser_events_textchange.js (100%) rename accessible/tests/browser/{ => e10s}/browser_treeupdate_ariadialog.js (100%) rename accessible/tests/browser/{ => e10s}/browser_treeupdate_ariaowns.js (100%) rename accessible/tests/browser/{ => e10s}/browser_treeupdate_canvas.js (100%) rename accessible/tests/browser/{ => e10s}/browser_treeupdate_cssoverflow.js (100%) rename accessible/tests/browser/{ => e10s}/browser_treeupdate_doc.js (100%) rename accessible/tests/browser/{ => e10s}/browser_treeupdate_gencontent.js (100%) rename accessible/tests/browser/{ => e10s}/browser_treeupdate_hidden.js (100%) rename accessible/tests/browser/{ => e10s}/browser_treeupdate_imagemap.js (100%) rename accessible/tests/browser/{ => e10s}/browser_treeupdate_list.js (100%) rename accessible/tests/browser/{ => e10s}/browser_treeupdate_list_editabledoc.js (100%) rename accessible/tests/browser/{ => e10s}/browser_treeupdate_listener.js (100%) rename accessible/tests/browser/{ => e10s}/browser_treeupdate_optgroup.js (100%) rename accessible/tests/browser/{ => e10s}/browser_treeupdate_removal.js (100%) rename accessible/tests/browser/{ => e10s}/browser_treeupdate_table.js (100%) rename accessible/tests/browser/{ => e10s}/browser_treeupdate_textleaf.js (100%) rename accessible/tests/browser/{ => e10s}/browser_treeupdate_visibility.js (100%) rename accessible/tests/browser/{ => e10s}/browser_treeupdate_whitespace.js (100%) rename accessible/tests/browser/{ => e10s}/doc_treeupdate_ariadialog.html (100%) rename accessible/tests/browser/{ => e10s}/doc_treeupdate_ariaowns.html (100%) rename accessible/tests/browser/{ => e10s}/doc_treeupdate_imagemap.html (100%) rename accessible/tests/browser/{ => e10s}/doc_treeupdate_removal.xhtml (100%) rename accessible/tests/browser/{ => e10s}/doc_treeupdate_visibility.html (100%) rename accessible/tests/browser/{ => e10s}/doc_treeupdate_whitespace.html (100%) rename accessible/tests/browser/{ => e10s}/events.js (100%) create mode 100644 accessible/tests/browser/e10s/head.js copy accessible/tests/browser/{head.js => shared-head.js} (71%) Review commit: https://reviewboard.mozilla.org/r/60080/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60080/
Attachment #8764038 - Flags: review?(surkov.alexander)
Attachment #8764039 - Flags: review?(surkov.alexander)
Attachment #8764040 - Flags: review?(surkov.alexander)
https://reviewboard.mozilla.org/r/60074/#review57100 ::: widget/windows/nsWindow.cpp:6972 (Diff revision 1) > // In case of popup window return a popup accessible. > nsView* view = nsView::GetViewFor(this); > if (view) { > nsIFrame* frame = view->GetFrame(); > if (frame && nsLayoutUtils::IsPopup(frame)) { > - nsCOMPtr<nsIAccessibilityService> accService = > + nsAccessibilityService* accService = GetAccService(); a11y won't be instantiated anymore ::: xpcom/build/ServiceList.h (Diff revision 1) > // IWYU pragma: private, include "mozilla/Services.h" > > -#ifdef ACCESSIBILITY > -MOZ_SERVICE(AccessibilityService, nsIAccessibilityService, > - "@mozilla.org/accessibilityService;1") > -#endif no retrieval service instead?
Comment on attachment 8764039 [details] Bug 527003 - separated out xpcom parts of nsAccessibilityService. https://reviewboard.mozilla.org/r/60078/#review57102 ::: accessible/xpcom/xpcAccessibilityService.cpp:1 (Diff revision 1) > +/* This Source Code Form is subject to the terms of the Mozilla Public it'd be good to hg copy the file to keep history ::: widget/windows/nsWindow.cpp:6972 (Diff revision 1) > // In case of popup window return a popup accessible. > nsView* view = nsView::GetViewFor(this); > if (view) { > nsIFrame* frame = view->GetFrame(); > if (frame && nsLayoutUtils::IsPopup(frame)) { > - nsAccessibilityService* accService = GetAccService(); > + nsAccessibilityService* accService = GetOrCreateAccService(); oh, you do change it back here, a little bit harder to review, since I need to look at two pathes same time
Attachment #8764039 - Flags: review?(surkov.alexander)
Comment on attachment 8764038 [details] Bug 527003 - renamed nsIAccessibleRetrieval to nsIAccessibilityService. https://reviewboard.mozilla.org/r/60076/#review57104 ::: accessible/interfaces/nsIAccessibilityService.idl:20 (Diff revision 1) > * An interface for in-process accessibility clients wishing to get an > * nsIAccessible for a given DOM node. More documentation at: > * http://www.mozilla.org/projects/ui/accessibility > */ > [scriptable, uuid(17f86615-1a3d-4021-b227-3a2ef5cbffd8)] > -interface nsIAccessibleRetrieval : nsISupports > +interface nsIAccessibilityService : nsISupports it'd be good to keep code changes from renamings and calls, I might miss but I don't see where xpcom service is renamed
Attachment #8764038 - Flags: review?(surkov.alexander)
I didn't see any problems, just had some difficulties to track the changes over patches (I wish they had different structure), however I'm not even close to finish the review. Yurich, please feel free to reassign the review to David or to any other peer, since I won't be able to complete it anytime soon (because of vacations).
Comment on attachment 8764037 [details] Bug 527003 - make sure a11y shutdown and start work with e10s. When accessibility service is requested, created, or shutdown, indicate if it is done by parent process, platform API or XPCOM. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60074/diff/1-2/
Attachment #8764037 - Attachment description: Bug 527003 - removing nsIAccessibilityService. → Bug 527003 - adding support for shutting down nsAccessibilityService.
Attachment #8764038 - Attachment is obsolete: true
Attachment #8764039 - Attachment is obsolete: true
Attachment #8764040 - Attachment is obsolete: true
Attachment #8764040 - Flags: review?(surkov.alexander)
Hi Marco, Here's Windows build try job to take a look if things look as expected: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2508072b7924 Thanks
Flags: needinfo?(mzehe)
From this end, looks good. The accessibility support starts normally with both NVDA and JAWS, no irregular behavior detected.
Flags: needinfo?(mzehe)
--- accessible/jsat/AccessFu.jsm | 2 +- accessible/jsat/ContentControl.jsm | 6 ++--- accessible/jsat/EventManager.jsm | 4 ++-- accessible/jsat/OutputGenerator.jsm | 4 ++-- accessible/jsat/Traversal.jsm | 2 +- accessible/jsat/Utils.jsm | 24 +++++++++---------- accessible/jsat/content-script.js | 2 +- accessible/tests/crashtests/448064.xhtml | 4 ++-- accessible/tests/crashtests/471493.xul | 6 ++--- accessible/tests/mochitest/common.js | 28 +++++++++++----------- accessible/tests/mochitest/events/docload_wnd.html | 12 +++++----- .../tests/mochitest/events/test_docload.html | 4 ++-- accessible/tests/mochitest/pivot.js | 2 +- .../tests/mochitest/states/z_frames_update.html | 6 ++--- testing/marionette/accessibility.js | 10 ++++---- testing/talos/talos/tests/a11y/a11y.js | 18 +++++++------- 16 files changed, 67 insertions(+), 67 deletions(-) Review commit: https://reviewboard.mozilla.org/r/64334/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64334/
Attachment #8764037 - Attachment description: Bug 527003 - adding support for shutting down nsAccessibilityService. → Bug 527003 - added tests for accessibility service shutdown (both e10s and non-e10s).
Attachment #8771075 - Flags: review?(surkov.alexander)
Attachment #8771076 - Flags: review?(surkov.alexander)
Attachment #8771077 - Flags: review?(surkov.alexander)
Attachment #8771078 - Flags: review?(surkov.alexander)
--- layout/build/nsLayoutModule.cpp | 3 +-- layout/inspector/inDOMView.cpp | 5 ++--- widget/gtk/nsWindow.cpp | 3 +-- widget/nsBaseWidget.cpp | 3 +-- widget/windows/nsWindow.cpp | 3 +-- xpcom/build/ServiceList.h | 4 ---- xpcom/build/Services.cpp | 3 --- 7 files changed, 6 insertions(+), 18 deletions(-) Review commit: https://reviewboard.mozilla.org/r/64336/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64336/
--- dom/ipc/ContentChild.cpp | 20 ++++++++++++++++---- dom/ipc/ContentChild.h | 4 +++- dom/ipc/ContentParent.cpp | 33 +++++++++++++++++++++------------ dom/ipc/PContent.ipdl | 7 ++++++- 4 files changed, 46 insertions(+), 18 deletions(-) Review commit: https://reviewboard.mozilla.org/r/64338/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64338/
--- accessible/base/DocManager.cpp | 25 +- accessible/base/moz.build | 4 +- accessible/base/nsAccessibilityService.cpp | 317 +--------------- accessible/base/nsAccessibilityService.h | 93 ++++- accessible/interfaces/moz.build | 7 +- accessible/interfaces/nsIAccessibilityService.h | 79 ---- ...leRetrieval.idl => nsIAccessibilityService.idl} | 12 +- accessible/xpcom/moz.build | 2 + accessible/xpcom/xpcAccessibilityService.cpp | 419 +++++++++++++++++++++ accessible/xpcom/xpcAccessibilityService.h | 67 ++++ 10 files changed, 610 insertions(+), 415 deletions(-) delete mode 100644 accessible/interfaces/nsIAccessibilityService.h rename accessible/interfaces/{nsIAccessibleRetrieval.idl => nsIAccessibilityService.idl} (92%) create mode 100644 accessible/xpcom/xpcAccessibilityService.cpp create mode 100644 accessible/xpcom/xpcAccessibilityService.h Review commit: https://reviewboard.mozilla.org/r/64340/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64340/
Comment on attachment 8764037 [details] Bug 527003 - make sure a11y shutdown and start work with e10s. When accessibility service is requested, created, or shutdown, indicate if it is done by parent process, platform API or XPCOM. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60074/diff/2-3/
accessible/tests/browser/browser_events_textchange.js accessible/tests/browser/e10s/browser_events_textchange.js (moved) just curious, why are all these moved files prefixed by browser_ while they are contained in browser folder
Those tests load original mochitests (like common.js) files both into chrome and content. It makes it hard to track all references to the xpcom objects in tests that test shutdown. So I logically moved e10s specific tests into their own folder to not interfere with the shutdown and any future tests. Also it somewhat dependent on how the browser tests run: from what I could tell, they do not start a new window/instance between tests so the object references persist.
Comment on attachment 8771078 [details] Bug 527003 - removing nsIAccessibleRetrieval, separating xpcom parts from nsAccessibilityService. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64340/diff/1-2/
Comment on attachment 8764037 [details] Bug 527003 - make sure a11y shutdown and start work with e10s. When accessibility service is requested, created, or shutdown, indicate if it is done by parent process, platform API or XPCOM. https://reviewboard.mozilla.org/r/60074/#review61648 I'm not sure I understand every and each piece there, but it looks good, r=me with questions answered/addressed ::: accessible/tests/browser/browser_shutdown_remote_no_reference.js:29 (Diff revision 3) > + let contentA11yInitOrShutdown = contentA11yInitOrShutdownPromise(browser); > + let accService = Cc['@mozilla.org/accessibilityService;1'].getService( > + Ci.nsIAccessibilityService); > + ok(accService, 'Service initialized in parent'); > + let [parentFlag, contentFlag] = yield Promise.all( > + [ parentA11yInitOrShutdown, contentA11yInitOrShutdown ]); not sure I caught how it works as I don't see where a11y service is created in content ::: accessible/tests/browser/browser_shutdown_remote_own_reference.js:24 (Diff revision 3) > + loadFrameScripts(browser, a11yInitOrShutdownPromise.toSource()); > + > + info('Creating a service in parent and waiting for service to be created ' + > + 'in content'); > + let parentA11yInitOrShutdown = a11yInitOrShutdownPromise(); > + let contentA11yInitOrShutdown = contentA11yInitOrShutdownPromise(browser); isn't it e10s test? ::: accessible/tests/browser/browser_shutdown_scope_lifecycle.js:19 (Diff revision 3) > + let accService = Cc['@mozilla.org/accessibilityService;1'].getService( > + Ci.nsIAccessibilityService); > + ok(accService, 'Service initialized'); > + })(); > + > + yield a11yInitThenShutdown; nice!
Attachment #8764037 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8771076 [details] Bug 527003 - renaming all XPCOM uses of deprecated accessible retrieval to accessibility service. https://reviewboard.mozilla.org/r/64336/#review61656 looks good with comments addressed ::: layout/build/nsLayoutModule.cpp:672 (Diff revision 1) > NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR(nsIHardwareKeyHandler, > HardwareKeyHandler::GetInstance) > #endif > > #ifdef ACCESSIBILITY > -#include "nsAccessibilityService.h" > +#include "xpcAccessibilityService.h" it seems it goes from another patch ::: layout/build/nsLayoutModule.cpp (Diff revision 1) > { GONK_GPS_GEOLOCATION_PROVIDER_CONTRACTID, &kGONK_GPS_GEOLOCATION_PROVIDER_CID }, > #endif > { MEDIAMANAGERSERVICE_CONTRACTID, &kNS_MEDIAMANAGERSERVICE_CID }, > #ifdef ACCESSIBILITY > { "@mozilla.org/accessibilityService;1", &kNS_ACCESSIBILITY_SERVICE_CID }, > - { "@mozilla.org/accessibleRetrieval;1", &kNS_ACCESSIBILITY_SERVICE_CID }, you shouldn't remove this for backward compatibility ::: layout/inspector/inDOMView.cpp:332 (Diff revision 1) > break; > } > > #ifdef ACCESSIBILITY > if (mShowAccessibleNodes) { > - nsCOMPtr<nsIAccessibilityService> accService = > + nsAccessibilityService* accService = GetOrCreateAccService(); nit: wrong identation ::: xpcom/build/ServiceList.h (Diff revision 1) > // IWYU pragma: private, include "mozilla/Services.h" > > -#ifdef ACCESSIBILITY > -MOZ_SERVICE(AccessibilityService, nsIAccessibilityService, > - "@mozilla.org/accessibilityService;1") > -#endif you should keep it, right?
Attachment #8771076 - Flags: review?(surkov.alexander) → review+
Attachment #8771075 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8771075 [details] Bug 527003 - making sure a11y shutdown and start work with e10s. https://reviewboard.mozilla.org/r/64334/#review61654
https://reviewboard.mozilla.org/r/64340/#review61660 ::: accessible/base/DocManager.cpp:94 (Diff revision 2) > DocManager::NotifyOfDocumentShutdown(DocAccessible* aDocument, > nsIDocument* aDOMDocument) > { > + RemoveListeners(aDOMDocument); > + > + if (nsAccessibilityService::IsShutdown()) { why? ::: accessible/base/DocManager.cpp:551 (Diff revision 2) > } > + > + iter.Remove(); > + } > + > + while(mXPCDocumentCache.Count() > 0) { nit: space after while ::: accessible/base/moz.build:10 (Diff revision 2) > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > EXPORTS += [ > 'AccEvent.h', > - 'nsAccessibilityService.h' > + 'nsAccessibilityService.h', > + 'nsCoreUtils.h' what's for and can it be avoided? ::: accessible/interfaces/nsIAccessibilityService.idl (Diff revision 2) > -// for component registration > -// {663CA4A8-D219-4000-925D-D8F66406B626} > -#define NS_ACCESSIBLE_RETRIEVAL_CID \ > -{ 0x663ca4a8, 0xd219, 0x4000, { 0x92, 0x5d, 0xd8, 0xf6, 0x64, 0x6, 0xb6, 0x26 } } > - > -%} we wanted to keep retrieval for backward compatibility ::: accessible/xpcom/xpcAccessibilityService.cpp:141 (Diff revision 2) > + return NS_OK; > +} > + > +NS_IMETHODIMP > +xpcAccessibilityService::GetStringStates(uint32_t aState, uint32_t aExtraState, > + nsISupports **aStringStates) I'd rather copied original file to keep history
https://reviewboard.mozilla.org/r/64340/#review61892 ::: accessible/base/DocManager.cpp:96 (Diff revision 2) > { > + RemoveListeners(aDOMDocument); > + > + if (nsAccessibilityService::IsShutdown()) { > + return; > + } can you comment here what's reasoning behind this, for both changes ::: accessible/base/DocManager.cpp:562 (Diff revision 2) > + if (xpcDoc) { > + xpcDoc->Shutdown(); > + } > + > + iter.Remove(); > - } > + } merging artifact? ::: accessible/base/nsAccessibilityService.cpp (Diff revision 2) > #undef ROLE > } > > -NS_IMETHODIMP > +void > -nsAccessibilityService::GetStringStates(uint32_t aState, uint32_t aExtraState, > - nsISupports **aStringStates) it'd be good to keep GetStringXXX methods in one place for consistence. ::: accessible/xpcom/xpcAccessibilityService.h:48 (Diff revision 2) > + static void EnsureAccService(); > + > + /** > + * Reference for xpc accessibility service instance. > + */ > + static xpcAccessibilityService* gAccessibilityService; since we are going to have both nsAcceessibilityService and xpcAccessibilityService, then gAccessibilityService is ambigious the way you use.
https://reviewboard.mozilla.org/r/60074/#review61648 > not sure I caught how it works as I don't see where a11y service is created in content Here's the original code: https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp?q=dom%2Fipc%2FContentChild.cpp&redirect_type=direct#2520-2521 I update that bit in the relevant e10s patch.
https://reviewboard.mozilla.org/r/64336/#review61656 > it seems it goes from another patch It might be, I just tried to split logically, with some minor issues like this one (product of a split). I was plannong on squashing the patches together when landing anyways.. If that's no good, I can get them split more carefully.
Attachment #8771077 - Flags: review?(dbolter)
https://reviewboard.mozilla.org/r/64340/#review61660 > why? That was thinking we looked at together when I just started working on this bug. There were a number of assertions associated with shutting down nsAccessibleService and it trying to shutdown doc accessibles and xpc doc accessibles. Let me know if you do not remember.
https://reviewboard.mozilla.org/r/64340/#review61892 > can you comment here what's reasoning behind this, for both changes See the comment above. > merging artifact? No we it's again related what we looked at. We need to make sure we are shutting down all xpc documents when a11y service is shutting down.
Comment on attachment 8764037 [details] Bug 527003 - make sure a11y shutdown and start work with e10s. When accessibility service is requested, created, or shutdown, indicate if it is done by parent process, platform API or XPCOM. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60074/diff/3-4/
Comment on attachment 8771075 [details] Bug 527003 - making sure a11y shutdown and start work with e10s. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64334/diff/1-2/
Comment on attachment 8771076 [details] Bug 527003 - renaming all XPCOM uses of deprecated accessible retrieval to accessibility service. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64336/diff/1-2/
Comment on attachment 8771077 [details] Bug 527003 - adding tests for accessibility service shutdown (both e10s and non-e10s). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64338/diff/1-2/
Comment on attachment 8771078 [details] Bug 527003 - removing nsIAccessibleRetrieval, separating xpcom parts from nsAccessibilityService. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64340/diff/2-3/
Comment on attachment 8764037 [details] Bug 527003 - make sure a11y shutdown and start work with e10s. When accessibility service is requested, created, or shutdown, indicate if it is done by parent process, platform API or XPCOM. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60074/diff/4-5/
Comment on attachment 8771075 [details] Bug 527003 - making sure a11y shutdown and start work with e10s. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64334/diff/2-3/
Comment on attachment 8771076 [details] Bug 527003 - renaming all XPCOM uses of deprecated accessible retrieval to accessibility service. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64336/diff/2-3/
Comment on attachment 8771077 [details] Bug 527003 - adding tests for accessibility service shutdown (both e10s and non-e10s). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64338/diff/2-3/
Comment on attachment 8771078 [details] Bug 527003 - removing nsIAccessibleRetrieval, separating xpcom parts from nsAccessibilityService. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64340/diff/3-4/
Marco, so the try build link is in Comment 65, give it a shot if you have a moment. Thanks!
Flags: needinfo?(mzehe)
(In reply to Yura Zenevich [:yzen] from comment #66) > Marco, so the try build link is in Comment 65, give it a shot if you have a > moment. Thanks! Looks like a warning ended up being an error during the build. Will re-submit tomorrow. Sorry, Marco.
Flags: needinfo?(mzehe)
Comment on attachment 8764037 [details] Bug 527003 - make sure a11y shutdown and start work with e10s. When accessibility service is requested, created, or shutdown, indicate if it is done by parent process, platform API or XPCOM. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60074/diff/5-6/
Comment on attachment 8771075 [details] Bug 527003 - making sure a11y shutdown and start work with e10s. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64334/diff/3-4/
Comment on attachment 8771076 [details] Bug 527003 - renaming all XPCOM uses of deprecated accessible retrieval to accessibility service. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64336/diff/3-4/
Comment on attachment 8771077 [details] Bug 527003 - adding tests for accessibility service shutdown (both e10s and non-e10s). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64338/diff/3-4/
Comment on attachment 8771078 [details] Bug 527003 - removing nsIAccessibleRetrieval, separating xpcom parts from nsAccessibilityService. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64340/diff/4-5/
Comment on attachment 8771078 [details] Bug 527003 - removing nsIAccessibleRetrieval, separating xpcom parts from nsAccessibilityService. https://reviewboard.mozilla.org/r/64340/#review62908 r=me with fixed/addressed ::: accessible/base/nsAccessibilityService.cpp:1778 (Diff revision 5) > return accessible.forget(); > } > #endif > > +nsAccessibilityService* > +GetOrCreateAccService(bool aIsStartedByPlatform) maybe aIsPlatformCaller instead aIsStartedByPlatform? ::: accessible/base/nsAccessibilityService.cpp:1796 (Diff revision 5) > + service->Shutdown(); > + return nullptr; > + } > + > + mozilla::a11y::statistics::A11yInitialized(); > + NS_ADDREF(service); this is scary part and likely wrong, I think you shouldn't addref it, since you don't do that above for cached version. I would do if (!gAccServ) { RefPtr<AccServ> serv = new AccServ(); if (!serv->Init() { serv->Shutdown(); return nullptr; } } MOZ_ASSERT(gAccServ); return gAccServ; ::: accessible/xpcom/xpcAccessibilityService.cpp:27 (Diff revision 5) > + > +void > +xpcAccessibilityService::ShutdownCallback(nsITimer* aTimer, void* aClosure) > +{ > + xpcAccessibilityService* xpcAccService = > + reinterpret_cast<xpcAccessibilityService*>(aClosure); it'd say to define the variable where it's used ::: accessible/xpcom/xpcAccessibilityService.cpp:41 (Diff revision 5) > + } > +} > + > +void > +xpcAccessibilityService::EnsureAccService() { > + GetOrCreateAccService(false); why don't you do GetOrCreateAccServ() directly where you need? ::: accessible/xpcom/xpcAccessibilityService.cpp:56 (Diff revision 5) > + nsrefcnt count = ++mRefCnt; > + NS_LOG_ADDREF(this, count, "xpcAccessibilityService", sizeof(*this)); > + > + if (mRefCnt > 1) { > + EnsureAccService(); > + } could you do that in constructor? or somebody keeps a reference to the object forever until firefox shuts down? ::: accessible/xpcom/xpcAccessibilityService.cpp:83 (Diff revision 5) > + mRefCnt = 1; /* stabilize */ > + delete (this); > + return 0; > + } > + > + if (count == 1 && !mShutdownTimer) { who is that '1'? ::: accessible/xpcom/xpcAccessibilityService.cpp:108 (Diff revision 5) > + return NS_OK; > +} > + > +NS_IMETHODIMP > +xpcAccessibilityService::GetAccessibleFor(nsIDOMNode *aNode, > + nsIAccessible **aAccessible) nit: type* here and below
Attachment #8771078 - Flags: review?(surkov.alexander) → review+
Attachment #8771077 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8771077 [details] Bug 527003 - adding tests for accessibility service shutdown (both e10s and non-e10s). https://reviewboard.mozilla.org/r/64338/#review62918 r=me with ref stuff fixed/explained, seconding r? to David, who's more expierenced in e10s part ::: dom/ipc/ContentChild.cpp:2516 (Diff revision 4) > } > return true; > } > > bool > -ContentChild::RecvActivateA11y() > +ContentChild::RecvActivateA11y(const bool& aIsStartedByPlatform) why this one is a referece?
Flags: needinfo?(mzehe)
https://reviewboard.mozilla.org/r/64338/#review62918 > why this one is a referece? As metnioned in IRC, I was following the pattern with other Recv methods.
https://reviewboard.mozilla.org/r/64340/#review62908 > nit: type* here and below I think this is best to be kept to a separate patch as I'm simply moving the method from the retrieval. Hope that's ok.
Comment on attachment 8764037 [details] Bug 527003 - make sure a11y shutdown and start work with e10s. When accessibility service is requested, created, or shutdown, indicate if it is done by parent process, platform API or XPCOM. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60074/diff/6-7/
Comment on attachment 8771075 [details] Bug 527003 - making sure a11y shutdown and start work with e10s. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64334/diff/4-5/
Comment on attachment 8771076 [details] Bug 527003 - renaming all XPCOM uses of deprecated accessible retrieval to accessibility service. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64336/diff/4-5/
Comment on attachment 8771077 [details] Bug 527003 - adding tests for accessibility service shutdown (both e10s and non-e10s). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64338/diff/4-5/
Comment on attachment 8771078 [details] Bug 527003 - removing nsIAccessibleRetrieval, separating xpcom parts from nsAccessibilityService. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64340/diff/5-6/
https://reviewboard.mozilla.org/r/64340/#review62908 > this is scary part and likely wrong, I think you shouldn't addref it, since you don't do that above for cached version. > > I would do > > if (!gAccServ) { > RefPtr<AccServ> serv = new AccServ(); > if (!serv->Init() { > serv->Shutdown(); > return nullptr; > } > } > MOZ_ASSERT(gAccServ); > return gAccServ; I believe we need should hold a reference when setting a static gAccessibilityService (I moved NS_ADDREF in the init) and it will correspond to NS_RELEASE in shutdown.
Comment on attachment 8764037 [details] Bug 527003 - make sure a11y shutdown and start work with e10s. When accessibility service is requested, created, or shutdown, indicate if it is done by parent process, platform API or XPCOM. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60074/diff/7-8/
Attachment #8771077 - Flags: review?(dbolter) → review?(tbsaunde+mozbugs)
Comment on attachment 8771075 [details] Bug 527003 - making sure a11y shutdown and start work with e10s. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64334/diff/5-6/
Comment on attachment 8771076 [details] Bug 527003 - renaming all XPCOM uses of deprecated accessible retrieval to accessibility service. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64336/diff/5-6/
Comment on attachment 8771077 [details] Bug 527003 - adding tests for accessibility service shutdown (both e10s and non-e10s). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64338/diff/5-6/
Comment on attachment 8771078 [details] Bug 527003 - removing nsIAccessibleRetrieval, separating xpcom parts from nsAccessibilityService. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64340/diff/6-7/
Comment on attachment 8771078 [details] Bug 527003 - removing nsIAccessibleRetrieval, separating xpcom parts from nsAccessibilityService. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64340/diff/7-8/
Comment on attachment 8764037 [details] Bug 527003 - make sure a11y shutdown and start work with e10s. When accessibility service is requested, created, or shutdown, indicate if it is done by parent process, platform API or XPCOM. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60074/diff/8-9/
Comment on attachment 8771075 [details] Bug 527003 - making sure a11y shutdown and start work with e10s. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64334/diff/6-7/
Comment on attachment 8771076 [details] Bug 527003 - renaming all XPCOM uses of deprecated accessible retrieval to accessibility service. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64336/diff/6-7/
Comment on attachment 8771077 [details] Bug 527003 - adding tests for accessibility service shutdown (both e10s and non-e10s). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64338/diff/6-7/
Comment on attachment 8771078 [details] Bug 527003 - removing nsIAccessibleRetrieval, separating xpcom parts from nsAccessibilityService. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64340/diff/8-9/
Attachment #8771078 - Attachment is obsolete: true
Alex could you take a look at the two patches that are not tests or renaming changes and if they look good to be landed separately ?
Flags: needinfo?(surkov.alexander)
(In reply to Yura Zenevich [:yzen] from comment #102) > Alex could you take a look at the two patches that are not tests or renaming > changes and if they look good to be landed separately ? it should be ok to land them separately as long as IsPlatformCaller is true by default
Flags: needinfo?(surkov.alexander)
Pushed by yura.zenevich@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/13c9ca2db921 separating XPCOM parts from nsAccessibilityService. Removing a11y service in favour of using nsAccessibilityService directly. Adding support for a11y service shutdown. r=surkov
Pushed by yura.zenevich@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/79d61727522a renaming all XPCOM uses of deprecated accessible retrieval to accessibility service. r=surkov
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4a9cd25be38 Fixing tests: forcing garbage collection and ensuring there are no timeouts
Trevor, would you have a sec to take a look at the IPC part, it's the only one not landed ATM and can be applied independently.
Flags: needinfo?(tbsaunde+mozbugs)
Comment on attachment 8771075 [details] Bug 527003 - making sure a11y shutdown and start work with e10s. Sorry about the lag, the windows e10s reviews took a really long time. >@@ -295,6 +300,7 @@ private: > friend mozilla::a11y::SelectionManager* mozilla::a11y::SelectionMgr(); > friend mozilla::a11y::ApplicationAccessible* mozilla::a11y::ApplicationAcc(); > friend mozilla::a11y::xpcAccessibleApplication* mozilla::a11y::XPCApplicationAcc(); >+ friend class mozilla::dom::ContentChild; This seems unnecessary, and really shouldn't be necessary. >+ContentChild::RecvActivateA11y(const bool& aIsPlatformCaller) this aPlatformCaller thing is a pretty bad name, it doesn't really have anything to do with the call stack. > { > #ifdef ACCESSIBILITY > // Start accessibility in content process if it's running in chrome > // process. >- GetOrCreateAccService(); >+ GetOrCreateAccService(aIsPlatformCaller); I don't think if accessibility is being enabled for a platform API is the important question here. It seems to me if the main process enables accessibility then the child process should enable accessibility until the main process disables it. consider what happens if some js thing enables a11y in the main process, but then doesn't poke at it in the child process or only looks at the tree for the child process in the main process. I think with this patch after a while a11y will get disabled in the child process and everyone will be confused why it isn't accessible any more. >@@ -546,7 +546,12 @@ child: > /** > * Start accessibility engine in content process. > */ >- async ActivateA11y(); >+ async ActivateA11y(bool isPlatformCaller); I don't know that the argument makes sense as above, but if you keep it it should be documented. >+ >+ /** >+ * Shutdown accessibility engine in content process. >+ */ >+ async ShutdownA11y(); worth noting if its not also in use in the content process.
Flags: needinfo?(tbsaunde+mozbugs)
Attachment #8771075 - Flags: review?(tbsaunde+mozbugs) → review-
Attachment #8771075 - Attachment is obsolete: true
Attachment #8771076 - Attachment is obsolete: true
Attachment #8771077 - Attachment is obsolete: true
Attachment #8764037 - Flags: review+ → review?(surkov.alexander)
Comment on attachment 8764037 [details] Bug 527003 - make sure a11y shutdown and start work with e10s. When accessibility service is requested, created, or shutdown, indicate if it is done by parent process, platform API or XPCOM. This doesn't handle a11y being enabled for multiple reasons well. One way to fix this would be to just use the ref count of nsAccessibilityService to keep it alive, take a ref in PlatformInit, ContentChild::EnableAccessibility, and xpcAccessibilityService, and release in the obvious places, but that isn't wonderful because you need to deal with several platforms. You could have a couple flags for is platform a11y in use, is xpcom in use, is the main process using a11y, and do the apropriate things you could skip the case of parent process and platform both in use and assert it never happens. There's also the problem that if xpcom keeps listening for accessibility events but drops all references to the service then the shutdown timer will fire, but not actually shut anything down. However that problem is kind of unrelated, and probably hard to fix without using a different event mechanism or getting the observer service to tell you when event listeners are added and removed. You can also come up with some other way of fixing the multiple consumer problems than the ones above if you have something better. >commit d6c16b5a2d1790d9f5fc7f322b31c41b30bed958 >Author: Yura Zenevich <yzenevich@mozilla.com> >Date: Tue Aug 16 14:16:20 2016 -0400 > > Bug 527003 - making sure a11y shutdown and start work with e10s. r=surkov, tbsaunde commit message could use improvement, at least s/making/make/ but it would be nice if you said what you are doing and why. >-nsAccessibilityService::Init() >+nsAccessibilityService::Init(mozilla::a11y::EFlag aFlag) Why are you changing to pass it here to set it? > nsAccessibilityService* >-GetOrCreateAccService(bool aIsPlatformCaller) >+GetOrCreateAccService(mozilla::a11y::EFlag aFlag) > { >- if (aIsPlatformCaller) { >- nsAccessibilityService::gIsPlatformCaller = aIsPlatformCaller; >- } >- > if (!nsAccessibilityService::gAccessibilityService) { > RefPtr<nsAccessibilityService> service = new nsAccessibilityService(); >- if (!service->Init()) { >+ if (!service->Init(aFlag)) { now you only know the first reason this was called and that's not enough to correctly decide if you can shut down. >+MaybeShutdownAccService(mozilla::a11y::EFlag aFlag) The aFlag name isn't really useful, aWhatShutdown maybe? not really great either. > { > nsAccessibilityService* accService = nsAccessibilityService::gAccessibilityService; >- if (!accService) { >- return false; >+ if (!accService || accService->IsShutdown()) { >+ return; >+ } >+ >+ if (nsAccessibilityService::gAccServiceFlag == aFlag && that is incorrect, consider a main process where first xpcom accessibility is enabled, then platform accessibility is enabled, and finally xpcom accessibility shuts down. In that case you'd shutdown acessibility, but shouldn't. >+ !nsCoreUtils::AccEventObserversExist() && >+ !xpcAccessibilityService::IsInUse()) { >+ accService->Shutdown(); >+ return; >+ } >+ >+ if (aFlag == eIsUsedByParent && XRE_IsContentProcess()) { >+ nsAccessibilityService::gAccServiceFlag = mozilla::a11y::eIsUsedByJSAPI; this doesn't handle the case that js shuts down in the main process, but there is still platform accessibility in use. > } >+/** >+ * A list of possible shutdown/initialized flags for accessibility service. there not really flags, but any way more importantly you can be in two of these states at once and it should at least be documented how that works. >+ */ >+enum EFlag EFlag is a bad name since it says nothing about what the type is for. Call it ServiceState maybe? Also putting it in nsAccessibilityService would let you type mozilla::a11y:: less which would be nice. >- bool Init(); >+ bool Init(mozilla::a11y::EFlag aFlag = mozilla::a11y::eIsUsedByPlatformAPI); is there some reason that arg needs a default? otherwise please remove it. >+ * Indicates a current state of accessibility service. s/a current/the current/ >+void MaybeShutdownAccService( >+ mozilla::a11y::EFlag aFlag = mozilla::a11y::eIsUsedByParent); why does this need a default? >@@ -47,7 +44,7 @@ xpcAccessibilityService::AddRef(void) > NS_LOG_ADDREF(this, count, "xpcAccessibilityService", sizeof(*this)); > > if (mRefCnt > 1) { >- GetOrCreateAccService(false); >+ GetOrCreateAccService(mozilla::a11y::eIsUsedByJSAPI); I don't see why this needs to exist at all off hand, but whatever. >+ GetOrCreateAccService(mozilla::a11y::eIsUsedByJSAPI); I'd say used by xpcom but whatever. >+ContentChild::RecvShutdownA11y() >+{ >+#ifdef ACCESSIBILITY >+ // Try to shutdown accessibility in content process if it's shutting down in >+ // chrome process. >+ MaybeShutdownAccService(); it'd be clearer without using a default argument.
Attachment #8764037 - Flags: review?(tbsaunde+mozbugs) → review-
Comment on attachment 8781644 [details] Bug 527003 - adding tests for accessibility service shutdown (both e10s and non-e10s). what has been changed from the last review? any specific stuff you want me to look at?
Comment on attachment 8781644 [details] Bug 527003 - adding tests for accessibility service shutdown (both e10s and non-e10s). https://reviewboard.mozilla.org/r/72026/#review70254 please make name changes as we discussed in persons, otherwise looks good
Attachment #8781644 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8764037 [details] Bug 527003 - make sure a11y shutdown and start work with e10s. When accessibility service is requested, created, or shutdown, indicate if it is done by parent process, platform API or XPCOM. https://reviewboard.mozilla.org/r/60074/#review70258 please improve enum commenting as we dicussed in person, and consider to replace enum on a structure to keep states flipping logic in one place, otherwise looks good
Attachment #8764037 - Flags: review?(surkov.alexander) → review+
Pushed by yura.zenevich@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/40dc3b9f1677 adding tests for accessibility service shutdown. r=surkov
Trevor, the patch should be OK to review now. Thanks
Flags: needinfo?(tbsaunde+mozbugs)
r=me if you fix the below. >commit fe7cb40ed79e4a11b50416d506f343cb5ab699a8 >Author: Yura Zenevich <yzenevich@mozilla.com> >Date: Mon Aug 29 11:23:11 2016 -0400 > > Bug 527003 - make sure a11y shutdown and start work with e10s. When accessibility service is requested, created, or shutdown, indicate if it is done by parent process, platform API or XPCOM. r=surkov, tbsaunde that's pretty long for one line, and doesn't really explain why you took this approach. Most of https://github.com/git/git/blob/master/Documentation/SubmittingPatches is worth reading accept of course the git.git specific parts. That said I think in light of wanting to keep nsAccessibilityService around while there are existing xpcom accessible objects using the refcount of nsAccessibilityService would be a bettter approach. However this approach seems correct so I guess its fine for now. > nsAccessibilityService::~nsAccessibilityService() > { >- NS_ASSERTION(gIsShutdown, "Accessibility wasn't shutdown!"); >+ NS_ASSERTION(!gConsumers, "Accessibility wasn't shutdown!"); gConsumers == 0 is probably clearer. >-GetOrCreateAccService(bool aIsPlatformCaller) >+GetOrCreateAccService(uint32_t aRequestedBy) aRequester might be better english? >+MaybeShutdownAccService(uint32_t aRequestedBy) kind of funny name since its what request is going away. > { > nsAccessibilityService* accService = nsAccessibilityService::gAccessibilityService; >- if (!accService) { >- return false; >+ if (!accService || >+ accService->IsShutdown() || >+ !(nsAccessibilityService::gConsumers & aRequestedBy)) { that seems like the wrong test, you want to bail out if there are any outstanding requesters other than the one going away. >+ return; >+ } >+ >+ if (!nsCoreUtils::AccEventObserversExist() && >+ !xpcAccessibilityService::IsInUse()) { >+ accService->Shutdown(); // Will unset all nsAccessibilityService::gConsumers >+ return; >+ } >+ >+ if (aRequestedBy & nsAccessibilityService::eMainProcess) { >+ nsAccessibilityService::gConsumers &= ~nsAccessibilityService::eMainProcess; > } I'd say first mask out the request type going away then check if gConsumers is 0. Also you should be more generic than just eMainProcess going away, and support whatever request going away. >+ static bool IsShutdown() { { on next line >+ /** >+ * A list of possible accessibility service consumers. Accessibility service >+ * can be shut down by a consumer only when it is exclusively used by that >+ * consumer. Seems easier to say it can only be shut down when there are no remaining consumers. >+ * >+ * eXPCOM - accessibility service is used by XPCOM. It can only be shut >+ * down if it's only used by XPCOM and not by main process >+ * and/or platform API. >+ * >+ * eMainProcess - accessibility service was started by main process in the >+ * content process. It can be shut down by main process if it's >+ * not also used by XPCOM. >+ * >+ * ePlatformAPI - accessibility service is used by the platform api in the >+ * main process. It can never can never be shut down. This seems like a bunch of words that don't add much you could just say what each enum value is then above comment about only shutting down when no consumers are left applies for the rest of this. >+ * Contains a list of accessibility service consumers. s/list/set/ >+nsAccessibilityService* GetOrCreateAccService( >+ uint32_t aRequestedBy = nsAccessibilityService::ePlatformAPI); I'd rather that didn't have a default, but it already had one so ok.
Flags: needinfo?(tbsaunde+mozbugs)
Attachment #8764037 - Flags: review?(tbsaunde+mozbugs) → review+
Pushed by yura.zenevich@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8f96a0ce280f make a11y service store info about its consumers. Only shut down a11y when there are no more consumers remaining. r=surkov, tbsaunde https://hg.mozilla.org/integration/mozilla-inbound/rev/cc3d60b79315 adding tests for accessibility service shutdown (both e10s and non-e10s). r=surkov
All dependencies are finished.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Keywords: leave-open
Target Milestone: --- → mozilla53
Depends on: 1330739
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: