Closed
Bug 1175913
Opened 9 years ago
Closed 9 years ago
Crash in mozilla::a11y::DocAccessibleParent::RemoveAccessible(ProxyAccessible* aAccessible)
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: lsocks, Assigned: lsocks)
References
Details
(Keywords: crash)
Crash Data
Attachments
(3 files, 7 obsolete files)
7.12 KB,
text/x-c++src
|
Details | |
2.04 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
12.98 KB,
patch
|
lsocks
:
review+
|
Details | Diff | Splinter Review |
I found this crash while trying to reproduce bug 1100602. No reliable way to reproduce yet, but it's been captured in rr.
Assignee | ||
Comment 1•9 years ago
|
||
So far it looks like we're creating duplicate ProxyAccessibles with the same ID. This seems to be happening when nodes are reparented and a new node is inserted between an existing parent and child. When this happens, the children ProxyAccessibles are being created anew instead of using the existing ones, so when they are shut down, we try to remove them from the DocAccessibleParent twice which throws the assertion failure.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → lorien
Assignee | ||
Comment 2•9 years ago
|
||
Caused when click event listeners are added to a previously non-accessible node, making it accessible. Since we don't listen to event listener changes, we don't properly handle this case.
Updated•9 years ago
|
Crash Signature: [@ mozilla::a11y::DocAccessibleParent::RemoveAccessible(ProxyAccessible* aAccessible)]
Keywords: crash
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8635521 -
Flags: feedback?(tbsaunde+mozbugs)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8635521 -
Attachment is obsolete: true
Attachment #8635521 -
Flags: feedback?(tbsaunde+mozbugs)
Attachment #8635523 -
Flags: feedback?(tbsaunde+mozbugs)
Assignee | ||
Comment 5•9 years ago
|
||
forgot to rename test2 when I renamed the tests
Attachment #8635523 -
Attachment is obsolete: true
Attachment #8635523 -
Flags: feedback?(tbsaunde+mozbugs)
Attachment #8635528 -
Flags: feedback?(tbsaunde+mozbugs)
Comment 6•9 years ago
|
||
Comment on attachment 8635528 [details]
recreate accessibles when relevant event listeners change
>+nsAccessibilityService::ListenersChanged(nsIArray* aEventChanges)
>+{
>+ nsCOMPtr<nsIEventListenerService> eventListenerService =
>+ do_GetService("@mozilla.org/eventlistenerservice;1");
>+ if (!eventListenerService)
>+ return NS_ERROR_FAILURE;
I'd say just assert it. caring about the xpcom idea this can fail is silly.
>+
>+ uint32_t targetCount;
>+ nsresult rv = aEventChanges->GetLength(&targetCount);
>+ if (NS_WARN_IF(NS_FAILED(rv))) {
>+ return rv;
>+ }
NS_ENSURE_SUCCESS please
>+
>+ for (uint32_t i = 0 ; i < targetCount ; i++) {
>+ nsCOMPtr<nsIEventListenerChange> change = do_QueryElementAt(aEventChanges, i);
>+ nsIDOMEventTarget* target;
nsCOMPtr please
>+ change->GetTarget(&target);
>+ nsIArray* listenerNames;
same
>+ change->GetChangedListenerNames(&listenerNames);
>+
>+ uint32_t changeCount;
>+ rv = listenerNames->GetLength(&changeCount);
>+ if (NS_WARN_IF(NS_FAILED(rv))) {
>+ return rv;
>+ }
NS_ENSURE_SUCCESS please
>+
>+ for (uint32_t i = 0 ; i < changeCount ; i++) {
>+ nsCOMPtr<nsIAtom> listenerName = do_QueryElementAt(listenerNames, i);
>+
>+ // We are only interested in event listener changes which may
>+ // make an element accessible or inaccessible.
>+ if (listenerName == nsGkAtoms::onclick ||
>+ listenerName == nsGkAtoms::onmousedown ||
>+ listenerName == nsGkAtoms::onmouseup) {
>+
>+ nsCOMPtr<nsIContent> node(do_QueryInterface(target));
>+ if (!node) {
>+ return NS_OK;
>+ }
>+
>+ nsCOMPtr<nsIDocument> ownerDoc = node->OwnerDoc();
I don't see a reason to hold a ref to the document so nsIDocument* is fine.
>+ if (!ownerDoc) {
>+ return NS_OK;
>+ }
not needed it can never fail.
Attachment #8635528 -
Flags: feedback?(tbsaunde+mozbugs)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8635528 -
Attachment is obsolete: true
Attachment #8636101 -
Flags: review?(tbsaunde+mozbugs)
Comment 8•9 years ago
|
||
Comment on attachment 8636101 [details] [diff] [review]
recreate accessibles when relevant event listeners change
># HG changeset patch
># User Lorien Hu <lorien@lorienhu.com>
># Date 1437413726 14400
># Mon Jul 20 13:35:26 2015 -0400
># Node ID 9a787e7db7c4fbecdc2a091bd21f898b532223fc
># Parent 5df788c56ae77e1a2e906535c5ef26837c22f5a2
>Bug 1175913 - Subscribe to EventListenerService and recreate accessibles on click listener changes
>
>diff --git a/accessible/base/nsAccessibilityService.cpp b/accessible/base/nsAccessibilityService.cpp
>--- a/accessible/base/nsAccessibilityService.cpp
>+++ b/accessible/base/nsAccessibilityService.cpp
>@@ -17,16 +17,17 @@
> #include "HTMLLinkAccessible.h"
> #include "HTMLListAccessible.h"
> #include "HTMLSelectAccessible.h"
> #include "HTMLTableAccessibleWrap.h"
> #include "HyperTextAccessibleWrap.h"
> #include "RootAccessible.h"
> #include "nsAccessiblePivot.h"
> #include "nsAccUtils.h"
>+#include "nsArrayUtils.h"
> #include "nsAttrName.h"
> #include "nsEventShell.h"
> #include "nsIURI.h"
> #include "OuterDocAccessible.h"
> #include "Platform.h"
> #include "Role.h"
> #ifdef MOZ_ACCESSIBILITY_ATK
> #include "RootAccessibleWrap.h"
>@@ -269,23 +270,74 @@ nsAccessibilityService::nsAccessibilityS
>
> nsAccessibilityService::~nsAccessibilityService()
> {
> NS_ASSERTION(gIsShutdown, "Accessibility wasn't shutdown!");
> gAccessibilityService = nullptr;
> }
>
> ////////////////////////////////////////////////////////////////////////////////
>+// nsIListenerChangeListener
>+
>+NS_IMETHODIMP
>+nsAccessibilityService::ListenersChanged(nsIArray* aEventChanges)
>+{
>+ uint32_t targetCount;
>+ nsresult rv = aEventChanges->GetLength(&targetCount);
>+ NS_ENSURE_SUCCESS(rv, rv);
>+
>+ for (uint32_t i = 0 ; i < targetCount ; i++) {
>+ nsCOMPtr<nsIEventListenerChange> change = do_QueryElementAt(aEventChanges, i);
>+ nsCOMPtr<nsIDOMEventTarget> target;
>+ change->GetTarget(getter_AddRefs(target));
>+ nsCOMPtr<nsIArray> listenerNames;
seems like blank line before this var would be typical.
>+ change->GetChangedListenerNames(getter_AddRefs(listenerNames));
>+
>+ uint32_t changeCount;
>+ rv = listenerNames->GetLength(&changeCount);
>+ NS_ENSURE_SUCCESS(rv, rv);
>+
>+ for (uint32_t i = 0 ; i < changeCount ; i++) {
>+ nsCOMPtr<nsIAtom> listenerName = do_QueryElementAt(listenerNames, i);
>+
>+ // We are only interested in event listener changes which may
>+ // make an element accessible or inaccessible.
>+ if (listenerName == nsGkAtoms::onclick ||
>+ listenerName == nsGkAtoms::onmousedown ||
>+ listenerName == nsGkAtoms::onmouseup) {
>+
and this one seems unnecessary
>+ nsCOMPtr<nsIContent> node(do_QueryInterface(target));
>+ if (!node) {
>+ return NS_OK;
>+ }
does that really make sense? I'm not sure if you can get these particular events on non nsIContent things, but you certainly can get other events on non nodes, and then I'd think you just want to skip this target's changes.
>+
>+ nsIDocument* ownerDoc = node->OwnerDoc();
>+ DocAccessible* document = GetExistingDocAccessible(ownerDoc);
>+
>+ if (document) {
>+ document->RecreateAccessible(node);
so you are possibly going to call RecreateAccessible multiple times for the same thing right? that seems pointless.
>+ }
>+ }
>+ }
>+ NS_RELEASE(target);
>+ NS_RELEASE(listenerNames);
these release those objects too many times now.
>+ // Subscribe to EventListenerService.
>+ nsCOMPtr<nsIEventListenerService> eventListenerService =
>+ do_GetService("@mozilla.org/eventlistenerservice;1");
>+ if (!eventListenerService)
>+ return false;
>+ eventListenerService->AddListenerChangeListener(this);
blank line after return please
Attachment #8636101 -
Flags: review?(tbsaunde+mozbugs)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8636101 -
Attachment is obsolete: true
Attachment #8636733 -
Flags: review?(tbsaunde+mozbugs)
Comment 10•9 years ago
|
||
Comment on attachment 8636733 [details] [diff] [review]
recreate accessibles when relevant event listeners change
>+ for (uint32_t i = 0 ; i < targetCount ; i++) {
>+ nsCOMPtr<nsIEventListenerChange> change = do_QueryElementAt(aEventChanges, i);
>+ nsCOMPtr<nsIDOMEventTarget> target;
>+ change->GetTarget(getter_AddRefs(target));
you could move this down to just before you QI it to nsINOde right?
Attachment #8636733 -
Flags: review?(tbsaunde+mozbugs) → review+
Assignee | ||
Comment 11•9 years ago
|
||
carry r=tbsaunde
Attachment #8636733 -
Attachment is obsolete: true
Attachment #8639953 -
Flags: review+
Assignee | ||
Comment 12•9 years ago
|
||
Fix test and only recreate for HTML accessibles.
Attachment #8639953 -
Attachment is obsolete: true
Attachment #8640621 -
Flags: review?(tbsaunde+mozbugs)
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8641704 -
Flags: review?(tbsaunde+mozbugs)
Comment 14•9 years ago
|
||
Comment on attachment 8641704 [details] [diff] [review]
don't expect accessible recreation on onclick change if accessible already exists
I'd rather test the accessible didn't get recreated, but I'm not sure how to do that.
Attachment #8641704 -
Flags: review?(tbsaunde+mozbugs) → review+
Comment 15•9 years ago
|
||
Comment on attachment 8640621 [details] [diff] [review]
recreate accessibles when relevant event listeners change
># HG changeset patch
># User Lorien Hu <lorien@lorienhu.com>
># Date 1437507758 14400
># Tue Jul 21 15:42:38 2015 -0400
># Node ID c883a1dc4e6efd12e3345f9776526b3cfee54383
># Parent 2ee9895e032c492705adaf213706d4260ca172c8
>Bug 1175913 - Subscribe to EventListenerService and recreate accessibles on click listener changes r=tbsaunde
>* * *
>try: -b do -p all -u mochitest-o -t none
>* * *
>try: -b do -p all -u mochitest-o -t none
>
>diff --git a/accessible/base/nsAccessibilityService.cpp b/accessible/base/nsAccessibilityService.cpp
>--- a/accessible/base/nsAccessibilityService.cpp
>+++ b/accessible/base/nsAccessibilityService.cpp
>@@ -17,16 +17,17 @@
> #include "HTMLLinkAccessible.h"
> #include "HTMLListAccessible.h"
> #include "HTMLSelectAccessible.h"
> #include "HTMLTableAccessibleWrap.h"
> #include "HyperTextAccessibleWrap.h"
> #include "RootAccessible.h"
> #include "nsAccessiblePivot.h"
> #include "nsAccUtils.h"
>+#include "nsArrayUtils.h"
> #include "nsAttrName.h"
> #include "nsEventShell.h"
> #include "nsIURI.h"
> #include "OuterDocAccessible.h"
> #include "Platform.h"
> #include "Role.h"
> #ifdef MOZ_ACCESSIBILITY_ATK
> #include "RootAccessibleWrap.h"
>@@ -269,23 +270,83 @@ nsAccessibilityService::nsAccessibilityS
>
> nsAccessibilityService::~nsAccessibilityService()
> {
> NS_ASSERTION(gIsShutdown, "Accessibility wasn't shutdown!");
> gAccessibilityService = nullptr;
> }
>
> ////////////////////////////////////////////////////////////////////////////////
>+// nsIListenerChangeListener
>+
>+NS_IMETHODIMP
>+nsAccessibilityService::ListenersChanged(nsIArray* aEventChanges)
>+{
>+ uint32_t targetCount;
>+ nsresult rv = aEventChanges->GetLength(&targetCount);
>+ NS_ENSURE_SUCCESS(rv, rv);
>+
>+ for (uint32_t i = 0 ; i < targetCount ; i++) {
>+ nsCOMPtr<nsIEventListenerChange> change = do_QueryElementAt(aEventChanges, i);
>+
>+ nsCOMPtr<nsIArray> listenerNames;
>+ change->GetChangedListenerNames(getter_AddRefs(listenerNames));
>+
>+ uint32_t changeCount;
>+ rv = listenerNames->GetLength(&changeCount);
>+ NS_ENSURE_SUCCESS(rv, rv);
>+
>+ for (uint32_t i = 0 ; i < changeCount ; i++) {
>+ nsCOMPtr<nsIAtom> listenerName = do_QueryElementAt(listenerNames, i);
>+
>+ // We are only interested in event listener changes which may
>+ // make an element accessible or inaccessible.
>+ if (listenerName == nsGkAtoms::onclick ||
>+ listenerName == nsGkAtoms::onmousedown ||
>+ listenerName == nsGkAtoms::onmouseup) {
seems like continue is better than indenting the rest of the function
>+ nsCOMPtr<nsIDOMEventTarget> target;
>+ change->GetTarget(getter_AddRefs(target));
>+ nsCOMPtr<nsIContent> node(do_QueryInterface(target));
>+ if (!node || !node->IsHTMLElement()) {
>+ break;
>+ }
I wonder if it would be better to hoist this above the loop
>+
>+ nsIDocument* ownerDoc = node->OwnerDoc();
>+ DocAccessible* document = GetExistingDocAccessible(ownerDoc);
>+
>+ if (document) {
>+ nsString testName;
>+ listenerName->ToString(testName);
unused?
>+
>+ if (nsCoreUtils::HasClickListener(node)) {
>+ if (!document->GetAccessible(node)) {
>+ document->RecreateAccessible(node);
its tempting to just use document->ContentInserted()
>+++ b/accessible/base/nsAccessibilityService.h
>@@ -10,16 +10,18 @@
>
> #include "mozilla/a11y/DocManager.h"
> #include "mozilla/a11y/FocusManager.h"
> #include "mozilla/a11y/Role.h"
> #include "mozilla/a11y/SelectionManager.h"
> #include "mozilla/Preferences.h"
>
> #include "nsIObserver.h"
>+#include "nsIEventListenerService.h"
>+#include "nsIArray.h"
isn't a forward decl of nsIArray enough?
Attachment #8640621 -
Flags: review?(tbsaunde+mozbugs) → review+
Assignee | ||
Comment 16•9 years ago
|
||
carry r=tbsaunde
Attachment #8640621 -
Attachment is obsolete: true
Attachment #8643462 -
Flags: review+
Comment 17•9 years ago
|
||
Comment 18•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/89002bda1f55
https://hg.mozilla.org/mozilla-central/rev/6d03329ef85e
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•